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

Ui::dnd_drop_zone() swallows the inner Response #4059

Closed
sowbug opened this issue Feb 16, 2024 · 2 comments · Fixed by #4079
Closed

Ui::dnd_drop_zone() swallows the inner Response #4059

sowbug opened this issue Feb 16, 2024 · 2 comments · Fixed by #4079
Labels
feature New feature or request

Comments

@sowbug
Copy link
Contributor

sowbug commented Feb 16, 2024

The signature is

pub fn dnd_drop_zone<Payload>(
        &mut self,
        frame: Frame,
        add_contents: impl FnOnce(&mut Ui),
    ) -> (Response, Option<Arc<Payload>>)
    where
        Payload: Any + Send + Sync;

Note that add_contents: impl FnOnce(&mut Ui) doesn't return a result. It's thus unclear how we're supposed to know what happened to the contents.

I can do this:

let (inner_response, response, payload) = {
    let mut inner_response = None;
    let (response, payload) =
        ui.dnd_drop_zone::<Payload>(Frame::default(), |ui| {
            inner_response =
                Some(ui.add(Slider::new(&mut value, Normal::range()).text("Label")));
        });
    (inner_response, response, payload)
};
if let Some(inner_response) = inner_response {
    if inner_response.changed() {
      // do something
    }
}

... but that's quite a bit more cumbersome than something like this:

let (response, payload) = {
    ui.dnd_drop_zone::<Payload>(Frame::default(), |ui| {
        ui.add(Slider::new(&mut value, Normal::range()).text("Label"))
    })
};
if let Some(response) = response.inner_response {
    if response.changed() {
        // do_something
    }
}

(That hypothetical code assumes that dnd_drop_zone uses something like an InnerResponse rather than a plain old Response.)

I hope there isn't anything obvious I'm missing here, but I looked at the dnd_drop_zone source, and it looks like there was no provision for add_contents's response to be preserved:

let mut frame = frame.begin(self);
add_contents(&mut frame.content_ui);
let response = frame.allocate_space(self);
@sowbug sowbug added the bug Something is broken label Feb 16, 2024
@emilk
Copy link
Owner

emilk commented Feb 20, 2024

It should be easy enough to change the signature of dnd_drop_zone to:

pub fn dnd_drop_zone<Payload, R>(
        &mut self,
        frame: Frame,
        add_contents: impl FnOnce(&mut Ui) -> R,
    ) -> (InnerResponse<R>, Option<Arc<Payload>>)

if someone wants to take a stab at it

@emilk emilk added feature New feature or request and removed bug Something is broken labels Feb 20, 2024
@sowbug
Copy link
Contributor Author

sowbug commented Feb 20, 2024

I'll give it a look today.

sowbug added a commit to sowbug/egui that referenced this issue Feb 20, 2024
emilk pushed a commit that referenced this issue Feb 21, 2024
* Closes <#4059>

```bash
$ ./scripts/check.sh 
[...]
+ echo 'All checks passed.'
```
hacknus pushed a commit to hacknus/egui that referenced this issue Oct 30, 2024
* Closes <emilk#4059>

```bash
$ ./scripts/check.sh 
[...]
+ echo 'All checks passed.'
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants