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

Move undo discard button #1242

Merged
merged 3 commits into from Nov 3, 2017

Conversation

Projects
None yet
6 participants
@kuychaco
Member

kuychaco commented Nov 2, 2017

Fixes #605

undo-discard-button

For now I'm simply going to move the button down. Still open for consideration is removing the button from the UI under certain circumstances, but then the question is when do we remove it? After a certain amount of time? If so, how long? After committing? @BinaryMuse has noted that sometimes she uses this feature for quick stashing (discarding changes, making a commit, then restoring the changes).

As a first step we'll move the button so it's harder to accidentally push it since it's clear that this is causing some pain. If there's still lingering discontentment we can discuss when it is reasonable to remove the button.

@kuychaco kuychaco requested a review from BinaryMuse Nov 3, 2017

@kuychaco kuychaco merged commit 324246e into master Nov 3, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kuychaco kuychaco deleted the ku-move-undo-discard-button branch Nov 3, 2017

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 3, 2017

Fixes #605

I disagree this is a fix for that issue it even hardly constitutes a workaround.

A fix would be to make it possible to dismiss the discard option altogether.

the-j0k3r commented Nov 3, 2017

Fixes #605

I disagree this is a fix for that issue it even hardly constitutes a workaround.

A fix would be to make it possible to dismiss the discard option altogether.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 3, 2017

Contributor

@the-j0k3r With a bit of CSS you can easily remove this button for yourself. Pop open the dev tools and craft a selector and it will be gone. Adding an affordance to dismiss it will clutter the UI and I'd rather find a solution that leaves it visible but makes it unlikely to click by mistake.

@kuychaco did you consider placing the button to the left of Stage All? I still worry it might be accidentally clicked if mixed with the files and it still feels a bit odd there, like an apple among oranges. I do appreciate the attention to this and think this is an improvement though.

Contributor

nathansobo commented Nov 3, 2017

@the-j0k3r With a bit of CSS you can easily remove this button for yourself. Pop open the dev tools and craft a selector and it will be gone. Adding an affordance to dismiss it will clutter the UI and I'd rather find a solution that leaves it visible but makes it unlikely to click by mistake.

@kuychaco did you consider placing the button to the left of Stage All? I still worry it might be accidentally clicked if mixed with the files and it still feels a bit odd there, like an apple among oranges. I do appreciate the attention to this and think this is an improvement though.

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 3, 2017

Adding an affordance to dismiss it will clutter the UI

How is adding an X to this button clutter anything, answer it wont? Click X memory is cleared and button is gone until something is discarded.

As is the contents of discard are held in memory indefinitely, something you discarded 10 commits ago or 20 commits ago is invalid, that is a problem, clicking button by mistake is not even a consideration in thta actual real scenario.

please tell me how I can fix that via css?

the-j0k3r commented Nov 3, 2017

Adding an affordance to dismiss it will clutter the UI

How is adding an X to this button clutter anything, answer it wont? Click X memory is cleared and button is gone until something is discarded.

As is the contents of discard are held in memory indefinitely, something you discarded 10 commits ago or 20 commits ago is invalid, that is a problem, clicking button by mistake is not even a consideration in thta actual real scenario.

please tell me how I can fix that via css?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 3, 2017

Contributor

@the-j0k3r I'm going to reply, but I'd appreciate it if you could be a bit nicer. So is the feature you are seeking to dismiss the button after a certain period of time? I thought you wanted to hide it entirely. If you can articulate the ideal behavior you're seeking that would be a constructive addition to the conversation.

Contributor

nathansobo commented Nov 3, 2017

@the-j0k3r I'm going to reply, but I'd appreciate it if you could be a bit nicer. So is the feature you are seeking to dismiss the button after a certain period of time? I thought you wanted to hide it entirely. If you can articulate the ideal behavior you're seeking that would be a constructive addition to the conversation.

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Nov 3, 2017

Member

did you consider placing the button to the left of Stage All?

@nathansobo actually we did originally have the button in the header to the left of the Stage All button and then moved it (#691). Things were too squished in the header when the panel is resized to be narrow, and I think accidental clicking might also be an issue with the undo button there. IMO having it at the bottom makes it the least likely to accidentally click it, and it seems to look fine amongst a long file list.

undo-discard-button-2

I agree that it feels a bit odd there as well, just not sure where better to place it. @donokuda have any 💭s?

I think I'll get this version onto master and beta for now and we can feel it out and continue to ideate about alternatives

Member

kuychaco commented Nov 3, 2017

did you consider placing the button to the left of Stage All?

@nathansobo actually we did originally have the button in the header to the left of the Stage All button and then moved it (#691). Things were too squished in the header when the panel is resized to be narrow, and I think accidental clicking might also be an issue with the undo button there. IMO having it at the bottom makes it the least likely to accidentally click it, and it seems to look fine amongst a long file list.

undo-discard-button-2

I agree that it feels a bit odd there as well, just not sure where better to place it. @donokuda have any 💭s?

I think I'll get this version onto master and beta for now and we can feel it out and continue to ideate about alternatives

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Nov 3, 2017

Member

@the-j0k3r as stated in the PR description I merely meant this to be a first step to save people some pain now, especially given that our attention and priorities are focused elsewhere at the moment.

Admittedly, it was a bit excessive to close the associated issue rather than simply referencing it in this PR. I'll go ahead and re-open it

Member

kuychaco commented Nov 3, 2017

@the-j0k3r as stated in the PR description I merely meant this to be a first step to save people some pain now, especially given that our attention and priorities are focused elsewhere at the moment.

Admittedly, it was a bit excessive to close the associated issue rather than simply referencing it in this PR. I'll go ahead and re-open it

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 3, 2017

If you can articulate the ideal behavior you're seeking that would be a constructive addition to the conversation.

You did read #605 (comment) and #605 (comment) and my previous articulation to which you replied to here, describes what I see as the problem is.

I actually thought #605 was describing that very same issue I tried to articulate, but I see now that the issue may be beyond that ticket and you are starting to see the actual issue..

So is the feature you are seeking to dismiss the button after a certain period of time?

Not after a period of time, just making the button/memory of that undo discard disposable entirely at ANY time.
That way it can suit peoples actual usage of the function + whatever use beyond the designed function and if like me you have no use to have an undo for an action left behind for something 20 commits or more into the past, you can just clear that memory.

the-j0k3r commented Nov 3, 2017

If you can articulate the ideal behavior you're seeking that would be a constructive addition to the conversation.

You did read #605 (comment) and #605 (comment) and my previous articulation to which you replied to here, describes what I see as the problem is.

I actually thought #605 was describing that very same issue I tried to articulate, but I see now that the issue may be beyond that ticket and you are starting to see the actual issue..

So is the feature you are seeking to dismiss the button after a certain period of time?

Not after a period of time, just making the button/memory of that undo discard disposable entirely at ANY time.
That way it can suit peoples actual usage of the function + whatever use beyond the designed function and if like me you have no use to have an undo for an action left behind for something 20 commits or more into the past, you can just clear that memory.

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 4, 2017

Contributor

@kuychaco upon further thought I'm a bit skeptical of the reasons not to put the button next to Stage All:

  • The buttons are squished when narrow: Seems like there could be good solutions to that. Change Unstaged Changes to Unstaged at very narrow widths buys you a ton of pixels.
  • Might click it accidentally: It's true that any button we render can be accidentally clicked, but I think rendering this button conditionally in a way that shares real estate with unstaged files makes accidentally clicking more likely. Maybe moving it to the bottom helps, but I think the fundamental issue is the sharing of real estate.

I'd really like to at least see what it would look like to have the button in the header where it's position will be stable relative to all other elements. Would you be cool spending a little more time on this to see if we can get something closer to optimal?

@the-j0k3r You're definitely welcome in this conversation and thanks for the feedback. It's much easier for me to read now that your tone is more friendly. It seems like what would work best for you is being able to dismiss the undo button, but I'm super worried about the complexity it would add to the UI. I could see a context-menu option maybe. I really think if we put the button somewhere stable where you wouldn't accidentally click it that maybe this would be less of an issue. Getting a good design is sort of an exercise in saying "no" to a lot of features that would work for some people but degrade the experience for others. My instinct is that a dismissal affordance for undo falls into that category. I would love to find a consistent policy for dismissing the button automatically or making it easy to redo the undo.

Contributor

nathansobo commented Nov 4, 2017

@kuychaco upon further thought I'm a bit skeptical of the reasons not to put the button next to Stage All:

  • The buttons are squished when narrow: Seems like there could be good solutions to that. Change Unstaged Changes to Unstaged at very narrow widths buys you a ton of pixels.
  • Might click it accidentally: It's true that any button we render can be accidentally clicked, but I think rendering this button conditionally in a way that shares real estate with unstaged files makes accidentally clicking more likely. Maybe moving it to the bottom helps, but I think the fundamental issue is the sharing of real estate.

I'd really like to at least see what it would look like to have the button in the header where it's position will be stable relative to all other elements. Would you be cool spending a little more time on this to see if we can get something closer to optimal?

@the-j0k3r You're definitely welcome in this conversation and thanks for the feedback. It's much easier for me to read now that your tone is more friendly. It seems like what would work best for you is being able to dismiss the undo button, but I'm super worried about the complexity it would add to the UI. I could see a context-menu option maybe. I really think if we put the button somewhere stable where you wouldn't accidentally click it that maybe this would be less of an issue. Getting a good design is sort of an exercise in saying "no" to a lot of features that would work for some people but degrade the experience for others. My instinct is that a dismissal affordance for undo falls into that category. I would love to find a consistent policy for dismissing the button automatically or making it easy to redo the undo.

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 4, 2017

I understand your concerns about clutter and user experience,

I cant see a negative result from this, stumbled on multiple bugs, still unreported because of how inconsistent this Undo Discard button results once clicked are on my end. But here are some possibilities that dont necessarily clutter the UI.

Option 1) Context menu
option 1

Currently the context menu is a good candidate IMO, as you can see, the options are unrelated to the undo discard button (not great user experience there right?), but if the context menu was content aware, this would make a good interesting location for a Clear Undo Discard State entry.

Option 2)
Map Clear Undo Discard State to a key combination shortcut CTRL+somekey

Option 2 could be a standalone or an addition to Option 1

Option 3)
Built in timer (based on your comment) Make a global setting with a default preset time where Clear undo discard state after [xx hours] spinbox. Where a user entry of 00 hours would be equal to never display Undo Discard button or something.

Option 4) using a message dialogue
capture
Currently when you click Undo Discard and the action causes a conflict, that dialogue message pops up warning you and giving you options. Also the Cancel undo should clear this discard state, but it doesn't. (this is already a bug to me)

-> You can make a similar dialogue popup if the undo state is 1 hour old or more, and offer there an option to ➡️ Clear this undo discard state

And Im sure other people would have visualised other possibilities by now. in any case.

It's much easier for me to read now that your tone is more friendly.

@nathansobo Your initial reply tone left a great deal to be desired, I intended to dismiss that because of language barriers and other factors, but maybe subconsciously my ire showed through anyway.
I tried to dismiss it since, but you keep bringing it up so clearly there's a problem. Perhaps wallowing in this is not productive, so let me be the first to apologise and suggest we just move on, no hard feelings right?

the-j0k3r commented Nov 4, 2017

I understand your concerns about clutter and user experience,

I cant see a negative result from this, stumbled on multiple bugs, still unreported because of how inconsistent this Undo Discard button results once clicked are on my end. But here are some possibilities that dont necessarily clutter the UI.

Option 1) Context menu
option 1

Currently the context menu is a good candidate IMO, as you can see, the options are unrelated to the undo discard button (not great user experience there right?), but if the context menu was content aware, this would make a good interesting location for a Clear Undo Discard State entry.

Option 2)
Map Clear Undo Discard State to a key combination shortcut CTRL+somekey

Option 2 could be a standalone or an addition to Option 1

Option 3)
Built in timer (based on your comment) Make a global setting with a default preset time where Clear undo discard state after [xx hours] spinbox. Where a user entry of 00 hours would be equal to never display Undo Discard button or something.

Option 4) using a message dialogue
capture
Currently when you click Undo Discard and the action causes a conflict, that dialogue message pops up warning you and giving you options. Also the Cancel undo should clear this discard state, but it doesn't. (this is already a bug to me)

-> You can make a similar dialogue popup if the undo state is 1 hour old or more, and offer there an option to ➡️ Clear this undo discard state

And Im sure other people would have visualised other possibilities by now. in any case.

It's much easier for me to read now that your tone is more friendly.

@nathansobo Your initial reply tone left a great deal to be desired, I intended to dismiss that because of language barriers and other factors, but maybe subconsciously my ire showed through anyway.
I tried to dismiss it since, but you keep bringing it up so clearly there's a problem. Perhaps wallowing in this is not productive, so let me be the first to apologise and suggest we just move on, no hard feelings right?

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 4, 2017

Contributor

@the-j0k3r Re-reading my reply I can see how it came off as passive aggressive, which wasn't my intention. What I felt and wish I would have written is "Hey, thanks a lot for taking the effort to be friendly even though you're probably frustrated about accidentally clicking this button." So yeah, I think we're all on the same side here. Let's move forward.

Contributor

nathansobo commented Nov 4, 2017

@the-j0k3r Re-reading my reply I can see how it came off as passive aggressive, which wasn't my intention. What I felt and wish I would have written is "Hey, thanks a lot for taking the effort to be friendly even though you're probably frustrated about accidentally clicking this button." So yeah, I think we're all on the same side here. Let's move forward.

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 4, 2017

:D well.

My issue isnt with accidentally clicking this button, never was, my issue is like I tried explaining a few times, is that the button becomes visible once you discarded something and it lingers on and on and on. So one day after some 30 commits went past, I pressed it to see what would happen and low and behold it undid something 30 commits back.

How is that helpful to keep this undo state for so long?

the-j0k3r commented Nov 4, 2017

:D well.

My issue isnt with accidentally clicking this button, never was, my issue is like I tried explaining a few times, is that the button becomes visible once you discarded something and it lingers on and on and on. So one day after some 30 commits went past, I pressed it to see what would happen and low and behold it undid something 30 commits back.

How is that helpful to keep this undo state for so long?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 4, 2017

Member

I agree with @the-j0k3r. Past a point, the button feels useless and you start wondering what will happen if you click on it because you don't remember the last time you discarded something.

I think it's very useful for right after you discarded something, but after a commit I think its usefulness decreases rapidly. Maybe at that point it would make sense to hide the Discard button, especially because you can still undo your discard through the Command Palette.

Member

50Wliu commented Nov 4, 2017

I agree with @the-j0k3r. Past a point, the button feels useless and you start wondering what will happen if you click on it because you don't remember the last time you discarded something.

I think it's very useful for right after you discarded something, but after a commit I think its usefulness decreases rapidly. Maybe at that point it would make sense to hide the Discard button, especially because you can still undo your discard through the Command Palette.

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Nov 6, 2017

Member

I appreciate all of the productive discussion that has unfolded here. Great ideas have come up and I'll try to summarize and propose a comprehensive solution. Thoughts are welcome.

Main concerns:

  • Cluttering the UI
  • Accidental clicking of the Undo button
  • Undo button doing something unwanted.

All of these are a bit inter-related.

Placement

I'd really like to at least see what it would look like to have the button in the header where it's position will be stable relative to all other elements.

Sure thing @nathansobo 👍 . I like your idea of shortening Unstaged Changes to Unstaged. We could also hide the text in the buttons as well and just show the icons.

Unintentional Undos

@the-j0k3r and @50Wliu I hear you (and others) that after a certain point the button becomes useless or even harmful if clicked. Removing the button after committing or after a certain amount of time are very reasonable solutions, but I'm nervous about the few times a user might find themselves in a situation where they actually want to undo after we've removed the button. I know of at least one case where the undo button is used to stash some changes with the intention of restoring them after committing. Given that the nature of the button is to guard against data loss, I feel inclined to err on the side of giving users access to all of their data. I understand that this comes with tradeoffs and the potential for unintentionally undoing the last discard and ending up with messed up data, which is certainly problematic. We can do more to mitigate this.

To address the issue of unintentional undos, there are two fronts we can look at. We can make it harder or impossible to unintentionally click the button, or we can add safety guards after the button has been clicked. I like the idea of doing the latter regardless as long as it doesn't feel bothersome.

Hiding the button after committing and still allowing the user to undo via the command palette sounds reasonable, however I'm concerned about discoverability. I don't think it'll be clear enough that data can be recovered that way. What about adding an option to hide the button after committing that people have to opt into? And in the description we say that you can still undo via the command palette. It would default to the current behavior, but for those who are bothered by the button's presence there's a way to make it go away, and learn of the fact that the undo command exists.

Adding prompt after hitting "Undo" as @the-j0k3r suggested (Option 4) would be a great safety guard. I'm imagining that we show it if the user has committed since the last discard and we give the user the option to "Confirm Undo", "Open in a new file" to preview changes, and "Cancel undo". I have a small concern that this prompting could be annoying to some, but it pales in comparison to my concern that people end up with a messed up working directory.

Clearing Undo History State

Clearing the undo history seems heavy-handed to me and I'm concerned about that one time the data is actually needed. Maybe I'm more paranoid than is reasonable about this and I'm open to shifting my viewpoint to a less conservative one. In my mind if we can adequately protect the user from accidentally undoing a discard then keeping this discard history around is harmless.

I'm open to adding a command to clear discard history (Option 2), but not sure if it's really warranted. Something to note is that there are two distinct undo histories - one for the discards done in the Git panel, and one for the discards done in a diff view for a file. Personally I don't think clearing either of them is necessary.

Happy to hear thoughts! 💭

Member

kuychaco commented Nov 6, 2017

I appreciate all of the productive discussion that has unfolded here. Great ideas have come up and I'll try to summarize and propose a comprehensive solution. Thoughts are welcome.

Main concerns:

  • Cluttering the UI
  • Accidental clicking of the Undo button
  • Undo button doing something unwanted.

All of these are a bit inter-related.

Placement

I'd really like to at least see what it would look like to have the button in the header where it's position will be stable relative to all other elements.

Sure thing @nathansobo 👍 . I like your idea of shortening Unstaged Changes to Unstaged. We could also hide the text in the buttons as well and just show the icons.

Unintentional Undos

@the-j0k3r and @50Wliu I hear you (and others) that after a certain point the button becomes useless or even harmful if clicked. Removing the button after committing or after a certain amount of time are very reasonable solutions, but I'm nervous about the few times a user might find themselves in a situation where they actually want to undo after we've removed the button. I know of at least one case where the undo button is used to stash some changes with the intention of restoring them after committing. Given that the nature of the button is to guard against data loss, I feel inclined to err on the side of giving users access to all of their data. I understand that this comes with tradeoffs and the potential for unintentionally undoing the last discard and ending up with messed up data, which is certainly problematic. We can do more to mitigate this.

To address the issue of unintentional undos, there are two fronts we can look at. We can make it harder or impossible to unintentionally click the button, or we can add safety guards after the button has been clicked. I like the idea of doing the latter regardless as long as it doesn't feel bothersome.

Hiding the button after committing and still allowing the user to undo via the command palette sounds reasonable, however I'm concerned about discoverability. I don't think it'll be clear enough that data can be recovered that way. What about adding an option to hide the button after committing that people have to opt into? And in the description we say that you can still undo via the command palette. It would default to the current behavior, but for those who are bothered by the button's presence there's a way to make it go away, and learn of the fact that the undo command exists.

Adding prompt after hitting "Undo" as @the-j0k3r suggested (Option 4) would be a great safety guard. I'm imagining that we show it if the user has committed since the last discard and we give the user the option to "Confirm Undo", "Open in a new file" to preview changes, and "Cancel undo". I have a small concern that this prompting could be annoying to some, but it pales in comparison to my concern that people end up with a messed up working directory.

Clearing Undo History State

Clearing the undo history seems heavy-handed to me and I'm concerned about that one time the data is actually needed. Maybe I'm more paranoid than is reasonable about this and I'm open to shifting my viewpoint to a less conservative one. In my mind if we can adequately protect the user from accidentally undoing a discard then keeping this discard history around is harmless.

I'm open to adding a command to clear discard history (Option 2), but not sure if it's really warranted. Something to note is that there are two distinct undo histories - one for the discards done in the Git panel, and one for the discards done in a diff view for a file. Personally I don't think clearing either of them is necessary.

Happy to hear thoughts! 💭

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 6, 2017

Contributor

Adding prompt after hitting "Undo" as @the-j0k3r suggested (Option 4) would be a great safety guard. I'm imagining that we show it if the user has committed since the last discard and we give the user the option to "Confirm Undo", "Open in a new file" to preview changes, and "Cancel undo".

This sounds like a good solution to me as well. There are still some open questions for me if we go down this path however. It doesn't seem like restoring discard in a different file would always be possible, because sometimes the changes are to entire files. Like you might have discarded a new file, and now it's back and polluting the stage and you have to figure out what is new and what is old. It's tricky to "preview" the restored discard without adding stuff to the stage that you might find yourself needing to clean out.

Showing a "Redo Discard" button after clicking "Undo Discard" seems like it might be a nice solution. That's how we solve this problem when editing text... if you accidentally hit too many undos, you just hit redo until you're happy. I realize that's complicated though. Where do we put the button? How do we deal with the edge cases redoing discards?

Maybe for now just a warning like "You have committed since discarding, are you sure?" [Yes] [No] [Don't ask again]. Don't ask again could toggle a setting that the user could reenable if they regretted their choice.

Maybe one step beyond that would be some kind of explicit discard preview UI that doesn't modify the stage until you confirm. But that seems like a lot of work when a simple warning might solve 90% of cases.

Contributor

nathansobo commented Nov 6, 2017

Adding prompt after hitting "Undo" as @the-j0k3r suggested (Option 4) would be a great safety guard. I'm imagining that we show it if the user has committed since the last discard and we give the user the option to "Confirm Undo", "Open in a new file" to preview changes, and "Cancel undo".

This sounds like a good solution to me as well. There are still some open questions for me if we go down this path however. It doesn't seem like restoring discard in a different file would always be possible, because sometimes the changes are to entire files. Like you might have discarded a new file, and now it's back and polluting the stage and you have to figure out what is new and what is old. It's tricky to "preview" the restored discard without adding stuff to the stage that you might find yourself needing to clean out.

Showing a "Redo Discard" button after clicking "Undo Discard" seems like it might be a nice solution. That's how we solve this problem when editing text... if you accidentally hit too many undos, you just hit redo until you're happy. I realize that's complicated though. Where do we put the button? How do we deal with the edge cases redoing discards?

Maybe for now just a warning like "You have committed since discarding, are you sure?" [Yes] [No] [Don't ask again]. Don't ask again could toggle a setting that the user could reenable if they regretted their choice.

Maybe one step beyond that would be some kind of explicit discard preview UI that doesn't modify the stage until you confirm. But that seems like a lot of work when a simple warning might solve 90% of cases.

kuychaco added a commit that referenced this pull request Nov 6, 2017

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Nov 6, 2017

Member

Hiding the button after committing and still allowing the user to undo via the command palette sounds reasonable, however I'm concerned about discoverability. I don't think it'll be clear enough that data can be recovered that way. What about adding an option to hide the button after committing that people have to opt into?

I'm worried about adding more options. I'd wager that the amount of times someone needs to undo a discard after a commit is very low. Maybe a new FAQ entry on Discuss would be a better solution? I think that most people tend to ask on Discuss (or GitHub Issues) when they have questions and then we can point them to the FAQ.

Member

50Wliu commented Nov 6, 2017

Hiding the button after committing and still allowing the user to undo via the command palette sounds reasonable, however I'm concerned about discoverability. I don't think it'll be clear enough that data can be recovered that way. What about adding an option to hide the button after committing that people have to opt into?

I'm worried about adding more options. I'd wager that the amount of times someone needs to undo a discard after a commit is very low. Maybe a new FAQ entry on Discuss would be a better solution? I think that most people tend to ask on Discuss (or GitHub Issues) when they have questions and then we can point them to the FAQ.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Nov 6, 2017

Member

I think I'm 👎 in general for automatically clearing the button state after a commit. I myself use the across commits fairly often. I'm 👍 for automatically hiding the button upon commit, and making it available in the command palette, the right-click menu, or some new affordance (like the ... menu that @smashwilson uses in the merge conflict decorations). I agree with @50Wliu that I don't wanna fall into the trap of "just add an option."

I very much like the idea of tracking the commit that the discard was made at, and warning the user before undoing a discard if HEAD has moved, and love the idea of giving the user some idea of what's in the discard.

Member

BinaryMuse commented Nov 6, 2017

I think I'm 👎 in general for automatically clearing the button state after a commit. I myself use the across commits fairly often. I'm 👍 for automatically hiding the button upon commit, and making it available in the command palette, the right-click menu, or some new affordance (like the ... menu that @smashwilson uses in the merge conflict decorations). I agree with @50Wliu that I don't wanna fall into the trap of "just add an option."

I very much like the idea of tracking the commit that the discard was made at, and warning the user before undoing a discard if HEAD has moved, and love the idea of giving the user some idea of what's in the discard.

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 7, 2017

When you give someone the ability to clear something, it doesn't automatically mean that all other use cases are invalidated, like @BinaryMuse use case

but I'm nervous about the few times a user might find themselves in a situation where they actually want to undo after we've removed the button. I know of at least one case where the undo button is used to stash some changes with the intention of restoring them after committing. Given that the nature of the button is to guard against data loss, I feel inclined to err on the side of giving users access to all of their data.

That implies some short term action, but even where it doesn't (for the edge cases), just because you can can clear it or hide it or whatever the solution will come to be (if any) it doesn't mean you have to do do it, merely presenting an option, it does not mean you have to take it.

To the concerns of cluttering and potentially annoying users with dialogs giving them valid control over the data stored in the undo state...

You have many possibilities of how to best implement a way to mitigate the issue beyond my suggestions, each Im sure has its pros and cons, and I hear the concerns voiced over the discussion from all participants.

Option 99

From a user stand point, I would be happy if there was something to activate that allows me control, doesn't matter if that's invisible (advanced) and requires some hidden configuration option (off by default) that's documented in some FAQ. This suits Atom philosophy of being a hackable editor, doesn't mean this philosophy has to be restricted to main program, you could apply it to resolve N amount of issues where the UI cluttering is a concern with any of the configurable components, this is a win win, one size fits all solution.

Currently however there is no option to mitigate the issues described nor advanced nor visible in UI, and its seems straight forward solution that requires only offering the user a choice and control to suit their actual usage scenarios.

the-j0k3r commented Nov 7, 2017

When you give someone the ability to clear something, it doesn't automatically mean that all other use cases are invalidated, like @BinaryMuse use case

but I'm nervous about the few times a user might find themselves in a situation where they actually want to undo after we've removed the button. I know of at least one case where the undo button is used to stash some changes with the intention of restoring them after committing. Given that the nature of the button is to guard against data loss, I feel inclined to err on the side of giving users access to all of their data.

That implies some short term action, but even where it doesn't (for the edge cases), just because you can can clear it or hide it or whatever the solution will come to be (if any) it doesn't mean you have to do do it, merely presenting an option, it does not mean you have to take it.

To the concerns of cluttering and potentially annoying users with dialogs giving them valid control over the data stored in the undo state...

You have many possibilities of how to best implement a way to mitigate the issue beyond my suggestions, each Im sure has its pros and cons, and I hear the concerns voiced over the discussion from all participants.

Option 99

From a user stand point, I would be happy if there was something to activate that allows me control, doesn't matter if that's invisible (advanced) and requires some hidden configuration option (off by default) that's documented in some FAQ. This suits Atom philosophy of being a hackable editor, doesn't mean this philosophy has to be restricted to main program, you could apply it to resolve N amount of issues where the UI cluttering is a concern with any of the configurable components, this is a win win, one size fits all solution.

Currently however there is no option to mitigate the issues described nor advanced nor visible in UI, and its seems straight forward solution that requires only offering the user a choice and control to suit their actual usage scenarios.

@CadenP

This comment has been minimized.

Show comment
Hide comment
@CadenP

CadenP Nov 7, 2017

What if the discarded pieces were given a separate section like Unstaged and Staged have separate sections? Then each discard could be undone at will, just like each change can be discarded or staged at will.

For when the Discarded section would be unnecessary, maybe it could be hidden when empty, and maybe it could be allowed to collapse by clicking a toggle button (or just the header).

This way a Undo Discard button is no longer necessary, except if one wants a button similar to Stage All (perhaps Keep All, Retain All, or Preserve All), and in that case placement already has a precedent.

It would also be nice if there could be a preview for the discarded change in the same way as there are previews for staged and unstaged changes. This method would make a preview intuitive to locate.

Of course, there is probably a big difference in terms of implementation, where git takes care of staging/unstaging but not discarding/undiscarding unstaged/uncommitted changes. But this way might make things easier in terms of UI and UX.

CadenP commented Nov 7, 2017

What if the discarded pieces were given a separate section like Unstaged and Staged have separate sections? Then each discard could be undone at will, just like each change can be discarded or staged at will.

For when the Discarded section would be unnecessary, maybe it could be hidden when empty, and maybe it could be allowed to collapse by clicking a toggle button (or just the header).

This way a Undo Discard button is no longer necessary, except if one wants a button similar to Stage All (perhaps Keep All, Retain All, or Preserve All), and in that case placement already has a precedent.

It would also be nice if there could be a preview for the discarded change in the same way as there are previews for staged and unstaged changes. This method would make a preview intuitive to locate.

Of course, there is probably a big difference in terms of implementation, where git takes care of staging/unstaging but not discarding/undiscarding unstaged/uncommitted changes. But this way might make things easier in terms of UI and UX.

@BinaryMuse

This comment has been minimized.

Show comment
Hide comment
@BinaryMuse

BinaryMuse Nov 7, 2017

Member

@CadenP That's a good point. This sounds a lot like the Git stash (which we want to implement). We considered using it for the discard feature but didn't as reflogs are subject to cleanup (and the stash is just a reflog).

Member

BinaryMuse commented Nov 7, 2017

@CadenP That's a good point. This sounds a lot like the Git stash (which we want to implement). We considered using it for the discard feature but didn't as reflogs are subject to cleanup (and the stash is just a reflog).

@nathansobo

This comment has been minimized.

Show comment
Hide comment
@nathansobo

nathansobo Nov 7, 2017

Contributor

I think @CadenP is onto something. Not sure what the plan is for stashes but treating discards as an extension of the stash experience could be good. Sounds like a shorter term band-aid might still be in order though.

Contributor

nathansobo commented Nov 7, 2017

I think @CadenP is onto something. Not sure what the plan is for stashes but treating discards as an extension of the stash experience could be good. Sounds like a shorter term band-aid might still be in order though.

@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Nov 9, 2017

Sounds like a good alternative what @CadenP suggested.

Sounds like a shorter term band-aid might still be in order though.

In an ideal world sure, but as it is now its not causing any big issues, its annoying and gets to some confusing states, but no real big issues, I think devs that understand the complexities and handling have their hands full. But sure would be nice to get some band aid fix.

There are bugs associated with undo discard that are probably more pressing, I have to get around to reporting them now that 1.22 is out and see how that is working now.

Im glad this generated a discussion that's yielding healthy suggestions, 👍

the-j0k3r commented Nov 9, 2017

Sounds like a good alternative what @CadenP suggested.

Sounds like a shorter term band-aid might still be in order though.

In an ideal world sure, but as it is now its not causing any big issues, its annoying and gets to some confusing states, but no real big issues, I think devs that understand the complexities and handling have their hands full. But sure would be nice to get some band aid fix.

There are bugs associated with undo discard that are probably more pressing, I have to get around to reporting them now that 1.22 is out and see how that is working now.

Im glad this generated a discussion that's yielding healthy suggestions, 👍

@kuychaco

This comment has been minimized.

Show comment
Hide comment
@kuychaco

kuychaco Nov 10, 2017

Member

Im glad this generated a discussion that's yielding healthy suggestions, 👍

Agreed, lots of great ideas . I, too, like @CadenP's suggestions for a longer term plan to explore. For the short-term bandaid here's what I'm thinking:

  • move undo button to header because current location is prone to accidental clicks
  • if clicked and discarded changes were from prior to latest commit, do as @nathansobo suggests:

Maybe for now just a warning like "You have committed since discarding, are you sure?" [Yes] [No] [Don't ask again]. Don't ask again could toggle a setting that the user could reenable if they regretted their choice.

And for those with a strong preference for having the undo button disappear after committing, something like the following can be added to your init script:

atom.commands.add('atom-workspace', {
  'me:commit-plus': (event) => {
    atom.commands.dispatch(event.target, 'github:commit')
    // find "Undo Discard" button and remove it from the DOM
  }
})
Member

kuychaco commented Nov 10, 2017

Im glad this generated a discussion that's yielding healthy suggestions, 👍

Agreed, lots of great ideas . I, too, like @CadenP's suggestions for a longer term plan to explore. For the short-term bandaid here's what I'm thinking:

  • move undo button to header because current location is prone to accidental clicks
  • if clicked and discarded changes were from prior to latest commit, do as @nathansobo suggests:

Maybe for now just a warning like "You have committed since discarding, are you sure?" [Yes] [No] [Don't ask again]. Don't ask again could toggle a setting that the user could reenable if they regretted their choice.

And for those with a strong preference for having the undo button disappear after committing, something like the following can be added to your init script:

atom.commands.add('atom-workspace', {
  'me:commit-plus': (event) => {
    atom.commands.dispatch(event.target, 'github:commit')
    // find "Undo Discard" button and remove it from the DOM
  }
})
@the-j0k3r

This comment has been minimized.

Show comment
Hide comment
@the-j0k3r

the-j0k3r Feb 17, 2018

@kuychaco @CadenP and everyone else concerned, I think its appropriate to mention #605 applies to the discussion going here and so do the solutions/workplan suggested.

Perhaps it would be wiser to continue it there?

the-j0k3r commented Feb 17, 2018

@kuychaco @CadenP and everyone else concerned, I think its appropriate to mention #605 applies to the discussion going here and so do the solutions/workplan suggested.

Perhaps it would be wiser to continue it there?

@kuychaco kuychaco referenced this pull request Sep 25, 2018

Merged

Move "Undo Discard" button behind context menu #1702

9 of 12 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment