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

Text Selection does not work correctly with multiple deferred viewports when the unfocused viewport requires repainting #4758

Closed
lukexor opened this issue Jul 2, 2024 · 1 comment · Fixed by #4760
Assignees
Labels
bug Something is broken viewports multiple viewports, viewports API
Milestone

Comments

@lukexor
Copy link

lukexor commented Jul 2, 2024

Describe the bug

This is a bit of an odd case and took me a long time to track down.

Use case:

Two windows - a primary and a secondary.
Primary window requires re-painting at 60fps (rendering animations/frames/etc)
Secondary window only requires re-painting in response to user interaction (e.g. a preferences window)

Trying to select text in the secondary window works fine only when it's the one being repainted. If the primary window repaints after selection has started, selection is cleared.

The relevant lines of code are from the label_text_selection.rs module:

        if !state.has_reached_primary || !state.has_reached_secondary {
            // We didn't see both cursors this frame,
            // maybe because they are outside the visible area (scrolling),
            // or one disappeared. In either case we will have horrible glitches, so let's just deselect.

            let prev_selection = state.selection.take();

I suspect the reason this happens is because the store/load happening for the temporary selection state is not keyed at all by viewport, and because deferred viewports require their own begin/end frame cycle it ends up clearing the selection since the primary window was repainted and no widgets matched the current selection state.

    pub fn load(ctx: &Context) -> Self {
        ctx.data(|data| data.get_temp::<Self>(Id::NULL))
            .unwrap_or_default()
    }

    pub fn store(self, ctx: &Context) {
        ctx.data_mut(|data| {
            data.insert_temp(Id::NULL, self);
        });
    }

To Reproduce

  1. Modify the test_viewports example to add a ctx.request_repaint_of(ViewportId::ROOT); to the main update method
  2. Open a deferred viewport and try to select a label - it gets immediately cleared
  3. Open an immediate viewport and try to select a label - works as expected

Expected behavior

I'd expect selecting labels to work correctly

Desktop (please complete the following information):

  • OS: Linux

Additional context

I was able to get a working version with the following. If it's acceptable, I can create a PR:

    pub fn load(ctx: &Context) -> Self {
        let id = Id::new(ctx.viewport_id());
        ctx.data(|data| data.get_temp::<Self>(id))
            .unwrap_or_default()
    }

    pub fn store(self, ctx: &Context) {
        let id = Id::new(ctx.viewport_id());
        ctx.data_mut(|data| {
            data.insert_temp(id, self);
        });
    }
@lukexor lukexor added the bug Something is broken label Jul 2, 2024
@emilk emilk added the viewports multiple viewports, viewports API label Jul 3, 2024
@emilk
Copy link
Owner

emilk commented Jul 3, 2024

One selection state per viewport makes sense to me 👍

@emilk emilk added this to the 0.28.0 milestone Jul 3, 2024
@emilk emilk self-assigned this Jul 3, 2024
@emilk emilk closed this as completed in 1b06c69 Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken viewports multiple viewports, viewports API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants