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

GPII-3440: debouncing onZoomChange event. #21

Merged
merged 7 commits into from Oct 24, 2018

Conversation

Projects
None yet
2 participants
@jobara
Copy link
Collaborator

commented Oct 18, 2018

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 18, 2018

@amb26 could you please take a look at this PR?

// Debouncing the event relay.
// GPII-3440: To prevent multiple open windows from triggering conflicting zoom changes and causing an infinite
// loop of onZoomChange events.
var relayZoomChange = fluid.debounce(that.events.onZoomChanged.fire, 100);

This comment has been minimized.

Copy link
@amb26

amb26 Oct 22, 2018

Member

Is this really the best strategy for thinning out the events? I am wondering if they are distinguished in some way in terms of which browser window they were targetted at and could perhaps be deduplicated that way. Do you happen to have a trace of what the event storm looks like? It would be helpful to be able to attach it to the parent JIRA.

Also, the file "zoom.js" could do with a good comment block explaining what the purpose of the file is and the strategy it uses for coordinating model state, etc.

This comment has been minimized.

Copy link
@jobara

jobara Oct 23, 2018

Author Collaborator

The problem is that when you make a change to the browser's zoom, via its own control mechanisms, it will update the zoom and then coordinate that update with UIO+. UIO+ will then push out these changes to all of the browser tabs. Because we are listening to the onZoomChanged event from all tabs, we then have this reported back. With the ctrl+scroll changes, it for some reason will try to change all Windows at the same time, even those that don't have focus. This isn't the case with using ctrl-(+/-); which only happens on focus. This combined with the rapid changes via ctrl-scroll causes the endless cycling between zoom factors.

I have explored another approach. The onZoomChanged event will include the TabID. The TabID can be used to query the information for the Tab. This information includes the WindowID. We can use this WindowID to query the information of the Window and determine if it has focus. However, the querying of the Tab and Window are both asynchronous. What I've found is that the issue became worse. It would even occur with a single window open.

This comment has been minimized.

Copy link
@jobara

jobara Oct 24, 2018

Author Collaborator

Okay, I've addressed this by making use of excludeSource. When we update the model from browser driven zoom changes, the tabs are only updated when they are opened or focused.

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

I notice that none of the zoom listeners which we bind ever get unbound - if this creates a limitation on the use of the component, this should be explained

@amb26

This comment has been minimized.

Copy link
Member

commented Oct 22, 2018

Sorry, ignore the last comment, I see that removal is handled within the chromeEvented grade

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 24, 2018

@amb26 this PR is ready for more review.

@jobara

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 24, 2018

@amb26 the error messages from gpii.chrome.zoom.applyZoomInTab is not covered in the tests because the chrome.tabs.setZoom function is mocked and always succeeds.

@amb26 amb26 merged commit 12a48d8 into GPII:master Oct 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.