Resolving #16 - Feature: add an "Are you sure you want to quit?" modal #44

mhdmhsni · 2020-06-22T20:50:22Z

this commit will add an ability to the app which asks user for confirmation just before exit, both in normal and loading mode. please feel free to comment or ask for any changes.

squashed commits:

add keyEvent handler function for exiting mode
add Exiting variant to UiMode enum | add prompt_exit method
implement ExitingMode widget render functionality
fix mixed UiState issue

add keyEvent handler function for exiting mode add Exiting variant to UiMode enum | add prompt_exit method implement ExitingMode widget rendering fix mixed UiState issue

mhdmhsni · 2020-06-23T13:05:51Z

should fix #16

imsnif

Hey @mhdmhsni, great work! I really liked your solution with the enum struct. I think it saved a lot of code, good call.
In addition to some comments I left, there's also one more thing we need to do:
If you notice, right now on this branch if you quit the app, it hangs a little. You'll have to press another ctrl-c (or any other key really) to get back to your prompt.
The reason for this is that there's a bit of a hack in main.rs, and we need to update it.
Essentially, you need to change this line: /~https://github.com/imsnif/diskonaut/blob/main/src/main.rs#L115-L116
To just check for the y key. This adds a bit of a delay before we keep listening for user input, so that the app has time to quit and we know we need to exit the loop. I know it's not the most elegant thing, but I could not think of a better solution - sorry for that. :)

imsnif · 2020-06-23T13:44:46Z

src/input/controls.rs

+        | Event::Key(Key::Esc)
+        | Event::Key(Key::Backspace)
+        | Event::Key(Key::Char('n')) => {
+            if app.loaded {


There is an app.reset_ui_mode() method that does this. You can use it in both of these conditions.

Sorry! I missed the method.
Thanks for the tip!

imsnif · 2020-06-23T13:49:18Z

src/ui/modals/confirm_box.rs

+        .modifier(Modifier::BOLD);
+
+    let text_max_length = confirm_rect.width - 4;
+    let confirm_text = truncate_middle(confirm_msg, text_max_length);


Right now, what happens with this line is that the "Are you sure you want to quit?" line gets truncated on a small window to something like "Are you su[...] to quit?"
Do you think we can have a few options here and make it "shorten" itself if the window is not wide enough?
eg. "Are you sure you want to quit?" ==> "Sure you want to quit?" ==> "Really quit?" ==> "Quit?"
There are a few examples of this in the code, like here: /~https://github.com/imsnif/diskonaut/blob/main/src/ui/term_too_small.rs#L16-L33

Sure. i think i can handle that with the explanation you just did.
I'll inform you in case of any problem.

imsnif · 2020-06-23T13:50:24Z

src/ui/modals/confirm_box.rs

+    );
+    buf.set_string(
+        y_n_line_start_position,
+        confirm_rect.y + confirm_rect.height / 2,


Can we lower the y/n line a little so the message is more spread out on the window? I think it'll look a little better. What do you think?

Of course! thanks for the remark. To be honest, it wasn't a good idea in the first place now that i think of it.
I think it might be good if i add a "+ 3" at the end of the line:
confirm_rect.y + confirm_rect.height / 2 + 3,
(this is the exact position of the y/n line in deletion_prompt.)

imsnif · 2020-06-23T13:57:51Z

Also, about merging from main: we changed controls.rs in a different PR to now have a macro called key that makes things a little more readable. I think if you take the file as-is from main and update your prompt_exit methods, you should be good, but let me know if you're having trouble or if I wasn't clear enough.

mhdmhsni · 2020-06-23T17:08:05Z

Hey @mhdmhsni, great work! I really liked your solution with the enum struct. I think it saved a lot of code, good call.

Thanks! Getting this kind of feedback is very heartwarming and encouraging. :)

Essentially, you need to change this line: /~https://github.com/imsnif/diskonaut/blob/main/src/main.rs#L115-L116

Thank you. fixed that.

Also, i think i have to thank you for your patience and your useful descriptive explanations. ❤️

mhdmhsni · 2020-06-23T19:16:50Z

Hi, @imsnif, i made the changes. but there are 2 things i have to mention:

i edited Exiting enum struct and removed "message" because i thought it was not needed anymore after resolving this problem:

Right now, what happens with this line is that the "Are you sure you want to quit?" line gets truncated on a small window to something like "Are you su[...] to quit?"

about the state where app should go back from confirm modal to normal mode or loading mode, as you suggested, i used reset_ui_mode method, but i had to call app.render() after that in my key_handler function, because it would not update the ui otherwise.

imsnif · 2020-06-24T08:42:05Z

Hey @mhdmhsni, thanks for all the changes. Looks like this is a bigger issue than I initially thought. :) There are a few more things we need to do before we can merge this. I'll list them here, but let me know if you want a hand with them:

I resolved the merge conflicts (this time I did it from the Github web ui because there weren't a lot of them, but next time if you want you can also do them by issuing git rebase main, or git merge --no-ff main, as you wish).
Now that the conflicts are resolved, the tests have run and failed because of formatting issues. To resolve this, run cargo fmt (if you don't have rustfmt installed, you can install it from here: /~https://github.com/rust-lang/rustfmt).
Once the formatting issues are resolved, the tests will run and fail. The reason they will fail is that all tests in their end use q or ctrl-c to quit, and now this does not quit the app. We'll need to add a short wait after quitting to each one of them, and then add a y keypress. This happens (for example) here: /~https://github.com/imsnif/diskonaut/blob/main/src/tests/cases/ui.rs#L662
Once we do that to the tests, each test will have a "snapshot" added to it (a snapshot is the textual representation of what happens on the screen). This is because now, when we quit we get a modal. This is good, because it means we will also be testing your change in all of our tests, but we need to account for it. To do that:

We need to update the amount of snapshots we assert for (I think just increment it by one, but run the tests to see what it's expecting): /~https://github.com/imsnif/diskonaut/blob/main/src/tests/cases/ui.rs#L728
We need to add another assertion for the new snapshot. This would mean adding another line here: /~https://github.com/imsnif/diskonaut/blob/main/src/tests/cases/ui.rs#L732

About snapshot testing with insta: we use insta (/~https://github.com/mitsuhiko/insta) for snapshot testing. It gives us the assert_snapshot macro. The first time it is run, it has no snapshot to compare to, so it creates one in the snapshots folder. If you install cargo-insta on your computer, you will be able to review them by doing cargo insta review after the snapshot is created. You'll see the new snapshot (in this case, the "are you sure?" modal) and be able to approve it or reject it. Once you approve it, the test should pass when you run it again.

I plan on writing all of this up in a CONTRIBUTING.md file, so please let me know if anything is unclear.

Thanks again for your work on this and for sticking by. I hope this is not more than you hoped for :)

mhdmhsni · 2020-06-24T14:20:07Z

Hey @imsnif, it seems that i had only one job and i failed it successfully! :) (sorry for the dad joke!)
anyway thanks again and again for being nice. it means a lot to me, as this is my first contribution to a open-source rust project. i like this project, i enjoy working on it and i like being challenged! :) So, it's not more than i hoped for. :)

about the issues you mentioned, I'll work on them as soon as i can. i do not see anything unclear about the things you said. it's clear enough by me. but if i faced any problem resolving these ones i'll let you know.

imsnif · 2020-06-25T10:30:53Z

Sounds great, @mhdmhsni - take your time, I'm here if you need anything.

mhdmhsni · 2020-06-25T15:07:16Z

Hey @imsnif, there are a few things i have to say:

The app crashes when user sets terminal width exactly to 50.
what happens here is when user tries to open any modal (exit_prompt, delete_file_or_folder_prompt, etc.) the app panics with internal error: entered unreachable code: app should not be rendered if window is so small. while the app has been rendered in normalMode or LoadingMode. actually, width=50 is not being handled in MessageBox or any other modal's render method.
it can be resolved by changing this line
else if area.width > 50
to this:
else if area.width >= 50
i think i have to fix this one for all of the modals at once. but if there is anything that i have to know about this change or any possible consequences, please feel free to let me know.
While the app is in ScreenTooSmall mode, it means nothing can be rendered except the related message. do i have to force the app to render ConfirmModal regardless of what the size of the terminal window is? if YES, there would be another problem! currently, display module checks for app_loaded to decide what to render (NormalMode widgets or LoadingMode widgets). i think in that case, we need a more concrete way to check for the current ui state, rather than just passing app_loaded and issuing an if-else-if statement because now we have to handle ScreenTooSmall too to render ConfirmModal before quitting.
about the tests, none of them can finish their job beacause they cannot quit the app and they run forever. adding y keypress and some delay before it, causes the tests to be failed immediatly. i'm investigating to find the cause, but any suggestion from you can be helpful.

imsnif · 2020-06-26T07:09:02Z

Hey @imsnif, there are a few things i have to say:

1. The app crashes when user sets terminal width exactly to 50.
   what happens here is when user tries to open any modal (exit_prompt, delete_file_or_folder_prompt, etc.) the app panics with `internal error: entered unreachable code: app should not be rendered if window is so small`. while the app has been rendered in normalMode or LoadingMode. actually, `width=50` is not being handled in MessageBox or any other modal's render method.
   it can be resolved by changing this line
   `else if area.width > 50`
   to this:
   `else if area.width >= 50`
   i think i have to fix this one for all of the modals at once. but if there is anything that i have to know about this change or any possible consequences, please feel free to let me know.

Oi, that's not good. Thanks for finding this! Yes, please change them all. :)

2. While the app is in ScreenTooSmall  mode, it means nothing can be rendered except the related message. do i have to force the app to render ConfirmModal regardless of what the size of the terminal window is? if YES, there would be another problem! currently, display module checks for `app_loaded` to decide what to render (NormalMode widgets or LoadingMode widgets). i think in that case, we need a more concrete way to check for the current ui state, rather than just passing `app_loaded` and issuing an if-else-if statement because now we have to handle ScreenTooSmall too to render ConfirmModal before quitting.

Hmm... I think the best solution in this case would be to skip the ConfirmModal. Because ScreenTooSmall can also mean that the terminal is 2 columns wide, and then we can't render anything no matter how hard we try. :) In this case, I don't want to force the user to go to another terminal and sudo pkill diskonaut... so let's assume that if this happens and the user presses ctrl-c, they really want to quit and did not do this by accident.

3. about the tests, none of them can finish their job beacause they cannot quit the app and they run forever. adding `y` keypress and some delay before it, causes the tests to be failed immediatly. i'm investigating to find the cause, but any suggestion from you can be helpful.

That's what I would do too... I would imagine now they fail because we need to update both the amount of snapshots we're expecting (because we added one with the confirm modal) and that the expected_terminal_events also needs to be updated. Let me know if you want a hand looking deeper into these.

fix panic in modals when terminal-width=50 listen for all possible keys when user tries to exit exit without confirmation in ScreenTooSmall mode fix tests to be compatible with ConfirmModal Changes

…snif#45) * Make enter select largest folder if nothing is selected * Rename method * Renamed and changed method to do what it originally said * Efficiency improvements * Added test for the feature * Run cargo insta review * Fixed len for assert_eq! * Fixed asserts at end of test * Run cargo insta review again

* Add <q> shortcut in the legend * Fix typo for description * Use <arrows> instead of <hjkl> or <arrow keys> * Apply fmt * Merge main

…imsnif#53) * Keep a stack up visited items and make go_up use it This will allow us to keep track of where our previous selections were so can can automatically select the parents when we go up in the hierarchy. Closes imsnif#48. Signed-off-by: Daniel Egger <daniel@eggers-club.de> * Redo snapshot tests to fix failures Signed-off-by: Daniel Egger <daniel@eggers-club.de> * style(naming): minor naming change for clarity Co-authored-by: Aram Drevekenin <aram@poor.dev>

…msnif#51) * Fix crash when truncating to middle of a character * Fix alignment of file names with wide characters * Respect use ::formatting convention

mhdmhsni · 2020-06-26T16:02:59Z

Hi @imsnif, there's a little conflict inside of CHANGELOG.md. and i'm sorry because i forgot to squash the commits after merging main branch into this one!

-- nothing fancy has changed, just fixed the tests, snapshots, fixed that terminal_width=50 panic issue, a little change in test_utils that i thought it might be better than defining an extra function which does the the same job. (added an extra parameter to sleep_and_quit_events signature.)

thanks again for all your helps. and if anything needs to change, please feel free to let me know.

UPDATE: fixed the conflict

imsnif

Everything looks great.
Thank you for your meticulous work and attention to detail! I'm very happy about this feature. :)

I'm hoping to see more contributions from you in the future.

mhdmhsni · 2020-06-27T15:16:21Z

Thank you! :)
To be honest, I've learnt a lot from you and from challenges i had. and i think i'm going to continue my contribution to this project as soon as i can. :)

feat(app): show confirm modal before exit

4cc8c6d

add keyEvent handler function for exiting mode add Exiting variant to UiMode enum | add prompt_exit method implement ExitingMode widget rendering fix mixed UiState issue

imsnif requested changes Jun 23, 2020

View reviewed changes

resolved requested changes

1a9e0e8

Merge branch 'main' into issue16

5cd6cf3

Mehdi Mohseni and others added 11 commits June 26, 2020 19:44

feat(app) add confirm modal before exit

a3b1b13

fix panic in modals when terminal-width=50 listen for all possible keys when user tries to exit exit without confirmation in ScreenTooSmall mode fix tests to be compatible with ConfirmModal Changes

docs(readme): add gentoo installation info (imsnif#47)

b9bc6ac

docs(changelog): enter largest folder

a407560

docs(readme): fix error in how it works

766b430

feat(ui): show quit shortcut ('q') in the legend (imsnif#46)

ef3803a

* Add <q> shortcut in the legend * Fix typo for description * Use <arrows> instead of <hjkl> or <arrow keys> * Apply fmt * Merge main

fix(formatting): prevent crashes on files with multibyte characters (i…

fcfbf38

…msnif#51) * Fix crash when truncating to middle of a character * Fix alignment of file names with wide characters * Respect use ::formatting convention

docs(changelog): update recent changes

ec95ecd

chore(release): 0.4.0

5676b94

fix enter_largest_folder_with_no_selected_tile test and its snapshot

f126031

Merge remote-tracking branch 'imsnif/main' into issue16

50d2711

imsnif approved these changes Jun 27, 2020

View reviewed changes

imsnif merged commit 2a3e2b6 into imsnif:main Jun 27, 2020

mhdmhsni deleted the issue16 branch June 27, 2020 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolving #16 - Feature: add an "Are you sure you want to quit?" modal #44

Resolving #16 - Feature: add an "Are you sure you want to quit?" modal #44

mhdmhsni commented Jun 22, 2020

mhdmhsni commented Jun 23, 2020

imsnif left a comment

imsnif Jun 23, 2020

mhdmhsni Jun 23, 2020

imsnif Jun 23, 2020

mhdmhsni Jun 23, 2020

imsnif Jun 23, 2020

mhdmhsni Jun 23, 2020

imsnif commented Jun 23, 2020

mhdmhsni commented Jun 23, 2020

mhdmhsni commented Jun 23, 2020

imsnif commented Jun 24, 2020

mhdmhsni commented Jun 24, 2020

imsnif commented Jun 25, 2020

mhdmhsni commented Jun 25, 2020

imsnif commented Jun 26, 2020

mhdmhsni commented Jun 26, 2020 •

edited

Loading

imsnif left a comment

mhdmhsni commented Jun 27, 2020

Resolving #16 - Feature: add an "Are you sure you want to quit?" modal #44

Resolving #16 - Feature: add an "Are you sure you want to quit?" modal #44

Conversation

mhdmhsni commented Jun 22, 2020

mhdmhsni commented Jun 23, 2020

imsnif left a comment

Choose a reason for hiding this comment

imsnif Jun 23, 2020

Choose a reason for hiding this comment

mhdmhsni Jun 23, 2020

Choose a reason for hiding this comment

imsnif Jun 23, 2020

Choose a reason for hiding this comment

mhdmhsni Jun 23, 2020

Choose a reason for hiding this comment

imsnif Jun 23, 2020

Choose a reason for hiding this comment

mhdmhsni Jun 23, 2020

Choose a reason for hiding this comment

imsnif commented Jun 23, 2020

mhdmhsni commented Jun 23, 2020

mhdmhsni commented Jun 23, 2020

imsnif commented Jun 24, 2020

mhdmhsni commented Jun 24, 2020

imsnif commented Jun 25, 2020

mhdmhsni commented Jun 25, 2020

imsnif commented Jun 26, 2020

mhdmhsni commented Jun 26, 2020 • edited Loading

imsnif left a comment

Choose a reason for hiding this comment

mhdmhsni commented Jun 27, 2020

mhdmhsni commented Jun 26, 2020 •

edited

Loading