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

Spurious prompt on quit, when the UI starts with boxes checked #89

Open
v-gb opened this issue Jan 18, 2025 · 2 comments
Open

Spurious prompt on quit, when the UI starts with boxes checked #89

v-gb opened this issue Jan 18, 2025 · 2 comments
Labels
bug Something isn't working

Comments

@v-gb
Copy link

v-gb commented Jan 18, 2025

Description of the bug

In the context of jj-vcs/jj#5393, I made jj diffedit start an scm-record ui with the whole diff selected (by setting is_checked: true everywhere). The issue is that, if the UI starts and you quit with 'q' without changing anything, you get prompted by an undesirable "N files changes, are you sure?" message.

Expected behavior

I expect no prompt when the current state matches the initial state.
Or a less principled solution would be to remove the prompt when everything is selected, since that's cheap to recreate.

Actual behavior

There is a prompt if the selection is non-empty.

Version of rustc

No response

@v-gb v-gb added the bug Something isn't working label Jan 18, 2025
@arxanas
Copy link
Owner

arxanas commented Jan 18, 2025

Thanks for reporting. I think I didn't consider the case of intentionally starting with everything selected when I wrote the check. Comparing against the start state or just allowing the state of everything being checked as a "trivial" no-op both sound fine to me.

In principle, the data structures are designed to support serialization/deserialization and therefore pausing/resuming the UI. In that kind of workflow, determining what's changed might be more subtle (does it mean what's changed since the UI was originally launched, or since the most recent launch?). But nobody uses that functionality at present, so we could ignore/remove it.

@v-gb
Copy link
Author

v-gb commented Jan 18, 2025

I tried earlier adding the initial state next to the is_checked boolean in v-gb@f86002a. It seemed easiest, and in principle it would allow a caller to specify what they consider the "initial" value separately from the current value, thus supporting the kind of pause/resume workflow you're talking about. But that breaks the json stability tests.

Does that look crazy? If yes, then a less invasive version would be to clone record_state.files early, and then diff the vector and the clone when quitting (I assume the structure of inside the vectors won't change, only the checkbox booleans).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants