Skip to content

Conversation

luaceseb
Copy link

PR and Code Checklist

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

Context

The paintWidget does not follow the guidelines of ignoring the alt, control and shift keys like the other widgets. Example DistanceWidget

Changes

  • Add function ignoreKey that checks if the alt, control or shift keys are pressed

Results

  • The painting action does not work if any of the alt, control or shift keys are pressed when painting begins

Testing

  • This change adds or fixes unit tests
  • All tests complete without errors on the following environment:
    • vtk.js: v20.2.1
    • OS: Windows 10
    • Browser: Chrome 94.0.4606.81

Copy link
Contributor

@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.

LGTM overall. Just would like to remove a stray comment.

publicAPI.handleLeftButtonPress = (callData) => {
if (!model.activeState || !model.activeState.getActive()) {
// if (!model.activeState || !model.activeState.getActive()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this comment.

@finetjul
Copy link
Member

Actually, the shape widget handles mouse moves differently depending on Shift and Ctrl (see modifierBehavior). Those modifier keys can enforce squared shape, or specify that handle 1 is the center of the shape vs a corner of the shape...

Why not having those ignoreKeys is a problem for you ?

@luaceseb
Copy link
Author

In our application, we use those keys+mouse for camera handling. Zoom, Pan, Rotation

@finetjul
Copy link
Member

Why not having those ignoreKeys is a problem for you ?
In our application, we use those keys+mouse for camera handling. Zoom, Pan, Rotation

Gotcha.
I still think the shape widget should be smarter than just discarding up-front events if alt, shift or ctrl is down. It should be conditional, based on the supported modifier keys in modifierBehavior.

@finetjul finetjul added the module: widget Gaps and errors in interactive widgets label Nov 4, 2021
@floryst
Copy link
Contributor

floryst commented Nov 15, 2021

Re-visiting this PR:

  • @finetjul does the shape/spline widget support runtime config of ignoring actions when modifier keys are pressed?
  • @luaceseb please rebase onto master and fix the listed conflicts. If @finetjul agrees that this is good moving forward, then we will merge.

@finetjul
Copy link
Member

finetjul commented Nov 16, 2021

@floryst ShapeWidget handles alt, shift or ctrl modifiers. Therefore ignoreKeys should look at modifierBehavior, e.g.:

function ignoreKey(e) {
    const supportedModifiers = Object.keys(model.getFactory().getModifierBehavior());
    return e.altKey && supportedModifiers.includes('Alt') ||
               e.controlKey && supportedModifiers.includes('Control') ||
               e.shiftKey && supportedModifiers.includes('Shift');
  }

(code not tested)

@floryst
Copy link
Contributor

floryst commented Nov 18, 2021

Thanks for the update. @luaceseb please look to using modifier behaviors to implement your ignoreKey functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: widget Gaps and errors in interactive widgets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants