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

Replace using global mouse position with known position for context menus, dropdown menus and tooltips #437

Merged

Conversation

m-sasha
Copy link

@m-sasha m-sasha commented Mar 14, 2023

We are currently using rememberCursorPositionProvider, and through it, the global mouse position state in three places:

  1. DefaultContextMenuRepresentation
  2. Positioning of CursorDropdownMenu.
  3. Positioning of tooltips: TooltipPlacement.CursorPoint

We would like to instead use the position of the mouse event that triggered the displaying of each of these.

Proposed Changes

  1. For DefaultContextMenuRepresentation, simply use the rect in the ContextMenuState.Status.Open it already has available. The modifier that changes the status to Open already places the mouse cursor position there.
  2. For CursorDropdownMenu:
  • Implement DropdownMenuState, a new state class, similar to ContextMenuState. Instead of a Rect, it will hold an IntOffset.
  • Implement a new DropdownMenu overload that will take a DropdownMenuState and be controlled by it.
  • Implement a Modifier.contextMenuOpenDetector(DropdownMenuState) that detects right-clicks and sets the state.status to Open(cursorPosition).
  • The user is expected to combine the new DropdownMenu and contextMenuOpenDetector together with state hoisting to display the dropdown menu (example code below).
  1. For TooltipArea:
  • Add a new overload TooltipPlacement.positionProvider(cursorPosition: IntOffset) and have TooltipArea provide the cursor position to it when it wants to show the tooltip.
  • TooltipPlacement.CursorPoint will use the new cursor position argument to position the tooltip popup.

Added utilities:

  • (private) PopupPositionProviderAtOffset that takes all the necessary arguments to position the popup.
  • Use the new PopupPositionProviderAtOffset from both rememberCursorPositionProvider and from a new popupPositionProviderAtPosition method.
  • Modifier.contextMenuOpenDetector(onOpen) and Modifier.contextMenuOpenDetector(DropdownMenuState)

Example use of DropdownMenu:

fun main() = singleWindowApplication {
    val menuState = remember{ DropdownMenuState() }
    Column{
        Spacer(Modifier.height(100.dp))
        Box(
            modifier = Modifier
                .fillMaxSize()
                .background(Color.Red)
                .contextMenuOpenDetector(menuState)
        ) {
            DropdownMenu(
                state = menuState,
            ){
                DropdownMenuItem(onClick = {}){
                    Text("Item 1")
                }
                DropdownMenuItem(onClick = {}){
                    Text("Item 2")
                }
            }
        }
    }
}

Testing

Test: Manual testing

@m-sasha m-sasha force-pushed the use-existing-coords-in-rememberCursorPositionProvider branch from bec5536 to 735e32a Compare March 14, 2023 21:22
@m-sasha m-sasha changed the title [WIP] Replace using global mouse position with known click position Replace using global mouse position with known position for context menus, dropdown menus and tooltips Mar 14, 2023
@m-sasha m-sasha requested a review from MatkovIvan March 15, 2023 19:32
Copy link
Member

@MatkovIvan MatkovIvan left a comment

Choose a reason for hiding this comment

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

One test fails on CI now. It doesn't seem related, but anyway - please check this

@m-sasha
Copy link
Author

m-sasha commented Mar 16, 2023

One test fails on CI now. It doesn't seem related, but anyway - please check this

It's unrelated, but I'm looking at it separately.

Copy link
Collaborator

@Walingar Walingar left a comment

Choose a reason for hiding this comment

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

Awesome PR! I just provided some comments, please take a look

@m-sasha m-sasha force-pushed the use-existing-coords-in-rememberCursorPositionProvider branch from 6047887 to f232717 Compare March 16, 2023 14:33
Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

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

Thanks! It makes API significantly better.

@m-sasha m-sasha force-pushed the use-existing-coords-in-rememberCursorPositionProvider branch from f232717 to 720e011 Compare March 18, 2023 15:23
@m-sasha m-sasha changed the base branch from nr/tooltip-without-awt to jb-main March 18, 2023 15:24
@m-sasha
Copy link
Author

m-sasha commented Mar 19, 2023

I've also renamed PopupPositionProviderAtOffset to PopupPositionProviderAtPosition and made it public, experimental API.

@m-sasha m-sasha merged commit f1a553f into jb-main Mar 20, 2023
1 check passed
@m-sasha m-sasha deleted the use-existing-coords-in-rememberCursorPositionProvider branch March 20, 2023 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants