Skip to content

Improve Dialog Placement #5280

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.

Sign up for GitHub

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

Merged
matthiasblaesing merged 1 commit into from
Feb 6, 2023
Merged

Improve Dialog Placement #5280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ public void showDialog () {
// if a modal dialog is active use it as parent
// otherwise use the main window

NbPresenter presenter = null;
NbPresenter presenter;
if (descriptor instanceof DialogDescriptor) {
if (NbPresenter.currentModalDialog != null) {
if (NbPresenter.currentModalDialog.isLeaf ()) {
Expand All @@ -252,11 +252,8 @@ public void showDialog () {
presenter = new NbPresenter(descriptor, NbPresenter.currentModalDialog, true);
}
} else {
Frame f = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow()
instanceof Frame ?
(Frame) KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow()
: WindowManager.getDefault().getMainWindow();

Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
Frame f = w instanceof Frame ? (Frame) w : WindowManager.getDefault().getMainWindow();
Comment on lines +255 to +256
Copy link
Contributor Author

@errael errael Jan 13, 2023

Choose a reason for hiding this comment

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

This is a cosmetic change. It's both more clear and allows easier comparison to what's going on in the

if (descriptor instanceof DialogDescriptor) {

case

if (noParent) {
f = null;
}
Expand Down
12 changes: 12 additions & 0 deletions platform/core.windows/src/org/netbeans/core/windows/services/NbPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1591,6 +1591,18 @@ private void initBounds() {
*/
private Window findFocusedWindow() {
Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusedWindow();
if( w == null ) {
// PR#5280
LOG.fine( () -> "No focused window, find mainWindow" );
Comment on lines 1593 to +1596
Copy link
Contributor Author

@errael errael Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If there is no focused window, then try the mainWindow before doing the backup which centers on the JDK's default screen.

for( Frame f01 : Frame.getFrames() ) {
Copy link
Member

@sdedic sdedic Jan 19, 2023

Choose a reason for hiding this comment

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

Nitpick: call findMainWindow ?

Copy link
Contributor Author

@errael errael Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

call findMainWindow ?

@sdedic I'd like to do that; can you give me some guidance? Need to find a non-API file in platform that's OK to access from both core.windows and openide.util.ui where it's appropriate to declare findMainWindow as a public method. Such a file must exist, any pointers?

Copy link
Member

@sdedic sdedic Jan 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah ;) my bad, I overlooked the module boundary. No suggestion that 'feels' right then. An option would be to add a API/SPI method on e.g. DialogDisplayer.

Related to existing 'nearby' code: this method uses KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusedWindow() while getCurrentGraphicsConfiguration() (in diff below) uses KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner() then getWindowAncestor(). Deliberate difference or inconsistency ?

Copy link
Contributor Author

@errael errael Jan 21, 2023

Choose a reason for hiding this comment

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

I've wondered about that myself; are the two equivalent?

I'm copying your comment to #4887 which are notes for a follow on PR addressing some stuff in Utilities. The opening comment of this PR mentions issues for the next PR.

if( "NbMainWindow".equals(f01.getName())) { //NOI18N
if(f01.getWidth() != 0 || f01.getHeight() != 0) {
w = f01;
}
break;
}
}
}
while( null != w && !w.isShowing() ) {
w = w.getOwner();
}
Expand Down
31 changes: 24 additions & 7 deletions platform/openide.util.ui/src/org/openide/util/Utilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,11 +1157,9 @@ private static GraphicsConfiguration getCurrentGraphicsConfiguration() {
return w.getGraphicsConfiguration();
} else {
//#217737 - try to find the main window which could be placed in secondary screen
for( Frame f : Frame.getFrames() ) {
if( "NbMainWindow".equals(f.getName())) { //NOI18N
return f.getGraphicsConfiguration();
}
}
Frame f = findMainWindow();
if(f != null)
return f.getGraphicsConfiguration();
}
}

Expand Down Expand Up @@ -1325,6 +1323,25 @@ private static Rectangle findCenterBounds(GraphicsConfiguration gconf, Dimension
);
}

/**
* Find the main NetBeans window; must have width or height.
* This is used locally to avoid dependency issues.
* @return NetBeans' main window
*/
private static Frame findMainWindow()
{
Frame f = null;
for( Frame f01 : Frame.getFrames() ) {
if( "NbMainWindow".equals(f01.getName())) { //NOI18N
if(f01.getWidth() != 0 || f01.getHeight() != 0) {
f = f01;
}
break;
}
}
return f;
}

/**
* This is for use in situations where a standard swing API,
* such as {@linkplain JOptionPane.show*} or {@linkplain JFileChooser.show*},
Expand All @@ -1345,8 +1362,8 @@ public static Component findDialogParent() {
parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
}
if (parent == null) {
Frame[] f = Frame.getFrames();
parent = f.length == 0 ? null : f[f.length - 1];
// PR#5280
parent = findMainWindow();
Copy link
Contributor Author

@errael errael Jan 13, 2023

Choose a reason for hiding this comment

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

This is a change suggested by @matthiasblaesing in #4739 (comment). The change was deferred pending concrete examples. There is an example detailed in the opening comment for this PR.

Copy link
Member

@neilcsmith-net neilcsmith-net Jan 18, 2023

Choose a reason for hiding this comment

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

@errael I would possibly fall back to the original behaviour if the search for the main window returns null. Ideally it wouldn't use the name to find the right window, but that might be the easiest way to achieve. I have no idea how many people (if any!) use FileChoooserBuilder outside of the standard window system, but this will change the behaviour of those cases.

Copy link
Contributor Author

@errael errael Jan 18, 2023

Choose a reason for hiding this comment

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

@errael I would possibly fall back to the original behaviour if the search for the main window returns null.

The original behavior is severely flawed. Take a look in the comment that opened this PR, under notes on Utilities.findDialogParent. And from the javadoc for Frame.getFrames()

Warning: this method may return system created frames, such as a shared, hidden frame which 
is used by Swing. Applications should not assume the existence of these frames, nor should an 
application assume anything about these frames such as component positions, LayoutManagers 
or serialization.

Ideally it wouldn't use the name to find the right window, but that might be the easiest way to achieve.

Notice that this technique is already in use in this file. May have to do with dependencies. This PR encapsulates its use in a method so at least it's not buried.

I have no idea how many people (if any!) use FileChoooserBuilder outside of the standard window system, but this will change the behaviour of those cases.

If someone explicitly wants random dialog placement there are probably more interesting ways. I believe this case rarely comes up AFAIK, I've just been lucky.

A null owner is probably more consistent (better?).

If this is a strong concern for FileChooserBuilder, then suggest using a private version of findDialogParent() with the old code rather than propagate buggy behavior in a new-ish public API.

(Aside: I've wondered about use of non standard window system; are there any known examples in "production"? )

Copy link
Member

@neilcsmith-net neilcsmith-net Jan 18, 2023

Choose a reason for hiding this comment

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

@errael yes, I read the doc. Let's stick with what you have and see if any complaints come out of the woodwork!

}
return parent;
}
Expand Down