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

8292922 - [Linux] No more drag events when new Stage is created in drag handler #977

Closed
wants to merge 30 commits into from

Conversation

tsayao
Copy link
Collaborator

@tsayao tsayao commented Dec 18, 2022

This PR was previously discussed on #905.

The approach is to grab the keyboard focus so the window that originated the drag will keep it.

I did some cleanup on grabbing related functions as well.

gdk_keyboard_focus() is deprecated, so is gdk_device* functions in favor of gdk_seat*. But that's only available in later Gtk versions. I checked and newer Gtk will use gdk_seat* inside the deprecated gdk_keyboard_focus().

Edit:

The current changes uses another approach that is to not ungrab pointer device when focus is received on another window. There's also some cleanup on grabbing related functions and grab moved from starting on mouse click to the actual drag.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

  • JDK-8292922: [Linux] No more drag events when new Stage is created in drag handler

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/977/head:pull/977
$ git checkout pull/977

Update a local copy of the PR:
$ git checkout pull/977
$ git pull https://git.openjdk.org/jfx pull/977/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 977

View PR using the GUI difftool:
$ git pr show -t 977

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/977.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 18, 2022

👋 Welcome back tsayao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Dec 18, 2022

⚠️ @tsayao This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk openjdk bot added the rfr Ready for review label Dec 18, 2022
@mlbridge
Copy link

mlbridge bot commented Dec 18, 2022

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Dec 19, 2022

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

@tsayao
Copy link
Collaborator Author

tsayao commented Dec 19, 2022

I had another idea to approach this, will test tonight if time permits.

One thing that bugs me is that it's grabbing on click, it probably should grab on drag detection. Let me know it this change would be welcome.

@kevinrushforth
Copy link
Member

One thing that bugs me is that it's grabbing on click, it probably should grab on drag detection. Let me know it this change would be welcome.

This seems like a good idea.

@andy-goryachev-oracle
Copy link
Contributor

andy-goryachev-oracle commented Dec 19, 2022

While I cannot meaningfully review this PR, I can attest to the fact that this change fixes JDK-8292922 (and andy-goryachev/FxDock#6)

Verified by running FxDock.

  • dragging a dockable tab creates a new stage and the user is able to move it
  • dropping the said stage successfully creates a new dockable window in the right position
  • the dropped window is laid on top of any existing windows

It is hard to see any other effects such as flicker (running in a VirtualBox). I ddi not notice any focus-related issues.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

approving. if you want to add sizing changes and/or grab focus on drag start - I'll re-test.

@tsayao
Copy link
Collaborator Author

tsayao commented Dec 20, 2022

This one deserves a laught. Just commenting the ungrab_mouse_drag_focus(); on focus lost fixes the problem. I will rework it.

void WindowContextBase::process_focus(GdkEventFocus* event) {
    if (!event->in && WindowContextBase::sm_mouse_drag_window == this) {
        ungrab_mouse_drag_focus();
    }
//    if (!event->in && WindowContextBase::sm_mouse_drag_window == this) {
//        ungrab_mouse_drag_focus();
//    }
    if (!event->in && WindowContextBase::sm_grab_window == this) {
        ungrab_focus();
    }

The rationale is that glass_gdk_mouse_devices_grab_with_cursor grabs motion events so it keeps coming to the window, but there's an explicit "stop grabbing" on focus lost.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

re-tested the same scenarios. somehow it feels there is less flicker than before - it's hard to tell in a virtualized environment.

@tsayao
Copy link
Collaborator Author

tsayao commented Dec 21, 2022

I did a few tests and it works.
If anyone can name scenarios where mouse grab would make a difference it would help testing.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

All my testing looks good, too. Since there is no merge conflict between this and PR #915, I'll leave it up to you if you want to integrate this one first or wait for the other.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

I took a second look at the code that calls grab_mouse_drag_focus, which was changed to call it on a drag start rather than on a mouse click, and it looks like there is a problem with the way it is now implemented. It will now be called multiple times during a drag operation, instead of just at the start of the drag operation.

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

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

Looks good now. Thanks.

Copy link
Contributor

@andy-goryachev-oracle andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

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

re-tested with FxDock. beautiful!

@openjdk
Copy link

openjdk bot commented Dec 23, 2022

@tsayao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8292922: [Linux] No more drag events when new Stage is created in drag handler

Reviewed-by: angorya, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 7 new commits pushed to the master branch:

  • 48f6f5b: 8299272: Update copyright header for files modified in 2022
  • 5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
  • 1d9e2af: 8297068: Update boot JDK to 19.0.1
  • ac3f60c: 8297554: Remove Scene.KeyHandler
  • 9c52605: 8252863: Spinner keeps spinning if removed from Scene
  • bac8ee8: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed
  • ae86ed3: 8297067: Update Gradle to 7.6

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Dec 23, 2022
@tsayao
Copy link
Collaborator Author

tsayao commented Dec 23, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Dec 23, 2022

Going to push as commit a35c3bf.
Since your change was applied there have been 7 commits pushed to the master branch:

  • 48f6f5b: 8299272: Update copyright header for files modified in 2022
  • 5b96d34: 8296654: [macos] Crash when launching JavaFX app with JDK that targets SDK 13
  • 1d9e2af: 8297068: Update boot JDK to 19.0.1
  • ac3f60c: 8297554: Remove Scene.KeyHandler
  • 9c52605: 8252863: Spinner keeps spinning if removed from Scene
  • bac8ee8: 8296409: Multiple copies of accelerator change listeners are added to MenuItems, but only 1 is removed
  • ae86ed3: 8297067: Update Gradle to 7.6

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 23, 2022
@openjdk openjdk bot closed this Dec 23, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 23, 2022
@openjdk
Copy link

openjdk bot commented Dec 23, 2022

@tsayao Pushed as commit a35c3bf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
3 participants