Improve Dialog Placement #5280

errael · 2023-01-11T23:07:14Z

When placing a dialog through NetBeans' API, if no focused component is found then use NbMainWindow. Change fallback for Utilities.findDialogParent to use NbMainWindow.

@matthiasblaesing , @neilcsmith-net , @mbien alerting since you made comments on previous PR.

This PR is a follow on to #4739. Wanted to open a bigger change earlier in the cycle, but... So this is a minimal change, hopefully more to come next cycle with detailed examination of cases from #4887.

There are situations where

null == KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusedWindow();

And also related methods such as getFocusOwner(), getActiveWindow() are null.

What happens is NbPresenter.findFocusedWindow() returns null and the logic in NbPresenter.initBounds() ends up doing Utilities.findCenterBounds(size) which does Utilities.getCurrentGraphicsConfiguration() and ends up on the default screen instead of a screen that NetBeans is on. The bigger change I've been thinking about addresses some issues with Utilities.getCurrentGraphicsConfiguration() and would put the dialog on the same screen as NetBeans is on, but even better is to have the dialog over the Window that NetBeans is in. This PR does that with a hack to NbPresenter.findFocusedWindow(); taken from Utilities.

In NbPresenter.initBounds(), can consider Utilities.findDialogParent() instead of the local findFocusedWindow(), and then make sure that whatever's returned is a window. In any event, with this change the else clause is unlikely to ever be executed; just as well, notice the comment for the else clause:

//just center the dialog on the screen and let's hope it'll be
//the correct one in multi-monitor setup

notes on Utilities.findDialogParent

There was some discussion in the dialog parent PR #4739 (comment) from last release about the Utilities.findDialogParent() fallback. Investigation for this PR showed a situation where a fallback is needed, and the current fallback is not good.

Frame.getFrames() comes from a Vector<WeakReference>; doing garbage collection shortens the array. Here's an example I saw, the fallback

parent = f.length == 0 ? null : f[f.length - 1];

selects the last item and is choosing something ripe for garbage collection. Notice in this case the 2nd entry is the main window.

f	Frame[]	#16829(length=6)	#16829(length=6)	
[0]	Frame	#16831	java.awt.Frame[frame0,2640,390,480x300,invalid,hidden,
[1]	JFrame	#16832	javax.swing.JFrame[NbMainWindow,2566,544,1274x505,inva
[2]	Popup$DefaultFrame	#16833	javax.swing.Popup$DefaultFrame[frame3,
[3]	Popup$DefaultFrame	#16778	javax.swing.Popup$DefaultFrame[frame2,
[4]	Popup$DefaultFrame	#16834	javax.swing.Popup$DefaultFrame[frame4,
[5]	Popup$DefaultFrame	#16835	javax.swing.Popup$DefaultFrame[frame5,

These entries are like:

javax.swing.Popup$DefaultFrame[frame5,0,32,0x0,invalid,hidden

This PR makes the change to use the main window as fallback. This was previously proposed, but was deferred pending clarification on whether a fallback was needed at all.

if no focused component is found then use NbMainWindow. Change fallback for Utilities.findDialogParent to use NbMainWindow.

errael · 2023-01-11T23:09:20Z

Doing the same steps, the problem (stemming from no focused component or window) is intermittent; I'm guessing timing. Happens more frequently when run under the debugger.

The problem may be more likely to arise when using the jVi plugin. When the user does a vi colon command, a one line window, JDialog, is brought up for the command entry. Some jVi commands invoke actions which bring up a dialog. The one line window is disappearing while the NetBeans dialog is coming up.

mbien · 2023-01-13T10:20:12Z

not sure if I will have time to take a deeper look at this before feature freeze. But I found a situation where the direct parent is not the optimal answer:

the editor tab dropdown can close tabs. If the tab is unsaved a dialog will appear. This dialog's parent would be the popup which causes it open on the upper right area of the main window.

This doesn't look horrible but its something to check. Simple yes/no dialogs should probably always open centered on the main window which might be the better default.

errael · 2023-01-13T16:50:11Z

platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java

+                    Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
+                    Frame f = w instanceof Frame ? (Frame) w : WindowManager.getDefault().getMainWindow();


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

errael · 2023-01-13T16:59:44Z

to take a deeper look at this

This is actually two tiny changes to handle corner cases where the "normal" situation doesn't apply. Adding some review comments to to clarify.

errael · 2023-01-13T17:03:25Z

platform/openide.util.ui/src/org/openide/util/Utilities.java

-            Frame[] f = Frame.getFrames();
-            parent = f.length == 0 ? null : f[f.length - 1];
+            // PR#5280
+            parent = findMainWindow();


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.

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

@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"? )

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

errael · 2023-01-13T17:05:27Z

platform/core.windows/src/org/netbeans/core/windows/services/NbPresenter.java

        Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusedWindow();
+        if( w == null ) {
+            // PR#5280
+            LOG.fine( () -> "No focused window, find mainWindow" );


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

errael · 2023-01-13T18:00:29Z

a situation where the direct parent is not the optimal answer:

the editor tab dropdown can close tabs. If the tab is unsaved a dialog will appear. This dialog's parent would be the popup which causes it open on the upper right area of the main window.

Is this intermittent for you? I can't reproduce what you're seeing. I thought it might be because I use "focus follows mouse" but disabling that didn't change it. The Question-File xxx is modified. Save? dialog always shows up over the main window. I tried detaching the editor, the dialog shows up over the detached editor. I'm on current linux, with

XDG_CURRENT_DESKTOP=pop:GNOME

Could be a OS/window manager issue.

The close sequence starts with the following and notice the comment about focusing the top component before continuing.

netbeans/platform/core.windows/src/org/netbeans/core/windows/actions/CloseWindowAction.java

Lines 74 to 89 in cac7e65

    
           public void actionPerformed(java.awt.event.ActionEvent ev) { 
        
               TopComponent topC = tc; 
        
               if (topC == null) { 
        
                   // the updating instance will get the TC to close from winsys 
        
                   topC = TopComponent.getRegistry().getActivated(); 
        
               } 
        
               if(topC != null) { 
        
                   //132852 - if the close action is canceled then the input focus may be lost 
        
                   //so let's make sure the window get input focus first 
        
                   topC.requestFocusInWindow(); 
        
                   final TopComponent toClose = topC; 
        
                   SwingUtilities.invokeLater( new Runnable() { 
        
                       public void run() { 
        
                           ActionUtils.closeWindow(toClose); 
        
                       } 
        
                   });

The action does end up in NbPresenter.findFocusedWindow(), where I've got a change; in the cases I've looked at the code I've added doesn't come into play.

errael · 2023-01-13T19:43:58Z

a situation where the direct parent is not the optimal answer:
the editor tab dropdown can close tabs. If the tab is unsaved a dialog will appear. This dialog's parent would be the popup which causes it open on the upper right area of the main window.

Is this intermittent for you? I can't reproduce what you're seeing.
...

Could be a OS/window manager issue.

@mbien, If you have a chance to check what is returned by

Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusedWindow();

at the first line of

org.netbeans.core.windows.services.findFocusedWindow()

And it's something that doesn't make sense, something like

javax.swing.Popup

Then I could try to address it.

matthiasblaesing · 2023-01-16T21:05:09Z

Choosing the close icon in the editor tabs drop down gives this strange placement:

The windows are neither in the right position, nor the in the center of the right screen. The screen border is where the background image begins to repeat.

errael · 2023-01-17T02:01:01Z

@matthiasblaesing Thanks for the info. Could you and @mbien provide OS/window manager info.

I've been unable to reproduce this, see the last couple of messages I've posted. In particular, #5280 (comment) indicates some information I'd like to see (for example, collected with the debugger) that might provide some insight into what's going on.

This behavior has been there for a long while (assuming it's the same thing Michael brought up) and doesn't have anything to do with this PR.

matthiasblaesing

My observation in the previous comment is reproducible, but it is also not caused by this change. The code in org.netbeans.core.windows.services.DialogDisplayerImpl is just not optimal, leading to the strange placement of the dialog. A quick test inserting:

presenter.setLocationRelativeTo(presenter.getOwner());

before the setVisible(true) call, gives me sane centering over the main window. Not sure if that is desirable, but might be an option in the future to consider.

I checked what I got for the Window w / Frame f in line 255/256 and in both cases (editor pane in main window and editor pane in a separater window) I got the correect Window/JFrame objects I would have expected, not some popup.

errael · 2023-01-17T23:36:02Z

My observation in the previous comment is reproducible

@matthiasblaesing Wish I could reproduce it. Here's what the code does

Note that the placement code in NbPresenter does not use the owner,
it uses the focused window (if found).

DialogDisplayerImpl.AWTQuery.showDialog

presenter = new NbPresenter(desc, owner, modalTrue)
- initBounds()
  - // assuming focused window
  - setLocationRelativeTo( findFocusedWindow() )
  - XXX code to change location if screen boundary overlaps dialog
customizeDlg(presenter) // <<< does lookup, not found in my tests
// matthias: presenter.setLocationRelativeTo(presenter.getOwner())

XXX above could do what's being seen

Here's what I tried, but can't reproduce

Under the debugger

start up clean udir/cache dummy nbm project (no plugins)
open existing ant java library project used for testing, several tiny files
open a few java files
Add a character to a file
Context menu on editor tab, select close

Observe save? dialog centered on main window

sdedic · 2023-01-19T07:51:40Z

platform/core.windows/src/org/netbeans/core/windows/services/NbPresenter.java

+        if( w == null ) {
+            // PR#5280
+            LOG.fine( () -> "No focused window, find mainWindow" );
+            for( Frame f01 : Frame.getFrames() ) {


Nitpick: call findMainWindow ?

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?

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 ?

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.

errael · 2023-01-19T17:44:26Z

@mbien @matthiasblaesing

In #5280 (comment) I said that the code in NbPresenter.initBounds() code to change location if screen boundary overlaps dialog, could be responsible for the behavior. But it looks like there are other candidates for the problem.

I'm taking a look at Window.setLocationRelativeTo(), one comment in the javadoc is

Note: If the lower edge of the window is out of the screen, then the window is placed to the 
side of the Component that is closest to the center of the screen. So if the component is on 
the right part of the screen, the window is placed to its left, and vice versa.

which seems to echo the noted behavior.

In any event, knowing the arg in setLocationRelativeTo( findFocusedWindow() ) in NbPresenter.initBounds() is important for getting to the bottom of what's going on. Also seeing getLocation() for the various components and the precise code path and intermediate values would be useful.

platform/openide.util.ui/src/org/openide/util/Utilities.java

eirikbakke

PR looks fine to me, though I don't have deep knowledge of what's going on here. But it sounds like it's been extensively tested and discussed. Thanks for this PR!

errael · 2023-02-06T18:34:23Z

There are two approvals, and someone wearing a release manager's hat said "Let's stick with what you have and see if any complaints come out of the woodwork!". I'll count that as 2+ approvals, no negatives.

@mbien @sdedic (or @neilcsmith-net wearing a different hat) any further comments?

Can this get merged?

BTW, I think this PR will take care of several items I listed in #4887, I'll go through them all and see if they meet the criteria covered by this PR. The big revelation for me is that the layout decisions happen mostly in NbPresenter (with an occasional assist from openide.util.Utilities). I'll patch NB-17 release with this PR and keep my eyes open for follow on issues (I haven't been using NB lately).

errael · 2023-02-06T18:37:19Z

@matthiasblaesing @mbien I'm willing to consider the problem (not related to this PR) both of you can reproduce (and I can't).

I think info gathered from NbPresenter as mentioned in #5280 (comment) might be important in understanding what's going on.

There was some thought that it might be an issue with DialogDisplayer as mentioned in #5280 (review) but my guess is not.

And #5280 (comment) might help in understanding what's going on.

If you want/can open up a bug with some info (especially reproducability) I'd like to know about it.

matthiasblaesing · 2023-02-06T20:14:59Z

Lets get this in.

When placing a dialog through NetBeans' API,

9164e39

if no focused component is found then use NbMainWindow. Change fallback for Utilities.findDialogParent to use NbMainWindow.

errael force-pushed the ImproveDialogPlacement branch from e33f093 to 9164e39 Compare January 11, 2023 23:13

lkishalmi requested review from eirikbakke, matthiasblaesing, mbien and neilcsmith-net January 13, 2023 03:10

lkishalmi added this to the NB17 milestone Jan 13, 2023

lkishalmi added the Platform [ci] enable platform tests (platform/*) label Jan 13, 2023

errael commented Jan 13, 2023

View reviewed changes

matthiasblaesing approved these changes Jan 17, 2023

View reviewed changes

neilcsmith-net modified the milestones: NB17, NB18 Jan 18, 2023

sdedic reviewed Jan 19, 2023

View reviewed changes

eirikbakke reviewed Jan 20, 2023

View reviewed changes

platform/openide.util.ui/src/org/openide/util/Utilities.java Show resolved Hide resolved

eirikbakke approved these changes Jan 21, 2023

View reviewed changes

matthiasblaesing merged commit 23882ee into apache:master Feb 6, 2023

errael deleted the ImproveDialogPlacement branch February 6, 2023 21:09

neilcsmith-net mentioned this pull request May 24, 2023

Modal dialog with wrong parent can lock up the application #5987

Closed

errael mentioned this pull request May 30, 2023

#5987: Use the currently active dialog as the presenter parent #5989

Merged

errael mentioned this pull request Aug 3, 2023

Fix issue (GH6290) in new DialogDisplayerImpl behaviour #6294

Merged

		Window w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
		Frame f = w instanceof Frame ? (Frame) w : WindowManager.getDefault().getMainWindow();

Improve Dialog Placement #5280

Improve Dialog Placement #5280

Uh oh!

Conversation

errael commented Jan 11, 2023

notes on Utilities.findDialogParent

Uh oh!

errael commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbien commented Jan 13, 2023

Uh oh!

errael Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

errael commented Jan 13, 2023

Uh oh!

errael Jan 13, 2023

Choose a reason for hiding this comment

Uh oh!

neilcsmith-net Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

errael Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

neilcsmith-net Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

errael Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errael commented Jan 13, 2023

Uh oh!

errael commented Jan 13, 2023

Uh oh!

matthiasblaesing commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

errael commented Jan 17, 2023

Uh oh!

matthiasblaesing left a comment

Choose a reason for hiding this comment

Uh oh!

errael commented Jan 17, 2023

Uh oh!

sdedic Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

errael Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdedic Jan 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errael Jan 21, 2023

Choose a reason for hiding this comment

Uh oh!

errael commented Jan 19, 2023

Uh oh!

Uh oh!

eirikbakke left a comment

Choose a reason for hiding this comment

Uh oh!

errael commented Feb 6, 2023

Uh oh!

errael commented Feb 6, 2023

Uh oh!

matthiasblaesing commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

errael commented Jan 11, 2023 •

edited

Loading

errael Jan 13, 2023 •

edited

Loading

matthiasblaesing commented Jan 16, 2023 •

edited

Loading

errael Jan 19, 2023 •

edited

Loading

sdedic Jan 21, 2023 •

edited

Loading