Skip to content

Bind handleExpansionToggleDblClickEvent in TreeWidget#9877

Merged
colin-grant-work merged 2 commits intoeclipse-theia:masterfrom
scottaxcell:9876-unbound-tree-expansion-on-click-handler
Aug 13, 2021
Merged

Bind handleExpansionToggleDblClickEvent in TreeWidget#9877
colin-grant-work merged 2 commits intoeclipse-theia:masterfrom
scottaxcell:9876-unbound-tree-expansion-on-click-handler

Conversation

@scottaxcell
Copy link

@scottaxcell scottaxcell commented Aug 10, 2021

What it does

Fixes #9876 .

How to test

See #9876 steps to reproduce.

Review checklist

Reminder for reviewers

Signed-off-by: Scott Axcell scott.axcell@xilinx.com

@vince-fugnitto vince-fugnitto added the tree issues related to the tree (ex: tree widget) label Aug 10, 2021
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @scottaxcell, that looks like it does the job. I have one issue with this though:

This is a breaking change, as it does not allow to override handleExpansionToggleDblClickEvent as a method in subclasses of TreeWidget anymore. You would instead have to override doHandleExpansionToggleDblClickEvent.

I would usually recommend to instead simply replace its calling site with event => this.handleExpansionToggleDblClickEvent(event) in line 575. However, your approach keeps everything in line with other methods like toggle which is implemented in the same way. So I don't really care which way you go, but going with your current one requires you to describe your breaking changes in the changelog.

@scottaxcell
Copy link
Author

@msujew thanks for the feedback. In keeping with the current convention, I'll leave the change as is and update the changelog.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I can confirm that the issue exists on master and is nicely addressed by this change:

  • Double clicking a toggle node on master leads to an error stating: this is undefined.
  • Double clicking a toggle node with the changes in place correctly stops the propagation of the event.

@colin-grant-work
Copy link
Contributor

@vince-fugnitto, would you be willing to restart the tests for this PR? I don't believe this code is responsible for the failure, but always nice to have all green.

@colin-grant-work colin-grant-work merged commit efd8263 into eclipse-theia:master Aug 13, 2021
dna2github pushed a commit to dna2fork/theia that referenced this pull request Aug 25, 2021
…9877)

Ensure `handleExpansionToggleDblClickEvent` correctly bound in TreeWidget

Co-authored-by: Scott Axcell <scott.axcell@xilinx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tree issues related to the tree (ex: tree widget)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tree expansion toggle on click handler not bound correctly

4 participants