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

fix(handleWheel): mouse scrolling behavior for out-of-focus windows #2992

Merged

Conversation

jadh4v
Copy link
Collaborator

@jadh4v jadh4v commented Jan 19, 2024

Brief Description

When rebinding events with a different container, make sure all timeouts and state variables from previous container are cleared.

This PR is part of a react-vtk-js fix for out-of-focus wheel events (Kitware/react-vtk-js#125)

Context

Results

Changes

  • Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

  • semantic-release commit messages
  • Run npm run reformat to have correctly formatted code

Testing

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js: latest master
    • OS: Windows 11, macOS Sonoma
    • Browser: Chrome 120

@jadh4v jadh4v force-pushed the fix-mouse-scrolling-out-of-focus-window branch 3 times, most recently from 3476a90 to b39d1c9 Compare January 24, 2024 17:52
Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While that works, I find the logic quite convoluted.
How about the following:

unbindEvents() {
  ...
  // Kept for backward compatibility purpose
  publicAPI.setContainer(null);
}
setContainer(newContainer) {
  if (newContainer === model.container) {
     return false;
  }
  unbindEvents();
  model.container = newContainer;
  bindEvents();
  return true;
}

// container parameter is for backward compatibility purpose
bindEvents(container = model.container){
  if (model.container != container) {
    setContainer(container)
    return;
  }
  ...
}
...
publicAPI.delete(
  ...
  setContainer(null);
}

And to not forget about modified() and _onContainerChanged() (see macro.js::findSetter(),

setContainer(newContainer) {
  if (newContainer === model.container) {
     return false;
  }
  unbindEvents();
  const res = superclass.setContainer(newContainer);
  bindEvents(model.container);
  return res;
}
...
macro.setGet(...[container]);

@jadh4v
Copy link
Collaborator Author

jadh4v commented Jan 24, 2024

Oh yes, thanks for reminding about onChanged and modified.

I don't agree on the new call tree you suggested. It is more complex to my eyes. Specially since setContainer() calls both bindEvents and unbindEvents and then bind\unbindEvents again call setContainer. It makes the code hard to read since there is a possible circular call which can only be disentangled through reasoning about the values of arguments/variables.

@jadh4v jadh4v force-pushed the fix-mouse-scrolling-out-of-focus-window branch 2 times, most recently from 16deccc to 4731fad Compare January 25, 2024 00:57
Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@floryst floryst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last change needed: the getContainer() type needs to be updated to be getContainer(): Nullable<HTMLElement>. You can also update the setContainer type to use Nullable for consistency.

Sources/Rendering/Core/RenderWindowInteractor/index.js Outdated Show resolved Hide resolved
When rebinding events with a different container,
make sure all timeouts and state variables from
previous containter are cleared.
@jadh4v jadh4v force-pushed the fix-mouse-scrolling-out-of-focus-window branch from 4731fad to dae07b6 Compare January 26, 2024 17:16
@floryst
Copy link
Collaborator

floryst commented Jan 26, 2024

LGTM

@floryst floryst added this pull request to the merge queue Jan 26, 2024
Merged via the queue into Kitware:master with commit 050019b Jan 26, 2024
3 checks passed
Copy link

🎉 This PR is included in version 29.4.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants