This is ready for review 😄

@ahuth Many thanks for the contribution, much appreciated and good work!

Add "Discard all changes" to as a top-level menu item under Branch

This menu item should be disabled if there are no files in the Changes list.

@shiftkey Are we always going to show a "are you sure?" dialog for discarding all files? The discard file option in Preferences is only applicable for single files.

d564b97, mac

Good call! The menu item is now disabled when:

• There are no files with changes
• It's in a rebase conflict state

@shiftkey Are we always going to show a "are you sure?" dialog for discarding all files? The discard file option in Preferences is only applicable for single files.

@tierninho @ahuth I interpret the Show confirmation dialog before discarding changes setting as being applicable to any sort of discard operation. So 👍 to showing it when all files are discarded.

I interpret the Show confirmation dialog before discarding changes setting as being applicable to any sort of discard operation. So 👍 to showing it when all files are discarded.

Actually, after reviewing the changes and digging into the history of this area it looks like we made a conscious decision to always show the confirmation dialog and always used Discard All Changes... for the menu label, so I'm 👍 to preserving the current behaviour (always showing the dialog) rather than changing it to be in sync with the preference as part of this PR.

@ahuth one little bit of cleanup we could do while we're in here is to make isDiscardingAllChanges a required parameter:

diff --git a/app/src/ui/changes/changes-list.tsx b/app/src/ui/changes/changes-list.tsx
index d34a44faa..ba5ebcf2f 100644
--- a/app/src/ui/changes/changes-list.tsx
+++ b/app/src/ui/changes/changes-list.tsx
@@ -109,7 +109,7 @@ interface IChangesListProps {
   readonly focusCommitMessage: boolean
   readonly onDiscardChangesFromFiles: (
     files: ReadonlyArray<WorkingDirectoryFileChange>,
-    isDiscardingAllChanges?: boolean
+    isDiscardingAllChanges: boolean
   ) => void
 
   /** Callback that fires on page scroll to pass the new scrollTop location */
diff --git a/app/src/ui/changes/sidebar.tsx b/app/src/ui/changes/sidebar.tsx
index 64f734e5a..2d0a4c3c6 100644
--- a/app/src/ui/changes/sidebar.tsx
+++ b/app/src/ui/changes/sidebar.tsx
@@ -234,7 +234,7 @@ export class ChangesSidebar extends React.Component<IChangesSidebarProps, {}> {
 
   private onDiscardChangesFromFiles = (
     files: ReadonlyArray<WorkingDirectoryFileChange>,
-    isDiscardingAllChanges: boolean = true
+    isDiscardingAllChanges: boolean
   ) => {
     this.props.dispatcher.showPopup({
       type: PopupType.ConfirmDiscardChanges,

@ahuth Thank you for the updated logic and patience when making the latest changes.

All looks good now ✨🚢

Thanks for the review/thoughts! I've made the isDiscardingAllChanges param cleanup.

Hmm, not sure what's up with the failing tests.

Locally I get yarn test:integration failing because of a timeout while waiting for the window to launch.

On CI, however, I think the build logs show it failed to install both PhantomJS and Electron. Weird.

@ahuth I believe that's an intermittent network issue - I've re-run the build to see if that addresses it

Thanks!

@ahuth Thanks so much for the contribution - this is great and I know you'll make a lot of people happy who have inadvertently clicked "Discard all changes." This should get merged in the next couple days, and it'll likely ship to beta later this week and to production in our next large release (probably the one after next so it has a bit of time on beta to make sure it's working correctly). Congrats on your first PR! ❤️

Gosh I miss this on the right-click menu on the top. While I agree this is more safe, this change feels to me like a real regression in UX, and I thought it was a bug. Perhaps I'm just an old-timer with this app. :-(

Regardless, thanks for the contribution @ahuth !

Thanks for the feedback @nycdotnet! If we see quite a bit of evidence from people similar to you who are missing that functionality, I think we have ideas for a reasonable solution to help improve things. So it's really helpful to hear how you're missing it, thanks again.

While I agree this is more safe, this change feels to me like a real regression in UX, and I thought it was a bug. Perhaps I'm just an old-timer with this app. :-(

@nycdotnet I think a decent first step to iterate on this would be to give it a keyboard shortcut for easier access. Please bump #7588 if you agree.

I don't even know how I first discovered this option - I think I just subconsciously knew it would be there, which doesn't exactly say "bad UX" to me.

I'm sad this command is now gone and moved to the menu bar which isn't anywhere near as ergonomic - this was a big part of my workflow (do some experimental change -> experiment fails -> discard-all and try again). I didn't find the command particularly unsafe as there's a huge "Are you really sure?" warning that pops up which AFAICT can't be disabled, and right-clicking "X changed files" to discard everything is a natural extension of right-clicking individual files to discard them.

A keyboard shortcut would help, but personally I'm kind of mouse-bound (which is why I want to use a GUI client in the first place); right-clicking things to discover new stuff comes more naturally than hunting through menus, so I tend to miss features if that's the only way to discover them.

Just as I had tried switching to the new client again too... oh well, back to the ancient Classic Desktop again.

@fatcerberus does the keyboard shortcut proposed in #7650 make it more accessible to you?

I’m thinking no, as it’s a two-handed command (or a very gymnastic one-hander 😄) so I would have to take my hand off the mouse; but my point above was that I don’t find keyboard shortcuts particularly discoverable in general compared to just poking around with the right mouse button.

@fatcerberus thanks for the extra context!

It's challenging to reconcile your scenario with the initial feedback about the context menu being confusing in #5342, but we'll keep this in mind.

I never found it particularly confusing - it just seemed natural that if I can right-click individual files to discard changes in them, I would be able to do the same to the “summary” above the list to discard everything.

The confusion in that issue from my quick skim looks like it’s more about the function of the checkboxes than the discard feature itself (though maybe I didn’t read closely enough); since selecting multiple files didn’t affect the diff view, I figured out pretty quickly that they were only there to determine what gets committed and had nothing to do with anything else going on in the UI.

I never found it particularly confusing - it just seemed natural that if I can right-click individual files to discard changes in them, I would be able to do the same to the “summary” above the list to discard everything.

Exactly this. The old UX felt very tactile/interactive.

Can we please bring back the header right click menu?

I used this many times a day. When right clicking on an item like a file, I expect to have options for that thing. When I see a count up top, I naturally right clicked thinking I'd have options for n thing (and I did!). I was pretty surprised this was removed. Does also having the right click menu there cause some issue? Selecting all the files to do an action or hunting in another place is not intuitive to me. It also puts a lot of distance between a global work-loss action and the count of things I'm about to execute it on. That context should be readily apparent. While it did say "Discard all changes" before, it's far away from context now, and I have to go mentally check if I want to really execute that dangerous action. When it was immediately beside the things it affects? Yeah sure, I know where I'm at and what's about to happen. I know that's minor, but I wanted to convey why that feels like a regression in and of itself.

For anyone else stumbling into this issue, please add your feedback to #7696