Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid activating TopComponents while focus changes in progress. #8288

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

errael
Copy link
Contributor

@errael errael commented Mar 2, 2025

Fix #7952.

  • Use invokeLater() so KeyboardFocusManager changes are finished before possible activation. After focus is settled, do nothing if the original target is not currently focused.
  • Use permanentFocusOwner property (instead of focusOwner) for minor optimization.

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

  • are all your own work, and you have the right to contribute them.
  • are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

  1. Was this PR correctly labeled, did the right tests run? When did they run?
  2. Is this PR squashed?
  3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
  4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

Fix apache#7952.

Use invokeLater() so KeyboardFocusManager changes are finished.
Use "permanentFocusOwner" property (instead of "focusOwner") for minor optimization.
@errael
Copy link
Contributor Author

errael commented Mar 3, 2025

@nmatt could you try this out?

@nmatt
Copy link

nmatt commented Mar 3, 2025

@errael Yes, of course, but I need a pointer of what to install. Is the zip of the daily build under https://ci-builds.apache.org/job/Netbeans/job/netbeans-linux/lastSuccessfulBuild/artifact/nbbuild/ the correct thing to use? Also, thanks for looking into this issue.

@errael
Copy link
Contributor Author

errael commented Mar 3, 2025

Yes, of course, but I need a pointer of what to install. Is the zip of the daily build under https://ci-builds.apache.org/job/Netbeans/job/netbeans-linux/lastSuccessfulBuild/artifact/nbbuild/ the correct thing to use? Also, thanks for looking into this issue.

This follow on to #4603 is ready for review. You'd have to include this PR in your own build of NB to check it out. Looking for reviewers. @neilcsmith-net, @eirikbakke any suggestions?

@nmatt
Copy link

nmatt commented Mar 3, 2025

@errael I have no experience with building NetBeans, I'm not sure how straightforward that would be for me. If someone could provide me with a build to test, that would preferable. I'm on Windows, in case that makes a difference.

@errael

This comment was marked as outdated.

@errael

This comment was marked as outdated.

@matthiasblaesing matthiasblaesing added Platform [ci] enable platform tests (platform/*) ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Mar 5, 2025
@apache apache locked and limited conversation to collaborators Mar 5, 2025
@apache apache unlocked this conversation Mar 5, 2025
@matthiasblaesing
Copy link
Contributor

Labeled as "Platform" and "ci:dev-build". The former runs the platform unittests, the latter will provide a nightly build of this PR. This is not a review.

@eirikbakke @neilcsmith-net you might want to have a look.

@matthiasblaesing
Copy link
Contributor

@nmatt
Copy link

nmatt commented Mar 6, 2025

@errael I downloaded the build linked by Matthias and will test it tomorrow.

@nmatt
Copy link

nmatt commented Mar 7, 2025

@errael I tested the build, and it seems to work mostly fine again the way it did work in NetBeans 15 (the last version before it broke).

However, there is one difference,that I noticed: With the keyboard focus in the floating Projects pane, when closing a project by keyboard via the File menu (Alt+F, C), the keyboard focus doesn't revert back to the Projects pane the way it did previously (up to NetBeans 15). It would be nice if this worked again, though not crucial.

@errael
Copy link
Contributor Author

errael commented Mar 7, 2025

Thanks for checking it out.

With the keyboard focus in the floating Projects pane, when closing a project by
keyboard via the File menu (Alt+F, C), the keyboard focus doesn't revert back to
the Projects pane the way it did previously (up to NetBeans 15).

I disabled all the changes from the original PR by commenting out, in TabbedHandler,

KeyboardFocusManager.getCurrentKeyboardFocusManager()
    .addPropertyChangeListener("permanentFocusOwner", activationManager);

Then, with floating Projects, did

  1. Ctrl-0, Ctrl-1
  2. Alt-F
  3. <ESC>

Observe: (3.) closes the menu and the keyboard focus manager sets the focus to the editor; the Projects tab is still active.

This is the original problem from #4437 fixed by #4603. I also noted that if the editor file is modified, then even though it gets keystrokes, the undo/redo don't work.

If NB-15, and earlier, leaves the keyboard focus in Projects then the change doesn't seem to be due to #4603/#8288.

I tried it with NB-13 and don't see the KB focus left in Projects. Might you be able to verify this one way or the other? Maybe there's some other setting/??? affecting behavior.

@errael
Copy link
Contributor Author

errael commented Mar 7, 2025

With focus in Projects, select 2 projects, then after Alt-F the File menu has Close 2 projects.

@nmatt
Copy link

nmatt commented Mar 7, 2025

@errael I tested with a fresh install of NetBeans 13, and I realized now that the behavior is a bit more subtle than I thought: The desired behavior of the keyboard focus remaining in the Projects view occurs when the project being closed has a file open in the editor (which is also implicitly closed when the project is closed). When the project being closed has no file open, then the keyboard focus ends up in the editor. This is the same in NetBeans 15. I didn't notice this condition before, because usually I have some file open from the project. There is an analogous behavior when reopening a recent project (Alt+F, J) from the Projects view: In that case, the keyboard focus returns to the Projects view when the reopened project has no (remembered) file open. Otherwise the keyboard focus ends up in the editor with one of the reopened files.

However, in the current dev build, the keyboard focus never returns to the Projects view when closing a project, regardless of whether the project being closed has a file open. (It does, however, return to the Projects view when reopening a project without any reopened files.)

Can you reproduce this difference?

Regarding the closing of multiple projects, you are right, I had misremembered that.

@errael
Copy link
Contributor Author

errael commented Mar 7, 2025

...
Can you reproduce this difference?

@nmatt It's not clear to me that there's a reason to explore this difference as part of this PR.

Before NB-16 focus/TC-activation was inconsistent and caused a variety of problems. And reading your post, it looks like where the focus ended up was inconsistent depending on "subtle" initial state differences.

#4603 fixed an obvious bug. After that PR some multi-step focus changes, for example triggered by Alt-F in a floating window, incorrectly activated a TopComponent, see #7952. This PR, #8288, fixes that by waiting for all the steps of the focus change to complete before possibly activating the last focused component.

Neither #4603 nor #8288 change the keyboard focus. Where focus ends up is handled by the KeyboardFocusManger. You might want to open an issue about focus changes that you find problematic. NetBeans uses the DefaultKeyboardFocusManager. I didn't track down how/where Alt-F is handled and why it's multi-step focus change.


BTW, I used NB13 for testing because that's the only pre NB16 I have installed. I'd hoped, and you've verified, that NB13 and NB15 behave the same concerning these issues.

@nmatt
Copy link

nmatt commented Mar 7, 2025

@errael To clarify: NetBeans 16 broke the ability to access main menu actions by keyboard from a floating pane. Your fix restores this ability, but the focus behavior for File > Close has now changed. The change in behavior might very well be due to other changes since NetBeans 15, I can't really judge that. As NetBeans 15 was the last version where that action was accessible in the first place, there is no way to test the behavior in the versions in between. As I wrote in #8288 (comment) it's not crucial; I just mentioned it in case something was overlooked in the present fix. Thanks again for that fix.

The overall keyboard focus behavior between NetBeans components is a bit of a mess in general, so the File > Close behavior is just one detail among many.

@errael
Copy link
Contributor Author

errael commented Mar 8, 2025

the focus behavior for File > Close has now changed.

@nmatt Not really. There's a difference between focused component (Swing concept) and active component (NB concept). Before NB16 it was much easier for active/focused to be different components. The steps in #8288 (comment), leave the Projects window active and at the same time the Editor is focused. This breaks some things, see #4437.

The change in behavior might very well be due to other changes since NetBeans 15

Unlikely.

in case something was overlooked in the present fix

The effect of #4603/#8288 is to TopComponent activate the last focused component of a sequence of focus changes. It is certainly true that the active TopComponent behavior has changed. These PRs do not change the focused component.

The focus change after Alt-F from floating window is a 3 step process. Typically

permanentFocusOwner old ExplorerTree@d60489e, new QuietEditorPane@19568ae9
permanentFocusOwner old QuietEditorPane@19568ae9, new (null)
permanentFocusOwner old (null), new JRootPane@7ac53ef

And then hitting \<ESC> moves focus to the previously focused component

permanentFocusOwner old JRootPane@7ac53ef, new (null)
permanentFocusOwner old (null), new QuietEditorPane@19568ae9

Which is why the active TopComponent becomes the editor. If Alt-F were simply one step, ExplorerTree --> JRootPane, you would probably get the behavior you're looking for. The purpose of this PR is to keep the active TopComponent in sync with the focused component.

This gets us back to the previous message

Where focus ends up is handled by the KeyboardFocusManger.
You might want to open an issue about focus changes that you find problematic.

Perhaps some adjustments to some focus traversal policy would provide more intuitive behavior. That's out of scope for this PR.

@nmatt
Copy link

nmatt commented Mar 8, 2025

@errael Again, to clarify: I have no understanding of the internals of focus handling. I’m only reporting the changes in behavior that I see as a user, compared to NetBeans 15. I do not claim to be able to judge what is or isn’t relevant to this PR; that’s entirely up to your discretion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Platform [ci] enable platform tests (platform/*)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

focus bug when accessing main menu from floating pane
3 participants