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

[rkeen-siemens]

As described in #5987, DialogDisplayer.notify won't use the active window as the parent if it is a dialog that wasn't originally shown via NbPresenter. This causes issues in some cases due to JDK-8306001 because the displayed dialog does not have the previously active modal dialog as its parent.

The solution here is to use the Dialog overload when creating the NbPresenter if the currently active window is a Dialog instead of falling back to the main window.

[neilcsmith-net]

cc/ @errael - can't add you as a reviewer, but good if you can have a look at this one.

[neilcsmith-net]

The change makes sense, but shouldn't the same logic be used for DialogDescriptor above? Could the search for the parent happen before the check if a DialogDescriptor?

We've also got slightly different logic in createDialog(), not to mention in utilities. Feels like some of this could be consolidated?

The logic for isLeaf() feels wrong too, and potentially prone to the same problem? If any modal dialog is active, and another modal dialog is being opened, then shouldn't it always have the existing modal as a parent?

[rkeen-siemens]

shouldn't the same logic be used for DialogDescriptor above?

It certainly could. I was constraining my fix to the area that was causing the issue in my case.

We've also got slightly different logic in createDialog(), not to mention in utilities. Feels like some of this could be consolidated?

I can look at consolidating the logic.

The logic for isLeaf() feels wrong too, and potentially prone to the same problem? If any modal dialog is active, and another modal dialog is being opened, then shouldn't it always have the existing modal as a parent?

I am not really familiar with the need for the NbPresenter.currentModalDialog field. It looks like ProfilerDialogs is the only place that sets leaf to anything other than the default, although it was originally put in place to address Bugzilla #53525. I'm inclined to remove the currentModalDialog field and just keep the existing instanceof check on the active window.

We could also simplify the Frame vs. Window logic by adding constructor overloads that take Window since JDialog now has an equivalent overload (as of Java 6) which I assume was not available 20 years ago when the original NbPresenter constructors were created.

I'll add some changes in additional commits (which I can squash before merging if desired).

[errael]

We could also simplify the Frame vs. Window logic

Another possible simplification:
Could the two branches of

if (descriptor instanceof DialogDescriptor) {
} else {
}

be collapsed into a single branch by prefacing the unified branch with something like

createPresenter = descriptor instanceof DialogDescriptor
    ? (bunch_of_args) -> { return new NbDialog(needed_args); }
    : (bunch_of_args) -> { return new NbPresenter(needed_args); }

and using createPresenter.create(args) as needed?

[errael]

showDialog() is so clean and pleasing to view.

Only reviewed what I've seen before. Didn't consider or look at the removal of currentModalDialog in NbPresenter or changes for isLeaf().

[errael]

findMainWindow() is private, probably shouldn't be referenced in javadoc. Could just say the main window is the default if nothing better is found.

[rkeen-siemens]

I can remove this reference. It may also make sense to expose it publicly and call it from ViewHierarchy.getMainWindow(). However, that brings up another thought that if there is an alternative implementation registered for WindowManager that implements getMainWindow() differently, using this Utilities version in DialogDisplayerImpl could cause a regression. As such, it may be better to have a default provider or have this method return Optional<Component>. That way DialogDisplayerImpl can provide the appropriate getMainWindow() implementation as a fallback.

public static Component findDialogParent() {
    return findDialogParent(null, Utilities::findMainWindow);
}

public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
    Component parent = suggestedParent;
    if (parent == null) {
        parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
    }
    Window active = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
    if (parent == null) {
        parent = active;
    } else if (active instanceof Dialog && ((Dialog) active).isModal()) {
        Window suggested = parent instanceof Window ? (Window) parent : SwingUtilities.windowForComponent(parent);
        if (suggested != active) {
            return active;
        }
    }
    if (parent == null) {
        return fallback.get();
    }
    return parent;
}

Then DialogDisplayerImpl can provide the fallback to the main window via Utilities.findDialogParent(null, WindowManager.getDefault()::getMainWindow).

[errael]

if there is an alternative implementation registered for WindowManager that implements getMainWindow() differently, using this Utilities version in DialogDisplayerImpl could cause a regression

I don't see that as an alternative implementation; the difference between get and find is significant. findMainWindow() under no circumstance creates an NbMainWindow if one does not exist; it's a hack for some low level parenting decisions that I believe should remain hidden. This method can be used during startup, before there is a main window or even a WindowManager (I think).

[errael]

it may be better to have a default provider or have this method return Optional<Component>. That way DialogDisplayerImpl can provide the appropriate getMainWindow() implementation as a fallback.

I don't see how fallback adds much. At the call site something like

    if((parent = Utilities.findDialogParent(null)) == null)
        parent = fallback;

is equivalent (unless you believe that there is both a main window not named NbMainWindow and that there is frame with the name NbMainWindow which is not the main window). I wonder if this is the only spot where an alternative implementation of WindowManager might present an issue, and even if it is, is this the best way to deal with it? Seems like putting a lot of work into a method that isn't used anywhere and should be part of a broader strategy. That said, if the fallback goes in, I'd suggest handling fallback == null to find "NbMainWindow".

[rkeen-siemens]

That said, if the fallback goes in, I'd suggest handling fallback == null to find "NbMainWindow"

Doing that would silently ignore the fallback supplier. I'd rather have it throw in this case so you know you didn't specify a valid supplier or can use findDialogParent() instead.

[errael]

that would silently ignore the fallback supplier

Or be an indication that the default should be used. Are you saying that arguments should never be null, and should use Optional instead?

[errael]

I wonder if this is the only spot where an alternative implementation of WindowManager might present an issue, and even if it is, is this the best way to deal with it? Seems like putting a lot of work into a method that isn't used anywhere and should be part of a broader strategy.

noticed that in NbPresenter.java there's findFocusedWindow() which falls back to "NbMainWindow"

This should be considered as part of a general solution. I think the usage/meaning of findDialogParent(Component suggestedParent, Supplier<Component> fallback) in the context of alternate window managers needs to be addressed before this becomes a public API.

Possible issue, Utilities can not depend on core.windows as discussed in #5280 (comment); passing in a method from a module that this module does not depend on would probably get a runtime error.

[errael]

Don't see Component findDialogParent(Component suggestedParent) used anywhere in the PR. You're suggesting adding it to the API? I see how it's convenient to check if the suggestedParent is unsuitable because there's a modal dialog active.

[rkeen-siemens]

You're suggesting adding it to the API?

Yes, would be useful when using a JOptionPane (or similar) where you want the new dialog to be positioned relative to a specific component (suggestedParent) rather than just the containing window. If NotifyDescriptor had the option to specify a suggested parent, DialogDisplayerImpl would use this overload.

[errael]

Seems like this is for any dialog, not "dialog displayer". Maybe I'm just confusing myself.

Is it useful to mention that a suggestedParent is unsuitable if there's a modal dialog active and the suggestParent is not it.

[rkeen-siemens]

Sure, I was just using the same documentation in the existing findDialogParent() method. How about this (based on the added fallback supplier I suggested earlier)?

/**
 * Finds an appropriate component to use for a modal dialog's parent. Similar to {@link #findDialogParent()}
 * with the ability to specify a suggested parent component and fallback.
 *
 * @param suggestedParent
 *            the component to return if non-null and valid
 * @param fallback
 *            a supplier for the parent if no other suitable candidate exists
 * @return the suggested parent if there is either no active modal dialog or it is contained
 *         within that dialog, otherwise the active modal dialog or the fallback.
 */

[errael]

I guess isModaldialogOpen is here, even though it's only used from one spot, to keep this stuff together/centralized. Remember to document any API changes; this one in platform/openide.util.ui/apichanges.xml.

[rkeen-siemens]

Yes, this seemed like a general purpose method so adding it to Utilities seemed reasonable.

Remember to document any API changes

Will do; thanks for the reminder.

[errael]

Yes, this seemed like a general purpose method so adding it to Utilities seemed reasonable.

I'm not an official reviewer and decisions about what belongs in the API are above my pay grade, but I'm always willing (too willing some might say) to offer an opinion. I believe that most stuff in API is about NetBeans; I'm not sure that convenience methods that aren't NB related belong in the public API, for example isModalDialogOpen().

[rkeen-siemens]

It doesn't seem like a stretch to have it here, but I can make it private to NodeOperationImpl if that's more palatable.

[errael]

@neilcsmith-net Need feedback on API appropriateness.

[neilcsmith-net]

IMO it seems a useful method. Not sure if it needs to iterate up the window ownership tree though?

[errael]

I'll add some changes in additional commits (which I can squash before merging if desired)

Typically, from what I've seen, PRs are squashed into a single commit unless there's a reason not to. I've seen warnings about not doing on github, rather do it locally on you machine. @neilcsmith-net can clarify if needed.

[errael]

Just noticed that in NbPresenter.java there's findFocusedWindow() which falls back to "NbMainWindow"

[mbien]

some core.windows tests became very unreliable recently which is tracked in #5955. This is not related to this PR here, but quite annoying since it requires so many restarts. (usually causes tests to fail before they even start BeforeFirstTest: org.netbeans.core.windows.awt.ValidateLayerMenuTest)

However, the tests which fail right now do seem to be related.

testIfLasterWhenSplashShownThanWaitTillItFinished: org.netbeans.core.windows.services.NotifyLaterWithDialogDescriptorTest
testIfLasterWhenSplashShownThanWaitTillItFinished: org.netbeans.core.windows.services.NotifyLaterTest

[neilcsmith-net]

Definitely looks like those breaking tests need to be considered, and whether the behaviour should be changing in that situation.

I've been tied up with the release, and still catching up on other things - @sdedic might be a good person to review this, or could ping someone else to?

To clarify, my concern with isLeaf was mainly to do with always treating NotifyDescriptor as a leaf, even if modal. Quick look at this suggests this might ignore that now if there's a modal open? Is this now susceptible to the original BZ issue?

On squashing, yes this will need tidying up once ready, probably squashed into a single commit (but more if it makes sense) and force pushed into the PR branch before it can be merged.

[rkeen-siemens]

However, the tests which fail right now do seem to be related.

The NotifyLater* tests were failing because they were checking that the new dialog's parent was the shared owner frame which JDialog provides when constructing with a null Frame. Since the Window constructor bypasses that, the owner is null so I changed the assert. I believe the intent of the test is that the dialog will not pick up the splash screen as its parent and close when the splash screen closes. Having no owner in this scenario will achieve the same effect.

[rkeen-siemens]

One check is failing but doesn't appear related to this change:

[junit] Testcase: org.netbeans.core.windows.awt.ValidateLayerMenuTest:BeforeFirstTest: Caused an ERROR
[junit] Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.
[junit] junit.framework.AssertionFailedError: Forked Java VM exited abnormally. Please note the time in the report does not reflect the time until the VM exit.
[junit] at java.util.Vector.forEach(Vector.java:1277)
[junit] at java.util.Vector.forEach(Vector.java:1277)

I can run ant -Dtest-unit-sys-prop.ignore.random.failures=true -Dvanilla.javac.exists=true -f platform/core.windows test on my linux machine and all tests pass, so I'm assuming this is unrelated to my changes.

[mbien]

@rkeen-siemens yeah it is the above mentioned issue of the unreliable tests. Going to restart it a few times more to brute force through it. Would be good to know that everything is green before merging.

aaand its green. Took only 7 restarts this time :)

[rkeen-siemens]

I squashed all the commits into a single commit. Hopefully the checks won't need quite so many restarts this time.

[rkeen-siemens]

Now that NB 18 is released, I'm hoping someone has some time now to take a look at the updates here.

[neilcsmith-net]

Was meaning to cancel my request for changes rather than approve .. 🤷 Still, have had a quick look through ..

I think this is much better now. I'd potentially think another overload with findDialogParent(Supplier<Component> fallback) might be good if the most common usage is going to involve passing in null as first argument? And possibly better documentation of what a suggested parent being non-valid means. Just things to consider rather than requests for change.

Would still like to see another review from @sdedic , or @matthiasblaesing and @eirikbakke who reviewed #5280

[errael]

Here's a summary of my concerns/questions around findDialogParent, in it's current form, as a public API.

There's code scattered throughout NetBeans that does Utilities.findDialogParent(). I believe that the general consensus is that the WindowManager's existing MainWindow is the correct default, as @matthiasblaesing first mentions in #4739 (comment). Unfortunately, there's a dependency issue; also WindowManager.getMainWindow creates a main window if one doesn't exist which isn't appropriate for something in Utilities.

@sdedic discusses a solution in #5280 (comment)

I'm still interestd in hearing a response to #5989 (comment).

Also, in NbPresenter there's findFocusedWindow which has a hardcoded string NbMainWindow

The only use of the proposed findDialogParent is findDialogParent(null, WindowManager.getDefault()::getMainWindow) in DialogDisplayImpl; this feels like a bandaid/workaround for the inability of Utilities to find the main window. As a bandiad, should it become a public API? There is a workaound for DialogDisplayImpl, using this PR's previous version, something like

    if((parentComponent = Utilities.findDialogParent(null)) == null)
        parentComponent = WindowManager.getDefault()::getMainWindow);

What's the use case for providing a fallback? Other than working around a know deficiency with getting the main window?

There is a general problem with finding the default main window; if this problem were addressed, would it affect this new public API? And/or the need for it? Should the problem with finding the main window be fixed (or at least have a target solution) before there's a public API around this issue. Maybe they're orthoginal issues, it's not clear to me.

In general, this is a beautiful PR; but, IMHO, this new findDialolgParent public API seems mis-directed.

[errael]

Forgot to mention @neilcsmith-net 's #5280 (comment) about finding the "real" main window and not using a hardcoded string.

[rkeen-siemens]

this new findDialolgParent public API seems mis-directed

My primary goal in adding this was to allow for a dialog to be positioned relative to a component within the currently active window (assuming there isn't another modal window open) rather than to the whole window. This is similar to the way the parent component is used in JOptionPane (see JOptionPane.initDialog(JDialog, int, Component)).

If it is more palatable, I can remove the Supplier argument and just have it default to the main window which would connect it more closely to the platform rather than being a totally generic method.

[errael]

I'm confused. This statement seems innacurate.

adding this was to allow for a dialog to be positioned relative to a component within the
currently active window (assuming there isn't another modal window open) rather than to the
whole window.

If there's a "currently active window" (or a focused component), then the fallback is not used.

And I couldn't find JOptionPane.initDialog().

AFAICT, the fallback is only used when there is no focus owner and no active window, and no
dialog. I think this is a rare and unusual case; I'm not sure I know how to create it.

Maybe there's some miscommunication. The issues I raised, stated in the first line, are about the
findDialogParent in its current form, which means the fallback.

I want to understand the use-case for it; in particular, is it only to work around a NB bug?

[neilcsmith-net]

I think at this point, I'm inclined to the same reservations as @errael so probably removing the fallback supplier parameter for now. Adding (back) findDialogParent(Component suggestedParent) instead? And I think that must be used from DialogDisplayerImpl to account for the preferredParent parameter to createDialog(..)?

We probably should look at a better way to allow Utilities to find the main window in future.

[eirikbakke]

I will test this patch in my NetBeans Platform app and working IDE for a few days.

[rkeen-siemens]

Squashed the commits optimistically assuming no further changes will be requested, but I can still make updates if necessary.

[neilcsmith-net]

I've been mostly out of the NetBeans loop over the last couple of weeks freeing time up for the release. Shame this has missed further review.

I share some of @errael concerns on the API additions, but think it would be good to get the fix in 19. It'll miss merging before freeze, but can probably target 19-rc2.

[neilcsmith-net]

Is there a bug here? The old code doesn't use preferredParent if there's a modal dialog. Should the preferred parent be passed through to Utilities via findDialogParent ?

[rkeen-siemens]

Possibly, if there is an active modal dialog and it is not preferredParent. How about validating preferredParent?

Window w = preferredParent;
if (w != null) {
    // Ensure the preferred parent is valid
    Component p = Utilities.findDialogParent(w, () -> null);
    if (p != w) {
        w = null;
    }
}
if (w == null) {
    // Existing logic from 101 onward
}

[neilcsmith-net]

Or update and call through to findDialogParent() in this class -

private Window findDialogParent(Window preferred) {
        Component parentComponent = Utilities.findDialogParent(preferred);
    ...

I was wondering if we could get rid of the null check, and just do -

Window w = findDialogParent(preferredParent);

But I think that only makes sense if we removed the checks for NbPresenter etc. What are the chances we have an active modal dialog that is filtered out by that check?

[rkeen-siemens]

I started going down that route and testParent failed because it expected the input frame as the parent which it didn't get since the frame was not visible. After overriding that in the test method, it fell through to the other logic which would have it return the main window unless I customized the input frame further. Since I don't really understand the extra logic and didn't want to change the behavior I thought it would be best to just use the preferredParent unless it was not valid.

[neilcsmith-net]

@rkeen-siemens well, I've made the change as you suggested here and testParent still failed - seems to be another test leaving a modal dialog open. I noticed it was passing when run by itself.

Have opened #6216 which fixes that, changes the Utilities API, and updates a couple of docs.

[neilcsmith-net]

@rkeen-siemens @errael I wouldn't normally do this way, but have addressed some of the comments here in another PR #6216 . I want to get in before branching for NB19 so we can get in the first release candidate in case of problems. Please double-check that and if necessary we can make further changes in delivery before rc2. Thanks!

[mbien]

1-2 risky merges per release are ok - otherwise the RC phase would be too boring :)

[neilcsmith-net]

We need to revert or fix the code introduced here before we can build 19-rc4 (which will hopefully be our last release candidate). See #6290

We need to make sure the parent is a Frame or Dialog, and never a plain Window - see at https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/java/awt/Dialog.java#L676

[neilcsmith-net]

cc/ @rkeen-siemens @errael - we need to decide whether to revert these changes or merge what is hopefully a fix for NB19. Your input would be welcomed.