You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "rkeen-siemens (via GitHub)" <gi...@apache.org> on 2023/05/24 22:23:51 UTC
[GitHub] [netbeans] rkeen-siemens opened a new pull request, #5989: #5987: Use the currently active dialog as the presenter parent
rkeen-siemens opened a new pull request, #5989:
URL: https://github.com/apache/netbeans/pull/5989
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](https://bugs.openjdk.org/browse/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1210620156
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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 https://github.com/apache/netbeans/pull/5280#discussion_r1080908550; passing in a method from a module that this module does not depend on would probably get a runtime error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1206931059
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1562471150
cc/ @errael - can't add you as a reviewer, but good if you can have a look at this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1266567246
##########
platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java:
##########
@@ -95,53 +96,46 @@ public Dialog createDialog (final DialogDescriptor d, final Frame preferredParen
}
return Mutex.EVENT.readAccess (new Mutex.Action<Dialog> () {
public Dialog run () {
- // if a modal dialog active use it as parent
- // otherwise use the main window
- if (NbPresenter.currentModalDialog != null) {
- NbDialog dlg;
- if (NbPresenter.currentModalDialog.isLeaf ()) {
- dlg = new NbDialog(d, WindowManager.getDefault ().getMainWindow ());
- } else {
- dlg = new NbDialog(d, NbPresenter.currentModalDialog);
- }
- customizeDlg(dlg);
- return dlg;
- }
- else {
- Window w = preferredParent;
- if( null == w ) {
- w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
- if (!(w instanceof NbPresenter) || !w.isVisible()) {
- // undocked window is not instanceof NbPresenter although it's NetBeans's native window
- // all docked windows implements ModeUIBase interface
- if (! (w instanceof DefaultSeparateContainer.ModeUIBase)) {
- Container cont = SwingUtilities.getAncestorOfClass(Window.class, w);
- if (cont instanceof DefaultSeparateContainer.ModeUIBase) {
- w = (Window) cont;
- } else {
- // don't set non-ide window as parent
- w = WindowManager.getDefault ().getMainWindow ();
- }
+ Window w = preferredParent;
+ if( null == w ) {
+ w = findDialogParent();
Review Comment:
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1663923710
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1210605653
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1570543615
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1254800256
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1359,51 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null, Utilities::findMainWindow);
+ }
+
+ /**
+ * 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.
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
Review Comment:
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 https://github.com/apache/netbeans/pull/4739#discussion_r997414384. 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 https://github.com/apache/netbeans/pull/5280#discussion_r1083271052
I'm still interestd in hearing a response to https://github.com/apache/netbeans/pull/5989#discussion_r1207270472.
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1581084649
I squashed all the commits into a single commit. Hopefully the checks won't need quite so many restarts this time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1613470072
Now that NB 18 is released, I'm hoping someone has some time now to take a look at the updates here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1640127725
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1210593160
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
@neilcsmith-net Need feedback on API appropriateness.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1569817738
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265423576
##########
platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java:
##########
@@ -95,53 +96,46 @@ public Dialog createDialog (final DialogDescriptor d, final Frame preferredParen
}
return Mutex.EVENT.readAccess (new Mutex.Action<Dialog> () {
public Dialog run () {
- // if a modal dialog active use it as parent
- // otherwise use the main window
- if (NbPresenter.currentModalDialog != null) {
- NbDialog dlg;
- if (NbPresenter.currentModalDialog.isLeaf ()) {
- dlg = new NbDialog(d, WindowManager.getDefault ().getMainWindow ());
- } else {
- dlg = new NbDialog(d, NbPresenter.currentModalDialog);
- }
- customizeDlg(dlg);
- return dlg;
- }
- else {
- Window w = preferredParent;
- if( null == w ) {
- w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
- if (!(w instanceof NbPresenter) || !w.isVisible()) {
- // undocked window is not instanceof NbPresenter although it's NetBeans's native window
- // all docked windows implements ModeUIBase interface
- if (! (w instanceof DefaultSeparateContainer.ModeUIBase)) {
- Container cont = SwingUtilities.getAncestorOfClass(Window.class, w);
- if (cont instanceof DefaultSeparateContainer.ModeUIBase) {
- w = (Window) cont;
- } else {
- // don't set non-ide window as parent
- w = WindowManager.getDefault ().getMainWindow ();
- }
+ Window w = preferredParent;
+ if( null == w ) {
+ w = findDialogParent();
Review Comment:
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` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265471250
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
IMO it seems a useful method. Not sure if it needs to iterate up the window ownership tree though?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1210536747
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
It doesn't seem like a stretch to have it here, but I can make it private to `NodeOperationImpl` if that's more palatable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1572262582
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265670676
##########
platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java:
##########
@@ -95,53 +96,46 @@ public Dialog createDialog (final DialogDescriptor d, final Frame preferredParen
}
return Mutex.EVENT.readAccess (new Mutex.Action<Dialog> () {
public Dialog run () {
- // if a modal dialog active use it as parent
- // otherwise use the main window
- if (NbPresenter.currentModalDialog != null) {
- NbDialog dlg;
- if (NbPresenter.currentModalDialog.isLeaf ()) {
- dlg = new NbDialog(d, WindowManager.getDefault ().getMainWindow ());
- } else {
- dlg = new NbDialog(d, NbPresenter.currentModalDialog);
- }
- customizeDlg(dlg);
- return dlg;
- }
- else {
- Window w = preferredParent;
- if( null == w ) {
- w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
- if (!(w instanceof NbPresenter) || !w.isVisible()) {
- // undocked window is not instanceof NbPresenter although it's NetBeans's native window
- // all docked windows implements ModeUIBase interface
- if (! (w instanceof DefaultSeparateContainer.ModeUIBase)) {
- Container cont = SwingUtilities.getAncestorOfClass(Window.class, w);
- if (cont instanceof DefaultSeparateContainer.ModeUIBase) {
- w = (Window) cont;
- } else {
- // don't set non-ide window as parent
- w = WindowManager.getDefault ().getMainWindow ();
- }
+ Window w = preferredParent;
+ if( null == w ) {
+ w = findDialogParent();
Review Comment:
Possibly, if there is an active modal dialog and it is not `preferredParent`. How about validating `preferredParent`?
```java
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
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1636242791
Squashed the commits optimistically assuming no further changes will be requested, but I can still make updates if necessary.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1258302071
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1359,51 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null, Utilities::findMainWindow);
+ }
+
+ /**
+ * 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.
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] eirikbakke commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "eirikbakke (via GitHub)" <gi...@apache.org>.
eirikbakke commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1629154399
I will test this patch in my NetBeans Platform app and working IDE for a few days.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1563213030
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1206869897
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1206909445
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
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.
```java
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)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1259120715
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1359,51 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null, Utilities::findMainWindow);
+ }
+
+ /**
+ * 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.
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
Review Comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265821776
##########
platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java:
##########
@@ -95,53 +96,46 @@ public Dialog createDialog (final DialogDescriptor d, final Frame preferredParen
}
return Mutex.EVENT.readAccess (new Mutex.Action<Dialog> () {
public Dialog run () {
- // if a modal dialog active use it as parent
- // otherwise use the main window
- if (NbPresenter.currentModalDialog != null) {
- NbDialog dlg;
- if (NbPresenter.currentModalDialog.isLeaf ()) {
- dlg = new NbDialog(d, WindowManager.getDefault ().getMainWindow ());
- } else {
- dlg = new NbDialog(d, NbPresenter.currentModalDialog);
- }
- customizeDlg(dlg);
- return dlg;
- }
- else {
- Window w = preferredParent;
- if( null == w ) {
- w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
- if (!(w instanceof NbPresenter) || !w.isVisible()) {
- // undocked window is not instanceof NbPresenter although it's NetBeans's native window
- // all docked windows implements ModeUIBase interface
- if (! (w instanceof DefaultSeparateContainer.ModeUIBase)) {
- Container cont = SwingUtilities.getAncestorOfClass(Window.class, w);
- if (cont instanceof DefaultSeparateContainer.ModeUIBase) {
- w = (Window) cont;
- } else {
- // don't set non-ide window as parent
- w = WindowManager.getDefault ().getMainWindow ();
- }
+ Window w = preferredParent;
+ if( null == w ) {
+ w = findDialogParent();
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1567206891
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1572559283
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1623708029
Was meaning to cancel my request for changes rather than approve .. :shrug: 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1254809790
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1359,51 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null, Utilities::findMainWindow);
+ }
+
+ /**
+ * 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.
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
Review Comment:
Forgot to mention @neilcsmith-net 's https://github.com/apache/netbeans/pull/5280#discussion_r1073899226 about finding the "real" main window and not using a hardcoded string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1207270472
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1206926048
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
Review Comment:
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](https://github.com/apache/netbeans/pull/5989#discussion_r1206909445))?
```java
/**
* 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.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1638080918
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] mbien commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1640215530
1-2 risky merges per release are ok - otherwise the RC phase would be too boring :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1210542809
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1564892565
Just noticed that in `NbPresenter.java` there's `findFocusedWindow()` which falls back to "NbMainWindow"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1207269420
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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).
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
> 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".
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
> 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()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net merged pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net merged PR #5989:
URL: https://github.com/apache/netbeans/pull/5989
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1663572460
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 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1206147436
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
Review Comment:
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.
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
Review Comment:
`findMainWindow()` is private, probably shouldn't be referenced in javadoc. Could just say the main window is the default if nothing better is found.
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
+ Component parent = suggestedParent;
if (parent == null) {
- parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getActiveWindow();
+ 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) {
// PR#5280
- parent = findMainWindow();
+ return findMainWindow();
}
return parent;
}
+ /**
+ * Gets whether a modal dialog is open.
+ * @return true if a modal dialog is open, false otherwise
+ * @since 9.30
+ */
+ public static boolean isModalDialogOpen() {
Review Comment:
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`.
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1357,50 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null);
+ }
+
+ /**
+ * Finds an appropriate component to use for a dialog's parent. Similar to {@link #findDialogParent()}
+ * with the ability to specify a suggested parent component.
+ *
+ * @param suggestedParent the component to return if suitable
+ * @return A suitable parent component for swing dialog displayers.
+ *
+ * @see #findMainWindow()
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] errael commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "errael (via GitHub)" <gi...@apache.org>.
errael commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1563711291
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265455781
##########
platform/openide.util.ui/src/org/openide/util/Utilities.java:
##########
@@ -1357,17 +1359,51 @@ private static Frame findMainWindow()
*/
// PR4739
public static Component findDialogParent() {
- Component parent = KeyboardFocusManager.getCurrentKeyboardFocusManager().getFocusOwner();
+ return findDialogParent(null, Utilities::findMainWindow);
+ }
+
+ /**
+ * 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.
+ * @since 9.30
+ */
+ public static Component findDialogParent(Component suggestedParent, Supplier<Component> fallback) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] neilcsmith-net commented on a diff in pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#discussion_r1265698678
##########
platform/core.windows/src/org/netbeans/core/windows/services/DialogDisplayerImpl.java:
##########
@@ -95,53 +96,46 @@ public Dialog createDialog (final DialogDescriptor d, final Frame preferredParen
}
return Mutex.EVENT.readAccess (new Mutex.Action<Dialog> () {
public Dialog run () {
- // if a modal dialog active use it as parent
- // otherwise use the main window
- if (NbPresenter.currentModalDialog != null) {
- NbDialog dlg;
- if (NbPresenter.currentModalDialog.isLeaf ()) {
- dlg = new NbDialog(d, WindowManager.getDefault ().getMainWindow ());
- } else {
- dlg = new NbDialog(d, NbPresenter.currentModalDialog);
- }
- customizeDlg(dlg);
- return dlg;
- }
- else {
- Window w = preferredParent;
- if( null == w ) {
- w = KeyboardFocusManager.getCurrentKeyboardFocusManager ().getActiveWindow ();
- if (!(w instanceof NbPresenter) || !w.isVisible()) {
- // undocked window is not instanceof NbPresenter although it's NetBeans's native window
- // all docked windows implements ModeUIBase interface
- if (! (w instanceof DefaultSeparateContainer.ModeUIBase)) {
- Container cont = SwingUtilities.getAncestorOfClass(Window.class, w);
- if (cont instanceof DefaultSeparateContainer.ModeUIBase) {
- w = (Window) cont;
- } else {
- // don't set non-ide window as parent
- w = WindowManager.getDefault ().getMainWindow ();
- }
+ Window w = preferredParent;
+ if( null == w ) {
+ w = findDialogParent();
Review Comment:
Or update and call through to `findDialogParent()` in this class -
```java
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
[GitHub] [netbeans] rkeen-siemens commented on pull request #5989: #5987: Use the currently active dialog as the presenter parent
Posted by "rkeen-siemens (via GitHub)" <gi...@apache.org>.
rkeen-siemens commented on PR #5989:
URL: https://github.com/apache/netbeans/pull/5989#issuecomment-1563178530
> 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`](https://github.com/apache/netbeans/blob/master/profiler/profiler/src/org/netbeans/modules/profiler/impl/ProfilerDialogs.java) is the only place that sets `leaf` to anything other than the default, although it was originally put in place to address [Bugzilla #53525](https://bz.apache.org/netbeans/show_bug.cgi?id=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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists