Skip to content

Remove lastKnownPointerPosition of ComposeScene and other optimizations#2869

Open
ApoloApps wants to merge 8 commits intoJetBrains:jb-mainfrom
ApoloApps:pointerAndLazyOpts
Open

Remove lastKnownPointerPosition of ComposeScene and other optimizations#2869
ApoloApps wants to merge 8 commits intoJetBrains:jb-mainfrom
ApoloApps:pointerAndLazyOpts

Conversation

@ApoloApps
Copy link
Copy Markdown

@ApoloApps ApoloApps commented Mar 16, 2026

Remove lastKnownPointerPosition by migrating CursorDropdownMenu (its only usage) to use Awt’s MouseInfo.getPointerInfo().location. This avoids having to deprecate the component while maintaining functionality. Also, make some classes in RootOwner be lazily loaded to avoid loading unnecessary classes (whether its because it is not initially used or only needed for example only on tests)

Fixes https://youtrack.jetbrains.com/issue/CMP-9926

Testing

Modified the associated test of CursorDropdown and it correctly passes

Release Notes

N/A

val density = LocalDensity.current
val composeSceneContext = LocalComposeSceneContext.current
return remember {
val mouseLocation = MouseInfo.getPointerInfo().location
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Last known position" was introduced to avoid using this API. I don't think that it's the right way to resolve this TODO

See #432

cc @igordmn @m-sasha who has more context here

Copy link
Copy Markdown
Collaborator

@igordmn igordmn Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"MouseInfo" can be used, but we should move it to composeSceneContext?.platformContext?.currentCursorPosition: Offset (with MouseInfo.getPointerInfo().location.asDpOffset().toOffset(density) implementation for desktop)

Copy link
Copy Markdown

@m-sasha m-sasha Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just get rid of

  • lastKnownPointerPosition
  • rememberCursorPosition
  • rememberCursorPositionProvider
  • CursorDropdownMenu

completely?

CursorDropdownMenu feels weird to me, taking state from some external source. The menu will anyway be shown on some user event (mouse right click), so why can't the caller pass the location to the dropdown menu composable? That's exactly what happens in DropdownMenu...

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will work for Desktop as a getter function so I think it's a good way. But I do think we must first address whether or not CursorDropdownMenu is really necessary in the first place. Seems extremely unjustified having a single property in PlatformContext just for this single usecase in CursorDropdownMenu. This menu is irrelevant since we already have Popup in common API and I don't know whether or not y'all want top reduce platforms specific components (especially when there are common components witth the same behaviour). So I think we can proceed in 2 ways, deprecating CursorDropdownMenu in favour of common Popup APIs or following Igor's recommendation of currentCursorPosition getter in PlatformContext (only overriden in Desktop source set)

Copy link
Copy Markdown
Collaborator

@igordmn igordmn Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These use cases:

  • opening a popup on the cursor position after a click
  • opening a popup on the cursor position programmatically on move (for, example a tooltip)

may just require "the last position received by the last press/release/move event"

This use case:

  • opening a popup on the cursor position programmatically without any previous event (show a tooltip when we opened a new window or a screen)

requires the system-level cursor position.

If we decide that this is a valid use case (maybe not a popular one), then we should have a separate currentCursorPosition API.

If we decide to keep it, we still can reevaluate some API. For example, rememberCursorPosition can be public, and rememberCursorPositionProvider, CursorDropdownMenu, lastKnownPointerPosition can be deprecated. It all depends on how much code users have to write themselves.

want top reduce platforms specific components

For a new API we have 4 options:

  1. (as it was from the very beginning) introduce this API in desktopMain
  2. (as it was before some time later for some API) introduce this API in skikoMain
  3. (as I would want for the API CMP should own) introduce this API in compose-multiplatform/<some new library>/commonMain
  4. (as I would want for the API AOSP should own) introduce this API in AOSP/ui/commonMain

This is a strategy question on which we don't have a specific answer yet. So, for now, we can just keep the current strategy - keep it in desktopMain.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide that this is a valid use case (maybe not a popular one

When this API was introduced, it was decided that it is valid. I we don't decide otherwise today, we can just keep it, and refactor the code.

Copy link
Copy Markdown

@m-sasha m-sasha Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the first two pages of https://github.com/search?q=CursorDropdownMenu&type=code and found only one case where CursorDropdownMenu may be used as intended (https://github.com/andannn/Melodify; I couldn't verify because the actual use is behind too much indirection).

Everyone else uses it with the pattern:

var showMenu by remember { mutableStateOf(false) }
Content(
    onClick = { showMenu = true }
)
CursorDropdownMenu(
    expanded = showMenu)
    ...
)

which is actually a bad pattern, because onClick (it's not actually Modifier.onClick, it's also Button clicks, and Modifier.clickable) can be called on keyboard events, which will cause the dropdown menu to be shown god-knows-where.

Copy link
Copy Markdown
Author

@ApoloApps ApoloApps Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me see if I understand it.
What I see in the usecases you have shown is that there is 2 APIs:

//This is for retrieving last cursor position (which doens't come from a MouseEvent in the scene)
val PlatformContext.currentCursorPosition
//(which is Desktop is a getter and calls MouseInfo.getPointerInfo().location.asDpOffset().toOffset(density))

This one handles the current rememberCursorPosition (as done by the PR). I do think we can keep this with a different naming to better reflect its actual behaviour (does not observe latest pointer position, it is a single time call) like: rememberCursorPositionGetter which would actually return a lambda:

@Composable
private fun rememberCursorPositionGetter(): () -> Offset? {
     val composeSceneContext = LocalComposeSceneContext.current
    return remember {
	{
	composeSceneContext?.platformContext?.currentCursorPosition 
	}
	}
}

Then rememberCursorPositionProvider should be renamed to rememberCursorPopupPositionProvider since it returns a PopupPositionProvider. We could leave CursorDropdownMenu to use rememberCursorPopupPositionProvider under the hood and that it uses rememberCursorPositionGetter under the hood

Then comes the usecases of this below which I am not sure we want at all:

These use cases:
opening a popup on the cursor position after a click
opening a popup on the cursor position programmatically on move (for, example a tooltip)
may just require "the last position received by the last press/release/move event"

this is where it is not that clear we should actually do it since then I should revert everything related to
val lastKnownPointerPosition: Offset? whether it's InputHandler or ComposeScene and that defeats the purpose of this PR in the first place (removing so that we don't have to have logic in InputHandler which uses mutableStateMap under the hood and maybe accessing it in this hot path is not recommended)
This is because we have to filter all PointerEvents received "the last position received by the last press/release/move event"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants