Conversation
eb5e8c2 to
ab6011d
Compare
|
Needs 2 approvals, as it has new public API (new experimental flag) |
MatkovIvan
left a comment
There was a problem hiding this comment.
LGTM, but blocking until we create 1.10 branch
| var ComposeUiFlags.useLegacyRenderNodeLayers by SkikoComposeUiFlags::useLegacyRenderNodeLayers | ||
|
|
||
| /** This flag enables clearing focus on pointer down by default. */ | ||
| @ExperimentalComposeUiApi |
There was a problem hiding this comment.
Please create a task to remove Experimental with target 1.13
There was a problem hiding this comment.
Shouldn't we remove the flag itself, in 1.12 or so?
There was a problem hiding this comment.
Yes, we need to:
- remove the flag itself from ComposeUiFlags
- keep the ComposePanel/ComposeWindow property, remove Experimental from it
- do that in 1.13, as we release this API in 1.11
There was a problem hiding this comment.
|
|
||
| /** This flag enables clearing focus on pointer down by default. */ | ||
| @ExperimentalComposeUiApi | ||
| var ComposeUiFlags.isClearFocusOnPointerDownEnabled by SkikoComposeUiFlags::isClearFocusOnPointerDownEnabled |
There was a problem hiding this comment.
Android moved this flag to a ComposeView property.
It is probably legit to set different values for different cases, so I propose to do the same in ComposeWindow, ComposePanel and Window composables (still keep it experimental).
Cases in my mind where you don't want to clear focus:
- IntelliJ interop
- text editors (keep focus in editor when you click on a toolbar or a scrollbar)
There was a problem hiding this comment.
The value needs to be available in BaseComposeScene. What is the correct way to pass it there?
There was a problem hiding this comment.
SceneContext or PlatformContext
There was a problem hiding this comment.
Note that this is not only a desktop feature, so we will need flags in iOS and web interop entry points.
There was a problem hiding this comment.
It should provide working-no-op impl and must keep optional for platforms to implement
There was a problem hiding this comment.
IntelliJ interop
If we want to provide seamless interop, we need to keep this ability, as It doesn't clear focus on an empty space (open Settings, focus the Search field, click on an empty space).
text editors
Not so sure if it is right to configure it on global level, but from UX perspective It may look bad when the app loses focus out of the editor when I press on scrollbar, statusbar or toolbar (for example to switch to Bold)
If it's not standard desktop behavior to clear focus, then indeed we should allow both options.
I checked native apps and:
- it clears focus in WinUI (new Windows API) apps
- it doesn't clear focus in Win32 apps (old Windows API)
- it doesn't clear focus in AppKit apps
My experiments:
-
Windows (doesn't clear)
- File properties
- Notepad
- Nvidia Control Panel (seems uses Win32 API)
-
Windows (clears)
- Settings
- Paint
-
macOS (doesn't clear)
- Settings
- File properties
- TextEdit save window
- Choose file dialog (Search field)
- Grapher
- Activity monitor main table
-
macOS (clears)
- Activity monitor search field
- Finder search field
There was a problem hiding this comment.
If we go with the logic that behaviors need to be controlled per entry point, we should allow controlling from the entry point most behaviors currently controlled by global flags
We can follow the strategy - if we think the lack of configurability can introduce significant flaws, then we need to add configurability. Lack of configurability of the first focused element seems doesn't introduce significant flaws.
There was a problem hiding this comment.
It's not about the configurability itself, but about the granularity at which we need the configurability. Is per-application configurability enough, or do we need something more granular? Per window? Per entry point? Per element?
There was a problem hiding this comment.
Is per-application configurability enough, or do we need something more granular
Per-application granularity for temporary flags is enough. Granularity for something permanently configurable should be lower, at least because of common code principles ("no globals"), and because of different use cases in different part of the application.
Ideally it should be not just some property, but just an ability to achieve the desired behavior. In our example - the previous behavior (without clearing focus) was more configurable, because users that want to clear focus could just add a focusable background. And users that don't want to clear focus - just keep the default behavior. With the new feature, users that don't want to clear focus have no option.
There was a problem hiding this comment.
Per-application granularity for temporary flags is enough
Personally I would create a common class that we can pass to every entry point, it looks better design in my eyes, but it is not so important. More important is to keep the established AOSP design for temporary flags.
7f64316 to
a5cca00
Compare
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposeDialog.desktop.kt
Outdated
Show resolved
Hide resolved
compose/ui/ui/src/desktopMain/kotlin/androidx/compose/ui/awt/ComposePanel.desktop.kt
Outdated
Show resolved
Hide resolved
3cf937f to
3ee1038
Compare
Is it created? Can we merge this? |
…Dialog and ComposePanel
…poseContainer` is created.
…it the default value for the entry point flags.
3ee1038 to
43537cc
Compare
Not yet |
igordmn
left a comment
There was a problem hiding this comment.
LGTM for merging after the branch creation
|
Hi |
Originally implemented in AOSP here
Fixes https://youtrack.jetbrains.com/issue/CMP-8891/Implement-Click-To-Clear-focus
Testing
Added a unit test and also tested manually with a few text fields.
Release Notes
Features - Multiple Platforms
isClearFocusOnMouseDownEnabled = falseinComposePanel,ComposeWindoworComposeDialog.