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

UI flickering (loading toast displayed) if mouse is hold-own on color picker #35

Closed
mwittig opened this issue Dec 22, 2015 · 16 comments
Closed
Labels

Comments

@mwittig
Copy link
Collaborator

mwittig commented Dec 22, 2015

) if i hold down the mouse on the color wheel, the pimatic GUI is kind of blinking because of the "loading"-window. I cant really describe it so i made a video. https://youtu.be/Agt1bTkHmhw

See also: #27

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 22, 2015

@thymian I am able to reproduce this issue now. I am not sure whether this is to due recent changes in pimatic-led-light or it is due to some subtle changes in modible frontend code. I'll investigate this further as soon as I can.

@philip1986
Copy link
Owner

The blinking issue might be introduced by a change I did a while ago and you published recently.

See this and the following lines.

It delays a call to the light device until the call before is done and the callback was fired. Further it flushes the queue if there are already two commands waiting for execution and you fire another one. It was working well with my setup when I tested it, but I could easily imagine that this leeds to the loading toast.

A quick solution could be reducing the max queue length to 1.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 22, 2015

@philip1986 Thanks for pointing this out! I'll give it a try later on.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 23, 2015

@philip1986 I have tried the suggested change but it did not change anything. The point is, a new queue is created anyway with each call to _onLocalChange() which I think is not intended. Thus I changed the implementation to create the queue once as part in the constructor. However, this did not help to get around the flickering problem.

I have also tried the "old implementation" using the debounce logic which works pretty well. May be it is better to switch back to this for the timing being. What do you think?

@philip1986
Copy link
Owner

@mwittig the method name _onLocalChange() is a bit misleading, better would be _attachListenerOnLocalChange(). The method is executed after rendering and I thing the class itself is instanced during the rendering. The main purpose of this method is to attach an jQuery listener, which pushes tho the queue each time something changes.

The problem I see with the old debounce logic is: that is dosn't take in-flight calls in account, but we could which back to it as last resort.

But, did you also try to reduce the timeout? Also in success case it waits for 500ms to fire the callback and execute the next task in the queue. I thing we could lower this in success cases.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 23, 2015

@philip1986

The method is executed after rendering and I thing the class itself is instanced during the rendering.

Ah, OK. Looking at the generated Javascript code I realize my assumption was wrong.
The _onLocalChange() is called three times to register a "change" event listener. Thus the queue is created three times and each listener has its own queue held by the closure. So, there is one queue for on/off actions, one for color change actions, and on for white mode. Well, I think this make sense as you don't want to kill a pending on/off action if there are too many change color actions pending.

The problem I see with the old debounce logic is: that is dosn't take in-flight calls in account, but we could which back to it as last resort.

Yes. The debounce logic will skip the previous call if the timeout has not been reached.

But, did you also try to reduce the timeout?
Yes, I did lower it to 50ms (see async-queue) branch.

@philip1986
Copy link
Owner

@mwittig you still see the loading toast with 50ms?

We could also try to add some kind of debounce logic for the call of ajaxShowToast, specially in case of the dim slider it doesn't make sense to show it after each change.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 24, 2015

@philip1986 Yes I do see it even at 50ms. I have also commented the toast calls out, but I still git the "loading" toast which causes the flickering.

@philip1986
Copy link
Owner

@mwittig how did you reproduce the issue? In the next days I will have some time to also have a deeper look in to it.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 28, 2015

@philip1986 I am just using the current release as issue. See video link on the first issue post. If I change the color the "loading ..." toast always pops up for few milliseconds as shown in the video. It does not happend if I switch back to the old debounce logic.

Btw, I have added a debounce logig to the Milight driver. Along with the old debounce logic in the UI class it works pretty well - no hicups, no delays.

@philip1986
Copy link
Owner

I think this #40 should fix it.

Sometimes I really hate CoffeeScript!

@mwittig I not sure about the debounce logic in the Milight driver, but it looks like the promises are not returned properly e.g. this line: Promise.resolve() is in the scope on an cb function, so it will not provide the return value for setWhite

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 30, 2015

@philip1986 I've tried your fix #40. Works great for me! 👍

Regarding result handling from debounced functions in the Milight driver I did this on purpose, but I agree this need to be ellaborated for a general sollution.

I think we should make a new release. Let me know what you think.

@mwittig mwittig added the bug label Dec 30, 2015
@philip1986
Copy link
Owner

@mwittig nice, thank you for testing.

I think the debounce logic is Milight is just not nessacy. I checked all other device and all of then return a resolved promise (also iwy_master), so no we don't care about any feedback from any device!

Regarding a new release, I currently work also on some further UI improvements. So I would suggest to wait for a couple of days and make a bigger release.

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 31, 2015

I think the debounce logic is Milight is just not nessacy. I checked all other device and all of then return a resolved promise (also iwy_master), so no we don't care about any feedback from any device!

I have tested this without the debounce logic and with some tweak of the Milight driver parameters it is working pretty well without the debounce logic. It is still slightly lagging as the Milight driver is a little bit slower than what is assumed by front end. I have reverted the debounce commit for now and may re-introduce it at later stage.

@philip1986
Copy link
Owner

👍

@mwittig
Copy link
Collaborator Author

mwittig commented Dec 31, 2015

0.3.3

@mwittig mwittig closed this as completed Dec 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants