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

8269429: Linux: Only the last APPLICATION_MODAL window behaves correctly #551

Closed
wants to merge 20 commits into from

Conversation

tsayao
Copy link
Collaborator

@tsayao tsayao commented Jun 28, 2021

The PR approach is to set gtk_window_set_keep_above to true on APPLICATION_MODAL windows, so they will not stay behind non APPLICATION_MODAL windows.

This is passed on WindowStage.java:198 as a mask.

The weird thing is that _enterModal() is never called. This seems the right function to be called for APPLICATION_MODAL, as _enterModalWithWindow fits for WINDOW_MODAL.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8269429: Linux: Only the last APPLICATION_MODAL window behaves correctly

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/551/head:pull/551
$ git checkout pull/551

Update a local copy of the PR:
$ git checkout pull/551
$ git pull https://git.openjdk.java.net/jfx pull/551/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 551

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/551.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 28, 2021

👋 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.

@tsayao
Copy link
Collaborator Author

tsayao commented Jun 28, 2021

It's odd that glass Window.java defines enterModal but it seems to be never called. Modality is handled by calling setEnabled(false) on other windows.

I would refactor that some day :)

@tsayao tsayao changed the title WIP: 8269429: Linux: Only the last APPLICATION_MODAL window behaves corretly 8269429: Linux: Only the last APPLICATION_MODAL window behaves corretly Jun 29, 2021
@openjdk openjdk bot added the rfr Ready for review label Jun 29, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 29, 2021

Webrevs

@tsayao
Copy link
Collaborator Author

tsayao commented Jun 29, 2021

It's working as expected:

If there's two APPLICATION_MODAL (*1) and (*2) and a WINDOW_MODAL (*3) as a child of the primary stage the order of the windows on the screen will be (*2) (*1) (*3).

Will run the tests and post here.

@kevinrushforth
Copy link
Member

/reviewers 2

@openjdk
Copy link

openjdk bot commented Jun 29, 2021

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

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 tried the test program attached to the JBS bug, and I'm not seeing the problem. Are there any special steps I need to take to see the bug? I also left a question about WINDOW_MODAL vs APPLICATION_MODAL windows.

Regarding the {enter/exit/is}Modal methods in Window.java, it looks like they stubs that are never called. Modality is handled by the toolkit using (as you found out) only the setEnabled method.

Finally, you have a typo in the title of both this PR and the JBS bug: corretly --> correctly

@tsayao tsayao changed the title 8269429: Linux: Only the last APPLICATION_MODAL window behaves corretly 8269429: Linux: Only the last APPLICATION_MODAL window behaves correctly Jun 29, 2021
@tsayao
Copy link
Collaborator Author

tsayao commented Jun 29, 2021

I tried the test program attached to the JBS bug, and I'm not seeing the problem. Are there any special steps I need to take to see the bug? I also left a question about WINDOW_MODAL vs APPLICATION_MODAL windows.

Regarding the {enter/exit/is}Modal methods in Window.java, it looks like they stubs that are never called. Modality is handled by the toolkit using (as you found out) only the setEnabled method.

Finally, you have a typo in the title of both this PR and the JBS bug: corretly --> correctly

  • Added steps to reproduce on JBS. Basically the window manager will let you click the window in the back (primary stage in the example) and the APPLICATION_MODAL will go behind it, still blocking events;
  • Fixed the typo.

@tsayao
Copy link
Collaborator Author

tsayao commented Jun 29, 2021

image

@tsayao
Copy link
Collaborator Author

tsayao commented Jun 29, 2021

The first approach was to make the window not clickable or not "activable" when setEnabled(false) but I did not find a solution for that on Gdk or Gtk. Gtk has gtk_widget_set_sensitive but that did not work (probably only if using gtk signals). I did make other attempts with no success.

@pankaj-bansal
Copy link
Collaborator

I tried this on Ubuntu 20.04 and I see that the patch does solve the issue by bringing the Alert window above the primary window

@kevinrushforth kevinrushforth self-requested a review July 2, 2021 12:52
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.

This looks fine to me. I'll test it on a couple different systems. I left one comment inline.

@kevinrushforth
Copy link
Member

I verified that this fixes the problem, but it also changes the behavior of APPLICATION_MODAL dialogs on Linux to be "Always on Top" -- not just on top of other application windows (which is fine), but on top of all other windows an application might create, which is not desirable as a default behavior. This also makes it behave differently than Mac or Windows.

I think we should look for a solution that doesn't rely on forcing a dialog to be on top of other (no app) windows.

@tsayao
Copy link
Collaborator Author

tsayao commented Jul 20, 2021

Will try to find a solution to disable events on disabled windows setEnable(false)

@tsayao
Copy link
Collaborator Author

tsayao commented Aug 4, 2021

This is also fixed by #581.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfr Ready for review
Development

Successfully merging this pull request may close these issues.

3 participants