n00kii / egui-modal

a simple modal library for egui
MIT License
57 stars 9 forks source link

Calling `Modal::close()` inside a `Ui::input_mut()` results in a deadlock #15

Open Ununoctium117 opened 4 months ago

Ununoctium117 commented 4 months ago

In my application, I wanted to allow keyboard shortcuts to interact with/close the modal as well. This was my first attempt:

let modal = Modal::new(ui.ctx(), "confirmation_modal");
// shows the dialog if needed, as tracked by modal-internal state
modal.show(|ui| {
    // <snip> add other UI...
    modal.buttons(ui, |ui| {
        if confirmation_modal.button(ui, "Cancel (N)").clicked() {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DoNothing);
        }
        if confirmation_modal.button(ui, "Delete New (Del)").clicked() {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DeleteNew);
        }
        if confirmation_modal.button(ui, "Move Anyway (M)").clicked() {
            state.confirmation_modal_state.result = Some(ConfirmationAction::CopyAnyway);
        }
        if confirmation_modal
            .button(ui, "Replace Existing (R)")
            .clicked()
        {
            state.confirmation_modal_state.result = Some(ConfirmationAction::ReplaceOld);
        }
    });

    ui.input_mut(|input| {
        if input.consume_key(Modifiers::CTRL, Key::R) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::ReplaceOld);
            modal.close();
        } else if input.consume_key(Modifiers::CTRL, Key::M) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::CopyAnyway);
            modal.close();
        } else if input.consume_key(Modifiers::CTRL, Key::Delete) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DeleteNew);
            modal.close();
        } else if input.consume_key(Modifiers::CTRL, Key::N) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DoNothing);
            modal.close();
        }
    });
});

However, this caused deadlocks when any of the consume_key calls returned true. I worked around this issue by changing to the following:

    let should_close = ui.input_mut(|input| {
        if input.consume_key(Modifiers::CTRL, Key::R) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::ReplaceOld);
            true
        } else if input.consume_key(Modifiers::CTRL, Key::M) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::CopyAnyway);
            true
        } else if input.consume_key(Modifiers::CTRL, Key::Delete) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DeleteNew);
            true
        } else if input.consume_key(Modifiers::CTRL, Key::N) {
            state.confirmation_modal_state.result = Some(ConfirmationAction::DoNothing);
            true
        } else {
            false
        }
    });

    if should_close {
        modal.close();
    }

I believe this is because modal.close(); internally writes to the context IdTypeMap, which is also locked by input_mut.

I'm not sure what the best solution is, unfortunately. Adding documentation to Modal::close could be helpful, but easily miss-able.

n00kii commented 4 months ago

hi there, thanks for catching this. ill push a patch to update the relevant docs in the next update, though ill keep this issue open until i can better fix this