Prevent group expansion arrow from activating group when expanding or collapsing #8785

LIM0000 · 2022-05-11T07:15:32Z

Previous discussion for group activation when expanding or collapsing group #7982

When I want to assign an entry to a subgroup, I have to first expand the parent group and then drag-and-drop my entry in the appropriate subgroup. The problem is that when I expand the group, by clicking on the arrow on the right, the group gets activated (the entry table only shows the group entries) and, therefore, I have to go back to "All entries" and locate my entry again. The parent group gets activated when collapsing it as well.

Proposal solution: Prevent group activation/select when user clicks on group expanding or collapsing arrow

Fixes: #7982
Fixes #3176

if (event.getButton() == MouseButton.SECONDARY || event.getTarget() instanceof StackPane) {
          // Prevent right-click to select group
          event.consume();
}

1.1.mp4

Further discussion

It might not be a good idea to only check eventTarget that returns StackPane class, as the group expand arrow is the only column that is using the StackPane class at current JabRef version. Any feedbacks are much appreciated!

PR Checklist:

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.

… collapsing

ThiloteE · 2022-05-11T09:37:21Z

Fixes #7982
Fixes #3176

@LIM0000 did you know about this? :-)

Edit: Sorry, did not see you already linked. Ups, my fault. Wanted to link 3176 too though :)

calixtus · 2022-05-17T06:02:38Z

I like the basic idea, I'm just thinking if "instanceof StackPane" is determined enough to prevent other side effects (e.g. the hits badge is a StackPane too)
Maybe you can test on the styleclass "tree-disclosure-node" as well?

…de-disclosure styleclass

…-issue-7982

LIM0000 · 2022-05-17T11:52:29Z

Thank you for your review @calixtus.

I agree that just instanceof StackPane is not determined enough. I have fixed this issue in the latest commit by confirming if the targeted StackPane has either arrow or tree-disclosure-node style class.

The reason that tree-disclosure-node included is because StackPane arrow style class has a very small clickable area. It will be very tedious to click directly onto the arrow icon and prevent activating group.

Clickable area if only include arrow

Clickable area (red) if include both area and tree-disclosure-node

calixtus · 2022-05-17T13:35:36Z

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

@@ -266,7 +266,11 @@ private void initialize() {
                            .map(this::createContextMenuForGroup)
                            .orElse((ContextMenu) null));
            row.addEventFilter(MouseEvent.MOUSE_PRESSED, event -> {
-                if (event.getButton() == MouseButton.SECONDARY || event.getTarget() instanceof StackPane) {
+                if (event.getTarget() instanceof StackPane) {


Use pattern matching to avoid unnecessary casts

if (event.getTarget() instanceof StackPane pane) {

calixtus · 2022-05-18T07:48:33Z

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

@@ -269,6 +269,10 @@ private void initialize() {
                if (event.getButton() == MouseButton.SECONDARY) {
                    // Prevent right-click to select group
                    event.consume();
+                } else if (event.getTarget() instanceof StackPane pane) {
+                    if (pane.getStyleClass().toString().equals("arrow") || pane.getStyleClass().toString().equals("tree-disclosure-node")) {


Maybe it's better to check for contains, because there could also be multiple style classes assigned.

Thanks @calixtus , it has been updated with contains().
Really appreciate your review!

…-issue-7982

Siedlerchr · 2022-05-22T14:30:06Z

LGTM! Is this fix also for #3176 ?

…-issue-7982

LIM0000 · 2022-05-22T15:02:13Z

Hi @Siedlerchr , I believe that #3176 has also been fixed based on #7982 (comment) and #7982 (comment).
I have updated CHANGELOG.md file that stated fixes for #7982 and #3176.
Cheers.

Siedlerchr · 2022-05-22T15:26:59Z

Thanks again for your contributions! Superb so far! We really appreciate your engagement

credmond · 2023-07-24T00:03:25Z

Hey @Siedlerchr @LIM0000

This fix/improvement has been reversed/broken by this commit:

bc662cb

...because the event is being intercepted and consumed, now via an EventHandler (bubbling phase) rather than capture phase, as was previous. So it's too late, the node has already been selected as before.

E.g., row.addEventFilter(MouseEvent.MOUSE_PRESSED ...) is gone with the re-factor in the above link.

Siedlerchr · 2023-07-24T04:49:36Z

Thanks for the hint. Feel free to create a PR with a fix Cormac Redmond ***@***.***> schrieb am Mo., 24. Juli 2023, 02:03:

…

Hey @Siedlerchr <https://github.com/Siedlerchr> @LIM0000 <https://github.com/LIM0000> This fix/improvement has been reversed/broken by this commit: bc662cb <bc662cb> ...because the event is being intercepted and consumed, now via an EventHandler (bubbling phase) rather than capture phase, as was previous. So it's too late, the node has already been selected as before. E.g., row.addEventFilter(MouseEvent.MOUSE_PRESSED ...) is gone with the re-factor in the above link. — Reply to this email directly, view it on GitHub <#8785 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOFZBW7GCUA4JC3GLBTH3XRW3VRANCNFSM5VT4N6MQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…nt: JabRef#8785. Now, once again, the mouse click is handled and consumed at the capture stage if the expansion pane is licked, therefore preventing the node from being selected.

credmond · 2023-07-24T10:48:31Z

@Siedlerchr, done: #10111

Prevent group expansion arrow from activating group when expanding or…

4b47dd6

… collapsing

ThiloteE added ui groups labels May 13, 2022

LIM0000 added 3 commits May 17, 2022 21:02

Update solution by picking StackPane that has either arrow or tree-no…

012f42b

…de-disclosure styleclass

Merge branch 'main' of https://github.com/LIM0000/jabref into fix-for…

4531b20

…-issue-7982

Check secondary mousebutton, then only check if is StackPane class

5d3ab67

calixtus reviewed May 17, 2022

View reviewed changes

Pattern matching to remove unnecessary StackPane cast

04bb61c

calixtus reviewed May 18, 2022

View reviewed changes

Update equal() to contains() as object could have multiple styles

47fbbff

LIM0000 marked this pull request as ready for review May 19, 2022 23:25

LIM0000 added 2 commits May 22, 2022 23:27

Merge branch 'main' of https://github.com/LIM0000/jabref into fix-for…

ac34096

…-issue-7982

Update CHANGELOG.md

81f25f8

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 22, 2022

Siedlerchr approved these changes May 22, 2022

View reviewed changes

LIM0000 added 2 commits May 23, 2022 00:26

Merge branch 'main' of https://github.com/LIM0000/jabref into fix-for…

4720baa

…-issue-7982

Update CHANGELOG.md

de1f675

Siedlerchr merged commit a355534 into JabRef:main May 22, 2022

credmond mentioned this pull request Jul 24, 2023

Fixing unwanted tree group node expansion selection bug #10111

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent group expansion arrow from activating group when expanding or collapsing #8785

Prevent group expansion arrow from activating group when expanding or collapsing #8785

LIM0000 commented May 11, 2022 •

edited by Siedlerchr

Loading

ThiloteE commented May 11, 2022 •

edited

Loading

calixtus commented May 17, 2022

LIM0000 commented May 17, 2022

calixtus May 17, 2022

calixtus May 18, 2022

LIM0000 May 18, 2022

Siedlerchr commented May 22, 2022

LIM0000 commented May 22, 2022

Siedlerchr commented May 22, 2022

credmond commented Jul 24, 2023

Siedlerchr commented Jul 24, 2023 via email

credmond commented Jul 24, 2023

Prevent group expansion arrow from activating group when expanding or collapsing #8785

Prevent group expansion arrow from activating group when expanding or collapsing #8785

Conversation

LIM0000 commented May 11, 2022 • edited by Siedlerchr Loading

Previous discussion for group activation when expanding or collapsing group #7982

Proposal solution: Prevent group activation/select when user clicks on group expanding or collapsing arrow

Further discussion

PR Checklist:

ThiloteE commented May 11, 2022 • edited Loading

calixtus commented May 17, 2022

LIM0000 commented May 17, 2022

calixtus May 17, 2022

Choose a reason for hiding this comment

calixtus May 18, 2022

Choose a reason for hiding this comment

LIM0000 May 18, 2022

Choose a reason for hiding this comment

Siedlerchr commented May 22, 2022

LIM0000 commented May 22, 2022

Siedlerchr commented May 22, 2022

credmond commented Jul 24, 2023

Siedlerchr commented Jul 24, 2023 via email

credmond commented Jul 24, 2023

LIM0000 commented May 11, 2022 •

edited by Siedlerchr

Loading

ThiloteE commented May 11, 2022 •

edited

Loading