-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fix right-click to select word on macOS (take 2) #1439
Conversation
* This overload does not trigger the opening of the context menu; it's up to the caller to do so | ||
* by passing a [modifier] that does so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be confusing, we can avoid it by default args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's confusing, it's just not very convenient, but since this API is for advanced developers, I think it's better to prefer clarity (who's responsible for what, in this case) over convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, this is confusing for me. One overload looks like a default for another overload. Usual Compose components don't change behaviour between SomeComponent()
and SomeComponent(modifier = Modifier)
(the default value of the modifier parameter MUST be Modifier)
The suggested modifier: Modifier = Modifier.contextMenuOpenDetector(state, true)
looks the same, just as a real default value.
We need to figure out something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to figure out something else.
We need something public that can be reused in custom TextContextMenu
implementations.
I have 2 suggestions:
- Create
PlatformTextContextMenuArea
(it doesn't reuseContextMenuArea
):
@Composable
override fun Area(textManager: TextManager, state: ContextMenuState, content: @Composable () -> Unit) {
...
PlatformTextContextMenuArea({ emptyList() }, textManager, state, content = content)
}
Why PlatformTextContextMenuArea
and not TextContextMenuArea
: additional value that it adds to usual ContextMenuArea
is opinionated behaviour for Desktop/macOs platform (we probably had to rename platformContextMenuDetector
by the same logic)
- Add
onOpen: (() -> Unit)?
@Composable
override fun Area(textManager: TextManager, state: ContextMenuState, content: @Composable () -> Unit) {
...
ContextMenuArea({ emptyList() }, state, onOpen = textManager::selectWordAtPositionIfNotAlreadySelected, content = content)
}
state: ContextMenuState, | ||
modifier: Modifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state: ContextMenuState, | |
modifier: Modifier, | |
state: ContextMenuState = remember { ContextMenuState() }, | |
modifier: Modifier = Modifier.contextMenuOpenDetector(state, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that this would be confusing, because
- The default value for a
modifier
argument is (nearly?) alwaysModifier
. contextMenuOpenDetector
is not the modifier typically needed when using this overload. If you wantcontextMenuOpenDetector
, you should use the overload right above it, that just takes anenabled
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except we need to agree on desktopMain public API
/** | ||
* Returns whether the given pixel position is inside the selection. | ||
*/ | ||
internal fun TextLayoutResult.isPositionInsideSelection( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All commonMain
changes looks good to me.
* This overload does not trigger the opening of the context menu; it's up to the caller to do so | ||
* by passing a [modifier] that does so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, this is confusing for me. One overload looks like a default for another overload. Usual Compose components don't change behaviour between SomeComponent()
and SomeComponent(modifier = Modifier)
(the default value of the modifier parameter MUST be Modifier)
The suggested modifier: Modifier = Modifier.contextMenuOpenDetector(state, true)
looks the same, just as a real default value.
We need to figure out something else.
* This overload does not trigger the opening of the context menu; it's up to the caller to do so | ||
* by passing a [modifier] that does so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to figure out something else.
We need something public that can be reused in custom TextContextMenu
implementations.
I have 2 suggestions:
- Create
PlatformTextContextMenuArea
(it doesn't reuseContextMenuArea
):
@Composable
override fun Area(textManager: TextManager, state: ContextMenuState, content: @Composable () -> Unit) {
...
PlatformTextContextMenuArea({ emptyList() }, textManager, state, content = content)
}
Why PlatformTextContextMenuArea
and not TextContextMenuArea
: additional value that it adds to usual ContextMenuArea
is opinionated behaviour for Desktop/macOs platform (we probably had to rename platformContextMenuDetector
by the same logic)
- Add
onOpen: (() -> Unit)?
@Composable
override fun Area(textManager: TextManager, state: ContextMenuState, content: @Composable () -> Unit) {
...
ContextMenuArea({ emptyList() }, state, onOpen = textManager::selectWordAtPositionIfNotAlreadySelected, content = content)
}
I think both suggestions are overly complicating things to solve a very minor problem. I don't like I don't like adding Conceptually, the new |
9ba6f3b
to
41a9bbc
Compare
Maybe we look at it differently. I look that "detector" (how to react) is the same for usual and text menus. It is rather "handler" (what to do) that is different.
How much "Core" should be in it? How users should decide what to use just looking at the available names? (Option 3)
This can be refactored to (more controversial, not sure that it is good idea): (Option 4)
|
That happens to be true for what we're doing in this PR, but that doesn't mean that it's the right way to think about this API. The way I'm looking at it is that there's a more generic
This is a super common pattern in Compose, and elsewhere. Again, the problem here is just with being unable to provide the default via a default argument. If it weren't for that, we'd have one function with
Sometimes you have to read the documentation... We could rename
Why is it not justified to be extracted? Even one line of code is justified to be extracted if it shares the same meaning and purpose, and isn't trivial. P.S. I really think we're arguing about something very inconsequential here, so @igordmn please decide and I will implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Needs a second approval
@@ -251,7 +283,7 @@ class ContextMenuState { | |||
} | |||
} | |||
|
|||
object Closed : Status() | |||
data object Closed : Status() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it applies to data object
s. A data object only adds equals, hashcode and toString implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just follow the rule to avoid corner cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But there is no rule about data objects. None of the issues with data classes apply to data objects.
...foundation/src/desktopMain/kotlin/androidx/compose/foundation/ContextMenuProvider.desktop.kt
Show resolved
Hide resolved
I plan to wait with this until https://android-review.googlesource.com/c/platform/frameworks/support/+/3176245 is merged. |
Improve contextMenuOpenAdjustment in SelectionManager and TextFieldSelectionManager and rename them to selectWordAtPositionIfNotAlreadySelected. The new implementations do not select the word if the clicked position is already selected, and do not crash when an empty SelectionContainer is clicked. Corresponding CMP PR: JetBrains#1439 Test: Updated implementations for desktop only Change-Id: I6954cd6a0d3d1febf24a31bc4e228ab445aa54f8
Improve contextMenuOpenAdjustment in SelectionManager and TextFieldSelectionManager and rename them to selectWordAtPositionIfNotAlreadySelected. The new implementations do not select the word if the clicked position is already selected, and do not crash when an empty SelectionContainer is clicked. Corresponding CMP PR: #1439 Test: Updated implementations for desktop only Change-Id: I6954cd6a0d3d1febf24a31bc4e228ab445aa54f8
… SelectionContainer and TextField to TextContextMenu.Area
…xtFieldSelectionManager` and `SelectionManager`
… opening the context menu for selectable text. Also make LocalContextMenuData public.
30aa85b
to
8f41990
Compare
Improve contextMenuOpenAdjustment in SelectionManager and TextFieldSelectionManager and rename them to selectWordAtPositionIfNotAlreadySelected. The new implementations do not select the word if the clicked position is already selected, and do not crash when an empty SelectionContainer is clicked. Corresponding CMP PR: #1439 Test: Updated implementations for desktop only Change-Id: I6954cd6a0d3d1febf24a31bc4e228ab445aa54f8
There are currently several problems with text selection in
SelectionContainer
:SelectionContainer
with no text, the app will crash.Text
in aSelectionContainer
, the app will crash or select the wrong text.Fixes JetBrains/compose-multiplatform#4985
Note that I don't plan to merge this immediately. I want to first get feedback, then upstream the
common
changes to Google, and only then, after cherry-picking from AOSP, merge this PR.Testing
This should be tested by QA
Release Notes
Fixes - Desktop
SelectionContainer
or on the padding of aText
inside aSelectionContainer
.