Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updateConfig when move slider too #145

Merged

Conversation

ericfont
Copy link
Contributor

onchange only would trigger updateConfig when user releases the mouse from sliders.

Adding oninput also allows updateConfig when dragging the sliders, which is nice cause user gets immediate feedback of the changes.

A big reason why I want this change is because the LED Lamp is very bright, and previously when user dragged the light slider, the user wouldn't get any feedback about just how bright the LED would become, and that presents a safety risk, cause that LED is bright!

@ericfont
Copy link
Contributor Author

hmm...though I can't seem to get consistent correct behavior with Stop Stream & Start Stream. This probably messed that up, and so maybe that button needs to not have this change.

@ericfont
Copy link
Contributor Author

The Stop/Start stream button seems to work when first reboot. However the moment I adjust any slider, then that Stop/Start stream button is broken because of my code.

@ericfont ericfont force-pushed the el-oninput-update-config branch from 0c1e96c to dc47637 Compare July 21, 2021 05:37
onchange only would trigger updateConfig when user releases the mouse from sliders. Adding oninput also allows updateConfig when dragging the sliders, which provides immediate feedback of the changes for the user.
@ericfont ericfont force-pushed the el-oninput-update-config branch from dc47637 to 3f13480 Compare July 21, 2021 05:41
@ericfont
Copy link
Contributor Author

ericfont commented Jul 21, 2021

ok, I updated so triggers oninput only for range type inputs, I think. I don't notice the glitch I mentioned in my previous comment. However, I can cause the Stop/Start stream button to break if I do a lot of fiddling around with sliders and also press Stop/Start stream a bunch of times...though I'm not even sure that that is my code causing that glitch. Maybe it there are simply too many updates then the streaming simply fails to work.

@easytarget
Copy link
Owner

easytarget commented Jul 21, 2021

Cool! I hate the lack of updating while dragging that slider too, It hadn't occurred to me it could be done like this, good idea.
I'll look at this on my dev PC later, it may be that adding an minimum interval time to the update sending (150ms would seem reasonable) stops the glitching while remaining responsive to the user.
I'll play with your branch and see what happens.

@easytarget easytarget changed the base branch from master to interactive-slider September 24, 2021 13:35
@easytarget easytarget merged commit a536b94 into easytarget:interactive-slider Sep 24, 2021
@easytarget
Copy link
Owner

Merging onto a branch of my own to test

@easytarget
Copy link
Owner

easytarget commented Sep 25, 2021

Looking at this via browser developer tools 'network' tab, and the console (where lamp commands are logged), I can clearly see that a really large number of config update packets are being sent as the user moves the slider, and since this is an asynchronous connection they are arriving out of order at the esp32 itself.. Hence the laggyness, glitching and, sometimes, the lamp being left on when the last packet arrives out of order.

I've got a fix in mind and half-implemented, it's a bit fiddly but will, essentially, rate limit the packets. However it needs more coding and testing.

easytarget pushed a commit that referenced this pull request Sep 25, 2021
onchange only would trigger updateConfig when user releases the mouse from sliders. Adding oninput also allows updateConfig when dragging the sliders, which provides immediate feedback of the changes for the user.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants