Add pop stash command on Staches tab #628
Conversation
|
This is what it looks like
|
brunogouveia
force-pushed
the
stash-pop
branch
2 times, most recently
from
5312e1f to
be6d875
Compare
asyncgit/src/sync/stash.rs
Outdated
| @@ -44,6 +44,28 @@ pub fn stash_drop(repo_path: &str, stash_id: CommitId) -> Result<()> { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| /// | |||
| pub fn stash_pop( | |||
There was a problem hiding this comment.
please add matching unittests for this new method
asyncgit/src/sync/stash.rs
Outdated
| pub fn stash_pop( | ||
| repo_path: &str, | ||
| stash_id: CommitId, | ||
| allow_conflicts: bool, |
There was a problem hiding this comment.
lets simplify the signature as long as we do not make use of allow_conflicts to be false anyway
There was a problem hiding this comment.
My reasoning was to follow how we implemented stash_apply, which also is only called with allow_conflicts: false. Tbh I assumed there was plans to eventually use this flag, but I can remove it for now.
src/keys.rs
Outdated
| @@ -114,6 +115,7 @@ impl Default for KeyConfig { | |||
| stashing_toggle_index: KeyEvent { code: KeyCode::Char('i'), modifiers: KeyModifiers::empty()}, | |||
| stash_open: KeyEvent { code: KeyCode::Right, modifiers: KeyModifiers::empty()}, | |||
| stash_drop: KeyEvent { code: KeyCode::Char('D'), modifiers: KeyModifiers::SHIFT}, | |||
| stash_pop: KeyEvent { code: KeyCode::Char('P'), modifiers: KeyModifiers::SHIFT}, | |||
There was a problem hiding this comment.
I'd argue that pop should be the default: using enter as the key and apply should be the new key (maybe even a)
There was a problem hiding this comment.
Either way is fine IMHO, but I slightly prefer apply to be the default (since is the action I mostly use); I only use pop or drop when I'm absolutely sure I don't need that stash anymore.
The reason I decided to create a new key is to not implicitly change the current user experience (e.g., I usually don't check the action bar in order to figure out which key combination triggers an action) and have users accidentally dropping stashes because enter used to be git apply. We could make the default action configurable (and we could extend this concept to any other tab), although I don't think that is currently supported (?).
Let me know if I should make pop the default.
There was a problem hiding this comment.
I am not a fan of config files - there is none in gitui (aside keys and theme)
I was thinking about the same and I would agree if the new action was implicit but it is not it introduces a confirmation
There was a problem hiding this comment.
Fair enough. I changed stash pop to be the default and changed stash apply to key a
| sync::stash_drop(CWD, id).is_ok() | ||
| } | ||
|
|
||
| fn pop(&self, id: CommitId) -> bool { | ||
| match sync::stash_pop(CWD, id) { |
There was a problem hiding this comment.
This pattern seems pretty common: we run a git command synchronously and queue a InternalEvent when it succeeds or a ShowErrorMsg otherwise. I will take a look in a more DRY approach for the common cases. I don't want to make it part of this PR though.
|
@brunogouveia Really cool, thanks for this!❤️ |
As an alternative to asking whether the stash should be removed every time the user applies it, I added the
stash popcommand to theStashestab.I personally prefer having two commands: one for quick apply and one for pop.
Let me know what you think.
Closes #574