Skip to content

Rework interal Modal drawing to better use the the provided Area, allowing for draggable Modals. #6749

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mkalte666
Copy link
Contributor

@mkalte666 mkalte666 commented Apr 14, 2025

Rework modal drawing to better use provided Area
When creating a modal with a custom Area that is Area::is_movable, i
would expect to be able to drag it around.

However, that is not the case.

Modal, in rough terms, currently does the following:

  • Enter the area using Area::show()
  • Draw the backdrop and get its reponse
  • Create a child ui to stop events from going to the backdrop, as that
    might cause clicks inside the frame below to close the modal.
  • Draw the frame, and in it the user content.

The main issue this creates is that drawing the backdrop after entering
the area via Area::show() captures the events before they hit the
area, and thus a moveable area cannot be dragged - it does not see any
events to itself in the first place.

Instead, i have modified Modal::show to do the following

  • Create the backdrop response using Context::create_widget.
  • Then enter the area.
  • Draw the frame, and then the user content.

Note that the child ui is now not required anymore, since as long as the
Area senses click and drag, it eats those events now.

I was then left with two issues:

  • Modal::default_area() cannot ever become a draggable thing (even if
    setting movable to true), as it is anchored, and i cannot reset the
    anchor.
  • If i create a custom Area to be moved around, it is not centered
    anymore, as without an anchor it will just get a auto-position.

The solution for the first problem is that i have

  • Added documentation to Modal::area and Modal::default_area
  • Added a new function, Modal::draggable_area, to give a quick area
    that can be dragged around and behaves as expected.

The second problem is a bit trickier. Will a user want an arbirtrary
position for the modal? If yes, i expect they would set fixed_position,
right? If thats the case, there is no harm in re-centering the modal
every time it is first shown. I query that using the areas id and
Memory::areas().visible_last_frame() and it appears to be working
reasonably well.

If its however expected that the user will use default_pos,
currently that will "break" (as in, defaukt_pos gets ignored) as i have not find a way to query it in Modals code if
default_pos was set. Of course that is just an added function to
Area away, but i didn't want to expand the scope of this.

But for now i think this solution is a sufficient compromise.

Finally, i have also added code to the egui demo lib to demonstrate a draggable
and resizable modal.

All this results in the following - please forgive the lack of cursor, i forgot obs window capture on wayland does that :)

draggable.mp4

CI build on my fork went through, i have updated the tests as well and played around with it quite a bit.

In any case, i hope it helps!

~mkalte

  • I have followed the instructions in the PR template

When creating a modal with a custom Area that is `Area::is_movable`, i
would expect to be able to drag it around.

However, that is not the case.

Modal, in rough terms, currently does the following:
 * Enter the area using `Area::show()`
 * Draw the backdrop and get its reponse
 * Create a child ui to stop events from going to the backdrop, as that
   might cause clicks inside the frame below to close the modal.
 * Draw the frame, and in it the user content.

The main issue this creates is that drawing the backdrop after entering
the area via `Area::show()` captures the events before they hit the
area, and thus a moveable area cannot be dragged - it does not see any
events to itself in the first place.

Instead, i have modified `Modal::show` to do the following
 * Create the backdrop response using `Context::create_widget`.
 * Then enter the area.
 * Draw the frame, and then the user content.

Note that the child ui is now not required anymore, since as long as the
Area senses click and drag, it eats those events now.

I was then left with two issues:
  * Modal::default_area() cannot ever become a draggable thing (even if
    setting movable to true), as it is anchored, and i cannot reset the
    anchor.
  * If i create a custom Area to be moved around, it is not centered
    anymore, as without an anchor it will just get a auto-position.

The solution for the first problem is that i have
  * Added documentation to `Modal::area` and `Modal::default_area`
  * Added a new function, `Modal::draggable_area`, to give a quick area
    that can be dragged around and behaves as expected.

The second problem is a bit trickier. Will a user want an arbirtrary
position for the modal? If yes, i expect they would set fixed_position,
right? If thats the case, there is no harm in re-centering the modal
every time it is first shown. I query that using the areas id and
`Memory::areas().visible_last_frame()` and it appears to be working
reasonably well.

If its however expected that the user will use `default_pos`,
currently that will "break" (as in, defaukt_pos gets ignored) as i have not find a way to query it in `Modals` code if
`default_pos` was set. Of course that is just an added function to
`Area` away, but i didn't want to expand the scope of this.

But for now i think this solution is a sufficient compromise.

Finally, i have also added code to the egui demo lib to demonstrate a draggable
and resizable modal.
Copy link

Preview available at https://egui-pr-preview.github.io/pr/6749-draggablemodal
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Awesome! I think a draggable modal is a bit niche, but it's great that you were able to get rid of the extra scope, that is definitely cleaner than before.

Comment on lines +136 to +139
// Should the area be movable, and we are (re-)opening it, try to center it.
if area.is_movable() && !ctx.memory(|mem| mem.areas().visible_last_frame(&area.layer())) {
area = area.current_pos(ctx.screen_rect().center());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this would fail if someone creates a movable modal with a default pos that isn't center. Can we read the position from the Area struct? Or maybe move this to Area with a configurable bool like reset_position_on_open?

Copy link
Contributor Author

@mkalte666 mkalte666 Apr 15, 2025

Choose a reason for hiding this comment

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

We cannot, as far as i can see. We can get last frames position from the memory state IIRC, but that is only marginally helpful.

Some things come to mind in area:
Area currently positions itself like this, if i understand this correctly:

  • If there is an anchor, fix the area on the anchor. Sets movable to false. Setting moveable to true manually, you get an area that always snaps back to the anchor every time you touch it
  • If there is no anchor, but a fixed_pos, use the fixed_pos and it gets the same snappy movement if you force movable to true
  • If there is neither, and current_pos was set, set position to that, otherwise either use the last frames position, or if that is not available, default_pos.

AFAICS this is not documented well, though using Area directly also is kind of non-standard, so shrug.

I'd like to suggest doing the following: Change the way area positions to an AreaPositioning enum. Something Like

pub enum AreaPosition {
   Anchored {anchor: Align2, offset: Vec2},
   Fixed {pos: Pos2},
   Movable { 
       override_pos: Option<Pos2>,
       initial_position: InitialAreaPosition,
       reset_on_open: bool,
   }
}

pub enum InitialAreaPosition {
    Automatic,
    Center,
    DefaultPos(Pos2), // Or OpenAt or just Position... naming is hard. 
}

This would make the builder pattern for Area a bit less nice to use, but it would more or less represent the internal state as it is right now. And you cannot set conflicting properties.

What do you think? I might be able to throw a commit together with this later today.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I think it would be nice to change the Area to hold an enum like this, but we should also definitely keep the current builder methods.

@lucasmerlin lucasmerlin added feature New feature or request egui labels Apr 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
egui feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants