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

vtkMRMLSubjectHierarchyNode: Use of StartModify / EndModify leads to warnings. #4650

Open
slicerbot opened this issue Mar 13, 2020 · 5 comments
Assignees
Labels
Priority: Medium Issues that we plan to fix but there are available workarounds Type: Bug Something isn't working correctly
Milestone

Comments

@slicerbot
Copy link
Collaborator

This issue was created automatically from an original Mantis Issue. Further discussion may take place here.

@slicerbot slicerbot added this to the Slicer 4.11.0 milestone Mar 13, 2020
@lassoan lassoan added Priority: Medium Issues that we plan to fix but there are available workarounds Type: Bug Something isn't working correctly labels Mar 26, 2020
@lassoan lassoan self-assigned this Mar 26, 2020
@cpinter
Copy link
Member

cpinter commented Aug 26, 2020

The issue here seems more than just a few error messages.

The errors are logged because when the InvokePendingModifiedEvent function calls the pending events, it calls them without an argument, and that becomes an invalid item ID (0) in the callback. So StartModify/EndModify cannot really work with custom modified events that use arguments.

I am not sure what would be a good way to proceed with this one. Maybe just not treating the SubjectHierarchyItemModifiedEvent as a modified event, or disable StartModify/EndModify for the subject hierarchy node?

@pieper
Copy link
Member

pieper commented Aug 26, 2020

If the widget gets an EndModify event it should assume that it needs to rebuild from scratch, since there's no way to know what it missed during the batch operation.

@lassoan
Copy link
Contributor

lassoan commented Aug 26, 2020

I agree, modify event with 0 ID means batch update = rebuild the widget from scratch (but preserving selection state).

@cpinter
Copy link
Member

cpinter commented Aug 26, 2020

Thank you! Sounds like a conceptually easy solution. Not sure though if it's easy to implement as well because the plugin logic where this event is handled has not interacted with the model directly. I'll take a look soon.

@pieper
Copy link
Member

pieper commented Aug 26, 2020

yes, good point about selection state. Also the scroll state.

@lassoan lassoan modified the milestones: Slicer 4.13.0, Backlog Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium Issues that we plan to fix but there are available workarounds Type: Bug Something isn't working correctly
Development

No branches or pull requests

4 participants