Fixing unwanted tree group node expansion selection bug

[credmond]

Fixing bug introduced in #9728 which undid/broke this improvement: #8785, which prevented the group node from being selected when the user is just attempting to expand or shrink the tree node. Now, once again, the mouse click is handled and consumed at the capture stage if the expansion pane is clicked, therefore preventing the node from being selected.

Mandatory checks

• Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
• Tests created for changes (if applicable)
• Manually tested changed features in running JabRef (always required)
• Screenshots added in PR description (for UI changes)
• Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
• Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

[credmond]

@Siedlerchr: had to fix two comment typos (apologies, was a rushed PR) -- can you re-approve please?

[calixtus]

Thanks for investigating the issue underneath and for your PR.

However, I'm not that happy with the implementation yet, as this modifies a common utility class to fix a very specific bug and also it is not clear, what the boolean flag means from the viewpoint of the user. I will try to look into it in the next days and would ask the other reviewers not yet to merge this PR.

[credmond]

Currently ViewModelTreeTableRowFactory is used only in one class (GroupTreeView) and was introduced only in March for unspecified reasons (which introduced the bug).

I don't like the fix either (I didn't actually want to raise a PR, my time is limited to do any major refactoring), but I also don't like the constraints a utility class such as ViewModelTreeTableRowFactory enforces (and the many variations of it I've noticed). The idea of the class is obviously a good thing, but you cannot control where you listen: capture or bubbling phase. Would be good to have a common pattern or approach (for all of these factory utils) with a way to allow you to specify whether what you need is a listener vs. handler. Happy to implement it whatever way you want, if it isn't a mountain of work...

[calixtus]

I introduced this utility class back then, because we already have a pattern of similar utility classes and I wanted to clean up the GroupTreeView class, which was a mess back then.
About the issue itself, i was wondering why the already defined checks in GroupTreeView for the style class and the event.consume method is not working (

jabref/src/main/java/org/jabref/gui/groups/GroupTreeView.java

Lines 227 to 233 in 5e4cb4e

	.withOnMousePressedEvent((row, event) -> {
	if (event.getTarget() instanceof StackPane pane) {
	if (pane.getStyleClass().contains("arrow") || pane.getStyleClass().contains("tree-disclosure-node")) {
	event.consume();
	}
	}
	})

), but was interrupted today when I wanted to debug to understand the issue better. maybe I can look deeper the next days

[credmond]

I explained this a little bit in the comment here: #8785

The event.consume() is working in main, it's just too late (bubbling phase) -- the node has already selected itself during the capture phase.

It worked before your refactor in March because it was using an event filter, which is called during the capture stage, and consumed before the node could select itself. But your March change, unknowingly, changed it to use an event handler, which is called during the bubbling stage; that is too late as the node is already selected.

• Filters = capture phase
• Handlers = bubbling phase

E.g.:

• Capture chain = row -> child -> child's children, etc...
• Bubble phase is after that, in reverse: child's children -> child -> row

My PR changes it back to using an event filter, and it works again.

Example

E.g.: the arrow gets clicked, and something like the following happens:

• An event is created/dispatched.

The main branch with the bug is this:

• Capture chain:
  • row -> child (event processed and unwanted select of group here, and continues) -> child's child -> etc...
• Bubble phase is after that, in reverse:
  • etc -> child's child (event.consume() here, for example; but it's too late/pointless) -> child -> row

My PR (and pre-March) is this:

• Capture chain:
  • row (consume event here to stop it, manually expands node) -> child -> child's child -> etc...
• Bubble phase is after that, in reverse
  • etc... -> child's child -> child -> row

The boolean controlling this is also documented in src/main/java/org/jabref/gui/util/ViewModelTreeTableRowFactory.java:

// True if event capture should be at capture phase via an event filter, otherwise use default Node method to setup event handler (bubbling phase)

[calixtus]

I worked a little bit on your PR and modified the utility class to also forward event filters. Tested it, the bug is still fixed, but we can avoid the boolean and and the a misunderstandable withOnMouseClicked method by providing a more generic utility class.

[calixtus]

Thanks for your effort debugging this issue and providing the solution rationale.

[credmond]

I worked a little bit on your PR and modified the utility class to also forward event filters. Tested it, the bug is still fixed, but we can avoid the boolean and and the a misunderstandable withOnMouseClicked method by providing a more generic utility class.

Looks good!