-
Notifications
You must be signed in to change notification settings - Fork 107
[CMP-9342] Fix text context menus #2598
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
[CMP-9342] Fix text context menus #2598
Conversation
This changes the `SelectionManager.textManager` implementation for `SelectionContainer`s in `ContextMenu.desktop.kt`, so that the copy action is only available when the selected text is not empty. This lets implementors of `TextContextMenu` decide whether they want to show a disabled copy action in the menu, or not.
This fixes two issues with text context menus: ### 1. The menu should not show if it's empty If the context menu has no items, it should not show. This is easily achieved by only popping up the menu if the items list is not empty. ### 2. The menu should not pop up "after the fact" Once 1 is fixed, there is another issue that crops up: the menu which was not popped up initially, becomes visible as soon as there would be items in it. For example, if no menu pops up in a `SelectionContainer` if the user right-clicks an empty spot, but then does a selection. Same for BTF, if the user types any text in an initially-empty BTF after right clicking it. This is done by explicitly closing the menu session if it contains no items. Note: JPopupContextMenuRepresentation works differently and this only explicitly fixes JetBrains#1 for it. Due to it being a completely different implementation, this does not really suffer from JetBrains#2
|
I don't think this is the right solution, although we may adopt it regardless. What happens here is that the context menu is opened and then immediately closed, which is already not ideal. This is all very strange behavior. I see two better, but more complex, solutions:
This is also being made more complicated because of uncertainty w.r.t. new context menu API from AOSP. |
Correction: this doesn't actually happen. But the menu will get automatically closed if the state changes to "no items". It's also just bad practice to have session-closing logic in the composition. |
|
I agree with @m-sasha that opening and closing menu isn't the right approach. The code that does the opening should check first and don't open the menu. The menu itself should not change state, only react on it (it can do that in event handlers, but not in recomposition/layout/draw) Something like the first approach from m-sasha would work: We froze the items, but not the operations, but I think it is fine. |
Do you think it's ok that we will try to execute operations that may be invalid at the time when they're executed...?
|
|
@m-sasha afaik Swing also freezes the items when the menu opens. It's probably fine in 99% of cases; actions that know they're doing something potentially risky can just throw a try/catch just in case |
We should additionally check the validity during the operation (perform the same check, as for showing menu and the item), but it is fine to perform the same operation on a different state if it is still valid (copy/cut/paste another text). Also, it can be out of scope of this PR, because as far as I understand, the current code already froze the items, we just explicitly do this earlier for showing. |
|
I don't think we're freezing them. They are retrieved in |
Also I see that in the previous state it was without When you changed it in that PR, did you think about this case? If not, we can consider it undefined. If yes, but it is in fact frozen, it is a bug. But we can convert it to a feature ). |
I'm not sure I understand what you mean. Here's an example: Screen.Recording.2025-12-02.at.16.33.35.mp4 |
|
Thanks. So it works. I missed the code inside In the end, which option we choose, should depend on UX we need:
This is probably not UX friendly (copied something that is not seen by user), and difficult to implement
From native OS perspective it is a better UX, but from IDEA perspective it should hide the items (I just checked). So, it should be at most configurable (via custom
It looks fine to me from UX perspective. implemented here (it should already not perform invalid operations)
Also looks fine to me. Maybe even the best from the UX perspective when we hide the items? Implemented in the PR, but we need:
|
I don't think we should force the IDEA/Swing behavior. We should allow it, yes, (via a custom |
|
With #2617 merged, I think we can close this. |
This fixes two issues with text context menus:
1. The menu should not show if it's empty
If the context menu has no items, it should not show. This is easily achieved by only popping up the menu if the items list is not empty.
2. The menu should not pop up "after the fact"
Once 1 is fixed, there is another issue that crops up: the menu which was not popped up initially, becomes visible as soon as there would be items in it. For example, if no menu pops up in a
SelectionContainerif the user right-clicks an empty spot, but then doesa selection. Same for BTF, if the user types any text in an initially-empty BTF after right clicking it.
This is done by explicitly closing the menu session if it contains no items.
Note:
JPopupContextMenuRepresentationworks differently and this only explicitly fixes #1 for it. Due to it being a completely different implementation, this does not really suffer from #2Fixes CMP-9342
Important
This requires CMP-9229 (#2595) to be merged first
Testing
Screen.Recording.2025-11-26.at.16.02.33.mov
This should be tested by QA
Release Notes
Fixes - Desktop