search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.62k stars 840 forks source link

Dialog parent should not be null #4739

Closed errael closed 1 year ago

errael commented 1 year ago

This PR affects multi screen systems when the NetBeans main window is not the default window.

There are several scattered uses of JFileChooser and JOptionPane which use null for parent, so these dialogs go to the default screen (screen 0). If a dialog parent is not specified, generally it's preferred to go to the same screen as the MainWindow. This PR does that. The PR was done with 3 jackpot scripts, with some manual hints to add a core windows dependency when needed.

It's initially a draft PR. Look at the description for each of the 3 commits to see the jackpot scripts. I'll squish the commits later; now there's an opportunity to see the changes for each declarative refactoring script.

Note, usually the window that brings up a dialog should be the parent. But if that wasn't done, this PR at least brings up the dialog on the window that probably has the user's attention.

mbien commented 1 year ago

wondering what happens if a dialog opens a dialog. Would this risk to open the new dialog behind the other since the parent is the main window? I think with null parent it would open on top of everything what is already opened.

errael commented 1 year ago

wondering what happens if a dialog opens a dialog

I have an example of that in a plugin, in something in Options I do

JOptionPane.showMessageDialog(
        ViManager.getFactory().getMainWindow(),

Does the right thing. In particular, JOptionPane creates a modal dialog, end of story.

neilcsmith-net commented 1 year ago

Don't we have a lot of things that bypass NetBeans' APIs! :grimacing: I wonder if it's worth the effort of rewriting them? Other than that,DialogDisplayerImpl and FileChooserBuilder have slightly different approaches, which makes me wonder if use of KeyboardFocusManager might be better than getMainWindow()? eg. https://github.com/apache/netbeans/blob/master/platform/openide.filesystems.nb/src/org/openide/filesystems/FileChooserBuilder.java#L296

errael commented 1 year ago

bypass NetBeans' APIs! grimacing I wonder if it's worth the effort of rewriting them?

The current situation is a hassle, it happens enough to be anoying, but not enough to be the first thing I check (that's slowly changing).

I thought about saying something like "this could be viewed as an interim step, fixing bugs, until some kind of rewrite". The last sentence of the initial comment does suggest that things weren't done right, and implies that it should be handled better. I saw FileChooserBuilder, that method findDialogParent could be put somewhere public, maybe DialogDisplayer, and I could rerun jackpot with that. But then there's a testing problem, I don't know how to force most of those dialogs. But I believe that would be better than the current state, and maybe not as reliable as this current PR. I'm suspect about findDialogParent picking the last frame in the list.

I don't trust KeyboardFocusManager.getActiveWindow. I think it returns null at inconvenient times, see https://github.com/apache/netbeans/pull/4714#issue-1393511869 third paragraph; I've been thinking about a patch to DialogDisplayerImpl that handles a null return from KFM.getActiveWindow and makes a choice. I can't imagine where else random/occasional incorrect placement of the dialogs could be coming from.

matthiasblaesing commented 1 year ago

I noticed, that at least in some cases the class, that opens the dialog is itself an instance of java.awt.Component. In that case it could be passed to JFileChooser#show*Dialog and Swing would be responsible to find the right JFrame which should be the parent of the dialog. That relieves the need to fiddle with the main window or the keyboard focus or whatever and makes the code easier to follow. This should apply to TemplatesPanel.java, LoadGeneratorCustomizer.java, LocationCustomizer.java and SnapshotCustomizer.java.

errael commented 1 year ago

the class that opens the dialog is itself an instance of java.awt.Component. In that case it could be passed to JFileChooser#show*Dialog

Yes, that would be best. This PR is about automatically fixing obvious bugs.

errael commented 1 year ago

I copied FileChooserBuilder.findDialogParent() into a standalone app. In practice it has much better characteristics than simply using the main window (at least in my tests using JOptionPane(findDialogParent(),...) from a dialog).

I'm wondering if, for the last parent == null using MainWindow would be better than the last entry in the Frame.getFrames() array.

I started to put findDialgParent as a static method into DialogDisplayer; then ApiChange came to mind. Before I make any changes, I'd like some comments on where to put it. Where to put it is more of an issue, effort wise, that what's in it.

mbien commented 1 year ago

the class that opens the dialog is itself an instance of java.awt.Component. In that case it could be passed to JFileChooser#show*Dialog

Yes, that would be best. This PR is about automatically fixing obvious bugs.

I mean this inspection affects 30 files or so. I think a second manual pass would be realistic to fix those occurrences which apply.

There is also the inClass(String... classes) condition which could help here if you want to automate that too (I haven't used that one yet myself, it doesn't work for interfaces that I know), however I believe doing this manually is going to be faster (since we already know the occurences) than scanning all projects again (which can take a bit).

If you decide to do that manually, maybe also consider reducing the whitespace changes in the imports. Since they add empty lines in between every top level package change - this only makes sense if the imports are actually sorted ;)

errael commented 1 year ago

I'm just taking another look at DialogDisplayerImpl. I may have unfoundedly disparaged KeyboardFocusManager returns; I had thought that perhaps it was being called when the focus owner was not in the "same context as the calling thread" (not sure that's possible as a desktop app).

But now I notice the use of noParent in DialogDisplayerImpl which may get set if runDelayed is involved and gives a null parent.

Null is just not the right choice in a multiscreen environment. I'm wondering about, in showDialog, using MainWindow instead of null parent. Another possibility would be to use the "preferred screen" from #4714. I lean towards MainWindow. Which handles both noParent and KeyboardFocusManager. Perhaps something a bit more complicated, determine the screen, eg MainWindow, but leave the parent null and move the dialog to the center of the chosen screen, (if there are cases where you don't want the dialog to have a parent?). Comments?

errael commented 1 year ago

manual pass

Using findDialogParent, which is needed anyway since not all the classes in question meet the criteria, there's good odds that the Component in question is the focusOwner so things should work out. If it's not the focus owner, I guess it would still be safe to use it as the owner; the dialog in question is placed over that component; but I think focus owner would be better. Hmm, might need to check isVisible()?

I can probably be talked into it; but I'm uncomfortable making manual changes without testing and I'm not sure I'll be able to navigate to all the dialogs.

errael commented 1 year ago

maybe also consider reducing the whitespace changes in the imports

Yeah, I need to run it again anyway, I can turn off Separate groups in editor > formatting.

errael commented 1 year ago

@neilcsmith-net, put findDialogParent in DialogDisplayer as static method. Instance method didn't make sense, it's not part of a NetBeans dialog.

Upgraded PR from draft. ApiChanges updated.

neilcsmith-net commented 1 year ago

@errael having been looking at changes in #4714 I wonder if that method should just go in Utilities?

errael commented 1 year ago

wonder if that method should just go in Utilities?

I'm OK with that. Wanted to pick something to get the ball rolling... When I was looking for a Util class to put it in, I think I missed this one. Was looking for something UI related.

System went into the shop yesterday, limited mail access. Not going to setup for dev on another system. Hope to get it back this weekend, but at least before the freeze.

errael commented 1 year ago

looking at changes in https://github.com/apache/netbeans/pull/4714 I wonder if that method

@neilcsmith-net, Oh, wait. I may have misunderstood. Is your comment about getPreferredScreen() or findDialogParent()?

neilcsmith-net commented 1 year ago

@errael findDialogParent(). Well, both I guess. Just looking at that other PR made me think it might be the best place for this method too.

neilcsmith-net commented 1 year ago

@errael incidentally, I think moving it into Utilities could enable FileChooserBuilder to use it too? I think the module dependency is already there.

errael commented 1 year ago

Took me a while to realize that #4714 was the same Utilities.java that findDialogParent() is moving to. (I miss my system)

FileChooserBuilder to use it too? I think the module dependency is already there.

Right, in this version of the PR I had to add a dependendency on Dialogs API to get FileChooserBuilder to use the new location findDialogParent.

errael commented 1 year ago

Wow, there are 49, as of a recent count, Utilities.java in the tree.

It's looking like the system won't be back from the shop till later in the week, so setup old win7 system... Tested Utilities.findDialogParent() from a plugin. Changed FileChooserBuilder manually. The rest of the changes are generated by Jackpot.

Updated apichanges.xml, don't do that much so another set of eyes...

javafx/maven.htmlui was only module that didn't already have the dependency.

errael commented 1 year ago

Too bad using add module dependency doesn't preserve the file line endings.

errael commented 1 year ago

looking at the wrong dialog

Yeah, this only applies to a small number of dialogs. Swing dialogs that were using null parent. NB API dialogs are not affected.

other windows like main options are stubborn and open on screen 1

Yeah, many have a memory.

I may instrument all the places a dialog is displayed looking for ones that use a null parent. Those are obviously in correct.

errael commented 1 year ago

@mbien, addressing some of the annoying behavior you describe would require a rethink of part of the dialog displaying and/or some fixup maybe.

Might be worth an issue/discussion about problematic behavior that should be addressed. I suspect null parent doesn't play a role in much of it.

errael commented 1 year ago

Other than that,DialogDisplayerImpl and FileChooserBuilder have slightly different approaches

@neilcsmith-net, taking a look at DialogDisplayerImpl, in one case it use KeyboardFocusManager.getActiveWindow() which is similar; but it also checks several more cases. And I can't figure out when/how AWTQuery is used.

I'm thinking now that for local choices, JOptionPane is actually a better choice (particularly with findDialogParent()). In my tests, when I click on a button, the JOptionPane dialog shows up right under the mouse cursor; very nice.

neilcsmith-net commented 1 year ago

@errael yes, I agree, and there's probably more scope for consolidating this behaviour .. in the NB17 timeframe!

errael commented 1 year ago

a list of a few dialogs for users to test the corrected behaviour with.

@neilcsmith-net, here's some

Note that previously these would have shown up in the middle of default screen (not necessarily the one with the main window). Now they should appear over the button that was activated.

For changes in files: