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(AbstractWidget): clear activeState #2794

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

floryst
Copy link
Collaborator

@floryst floryst commented Apr 18, 2023

Context

activeState should not be set after calling deactivateAllHandles.

Results

activeState is cleared after calling deactivateAllHandles.

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:
    • OS:
    • Browser:

activeState should not be set after calling deactivateAllHandles.
@floryst floryst requested a review from jourdain April 18, 2023 18:55
Copy link
Collaborator

@jourdain jourdain left a comment

Choose a reason for hiding this comment

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

Don't recall the logic but LGTM

@floryst
Copy link
Collaborator Author

floryst commented Apr 18, 2023

The only weird case I can think of is what should happen when you do the following:

widgetManager.grabFocus(widget);
widget.deactivateAllHandles();

Though if you are doing manual activate/deactivate of handles, maybe you don't intend on using grabFocus/loseFocus.

@@ -32,6 +32,7 @@ function vtkAbstractWidget(publicAPI, model) {

publicAPI.deactivateAllHandles = () => {
model.widgetState.deactivate();
model.activeState = null;
Copy link
Member

Choose a reason for hiding this comment

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

deactivateAllHandles() being a convenient function and not the only way to deactivate handles, I wonder if a more robust fix wouldn't be :

  • for the widget to observe the activeState, and if it becomes inactive,then set activeState to null.
  • or when deactivating a substate in the widget state, it could check if it is the same as widget.getActiveState() of the associated widget and if so, set it to null.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I like the watch approach better, since I agree with your comment that it's not the only way to deactivate handles.

One thing we would have to watch out for with this change is how we handle grab/loseFocus. Suppose we do the following:

const polylineWidget = vtkPolyLineWidget.newInstance()
const w = widgetManager.addWidget(polylineWidget)
// this calls widgetState.getMoveHandle().activate()
widgetManager.grabFocus(w)

// sets model.activeState = null
w.deactivateAllHandles()

In this state, the widget manager still thinks the widget is focused, so all events go to the PolyLineWidget. However, since the widget's activeState is now null, no mouse event is processed.

Copy link
Member

Choose a reason for hiding this comment

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

Can the widget in focus be different than the activeState ?
if not, then deactivating the widget in focus could also make it loose focus.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I certainly think so. An example would be a widget that processes mouse events while in focus but there is no handle state. A concrete example of this would be the PolyLineWidget, but there's no move handle.

The concern is mostly for existing widgets, e.g. a focused PolyLineWidget means you're drawing, but no active state means nothing happens. This is more of a "developer beware" note that I think would be good to document.

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.

3 participants