Dialog parent should not be null #4739
Conversation
mbien
added
Java
|
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. |
mbien
removed
the
Java
I have an example of that in a plugin, in something in Options I do Does the right thing. In particular, JOptionPane creates a modal dialog, end of story. |
|
Don't we have a lot of things that bypass NetBeans' APIs! 😬 I wonder if it's worth the effort of rewriting them? Other than that, |
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 I don't trust |
|
I noticed, that at least in some cases the class, that opens the dialog is itself an instance of |
Yes, that would be best. This PR is about automatically fixing obvious bugs. |
|
I copied I'm wondering if, for the last parent == null using MainWindow would be better than the last entry in the I started to put |
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 ;) |
|
I'm just taking another look at But now I notice the use of Null is just not the right choice in a multiscreen environment. I'm wondering about, in |
Using 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. |
Yeah, I need to run it again anyway, I can turn off |
errael
changed the title
|
@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. |
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. |
@neilcsmith-net, Oh, wait. I may have misunderstood. Is your comment about |
|
@errael |
|
@errael incidentally, I think moving it into |
|
Took me a while to realize that #4714 was the same Utilities.java that
Right, in this version of the PR I had to add a dependendency on |
In a multi-screen setup, using null parent often puts the dialog
on the wrong screen.
Utilities.findDialogParent() copied from FileChooserBuilder.
Manually change FileChoolerBuidler to use Utilities.
Rest of the changes done with jackpot script:
<?
import org.openide.util.Utilities;
?>
"chooser dialog null":
$jfc.showOpenDialog(null) :: $jfc instanceof javax.swing.JFileChooser
=> $jfc.showOpenDialog(Utilities.findDialogParent())
;;
"chooser dialog null":
$jfc.showSaveDialog(null) :: $jfc instanceof javax.swing.JFileChooser
=> $jfc.showSaveDialog(Utilities.findDialogParent())
;;
"JOptionPane-2a null parent":
$jop.showConfirmDialog($parent, $rest$)
:: isNullLiteral($parent)
&& $jop instanceof javax.swing.JOptionPane
=> $jop.showConfirmDialog(Utilities.findDialogParent(), $rest$)
;;
"JOptionPane-2b null parent":
$jop.showMessageDialog($parent, $rest$)
:: isNullLiteral($parent)
&& $jop instanceof javax.swing.JOptionPane
=> $jop.showMessageDialog(Utilities.findDialogParent(), $rest$)
;;
"JOptionPane-2c null parent":
$jop.showOptionDialog($parent, $rest$)
:: isNullLiteral($parent)
&& $jop instanceof javax.swing.JOptionPane
=> $jop.showOptionDialog(Utilities.findDialogParent(), $rest$)
;;
|
Wow, there are 49, as of a recent count, It's looking like the system won't be back from the shop till later in the week, so setup old win7 system... Tested Updated javafx/maven.htmlui was only module that didn't already have the dependency. |
|
Too bad using add module dependency doesn't preserve the file line endings. |
There was a problem hiding this comment.
code looks good to me. I do also like that the logic is now at a central place and everything uses this method.
However, I couldn't really see any difference. I tried to reproduce it with the git clone action for example. And both, NB 15 and this PR behaved correctly, It always opened on the "correct" screen (the screen i moved the main window to). But maybe I was looking at the wrong dialog.
Some other windows like main options are stubborn and open on screen 1 no matter where i move the main window.
If I close an editor tab via the drop down documents list while its modified. the discard dialog is still displaced in the corner of my screen.
But this can be all fixed in followup PRs if necessary, now that we have this method public.
Yeah, this only applies to a small number of dialogs. Swing dialogs that were using null parent. NB API dialogs are not affected.
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. |
|
@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. |
| } | ||
| if (parent == null) { | ||
| Frame[] f = Frame.getFrames(); | ||
| parent = f.length == 0 ? null : f[f.length - 1]; |
There was a problem hiding this comment.
The documentation of Frame.getFrames reads a bit strange. For NetBean I suggest to fall back to WindowManager.getMainWindow and if that indeed is not present, the return null.
There was a problem hiding this comment.
The documentation of
Frame.getFramesreads a bit strange. For NetBean I suggest to fall back toWindowManager.getMainWindowand if that indeed is not present, the return null.
Strange indeed. getMainWindow() does seem a better fallback. (and acc'd to doc creates a frame if none exists.)
I'll make that change, please review carefully since I won't be able to test it.
There was a problem hiding this comment.
This is code that already exists in the platform, in FileChooserBuilder. I'm not sure using getMainWindow can happen without causing dependency issues?
There was a problem hiding this comment.
I'm manually looking for main window specifically to avoid adding the dependency. Similar to whats seen around
netbeans/platform/openide.util.ui/src/org/openide/util/Utilities.java
Lines 1160 to 1164 in abb1a97
There was a problem hiding this comment.
So I'll wait for further comment. I've got a change ready that find the main window (without a dependency).
I will not push it, unless/until there's a consensus.
There was a problem hiding this comment.
Oh yeah, it looks like
// KFM is out of it, check out known frames
if (parent == null) {
Frame[] frames = Frame.getFrames();
for( Frame f : frames ) {
if( "NbMainWindow".equals(f.getName())) { //NOI18N
parent = f;
break;
}
}
if(parent == null && frames.length != 0)
parent = frames[frames.length - 1];
}
There was a problem hiding this comment.
I am strongly inclined to leave the code as a verbatim move of the code from FileChooserBuilder for now - that is used a lot across the IDE already. Let's use as is for the additional cases unless there's a proven issue to address.
While the documentation Frame::getFrames is a bit odd, I'm curious how often that fallback is ever hit here? Have you tried a breakpoint? If it's rare/never then changing it is pointless. If it's commonly hit, then special casing NbMainWindow may be a change in logic that actually ends up being less correct.
There was a problem hiding this comment.
There's no consensus to change. As is.
There was a problem hiding this comment.
Merging now. Let's test and revisit this if necessary during RC phase.
Be good to have a list of a few dialogs for users to test the corrected behaviour with.
@neilcsmith-net, taking a look at DialogDisplayerImpl, in one case it use I'm thinking now that for local choices, JOptionPane is actually a better choice (particularly with |
|
@errael yes, I agree, and there's probably more scope for consolidating this behaviour .. in the NB17 timeframe! |
@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:
|
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.