StageAll and UnstageAll improvement when Filter active #8596 #8731

keco2 · 2021-01-08T16:44:14Z

Followup on #8596

Note: The first PR #8730 closed (because of I've pushed my changes on master instead of a separate branch and used merge-commit,...)

Proposed changes

update 2 misleading texts to display either "All" or "Filtered" words based on the filter:
- if the Filter is empty (i.e. inactive), use the "All" word
- otherwise use the "Filtered" word
change the icon on the StageAll button interactively based on the filefilter:
- if the Filter is empty (i.e. inactive), use the classic StageAll picture
- otherwise use another (new) picture with funnel
in addition to the above mentioned changes of StageAll button from "Stage and commit all unstaged files" may not stage all unstaged files #8596 there is a change related to UnstageAll button as well (as agreed in PR Improved text and icon on StageAll and commit when Filter active #8596 #8730):
- visual change (just like on StageAll button: icon and ToolTipText changes when Filter is active)
- behavioural change (to get it consistent with the StageAll button): if the Filter is not empty (i.e. it's active) and UnstageAll is clicked: unstage only those files matching the filter (instead of unstaging everything)

Screenshots

Before

After

StageAll (TooltipText/icon) - with filefilter:
Before:
After:

UnstageAll (TooltipText/icon) - with filefilter:
Before:
After:

OnCommitClick (message) - with filefilter:
Before:
After:

Test methodology

change of text in the MessageBox on Commit: Manual
change of ToolTipText on StageAll/UnstageAll buttons: Manual and IntegrationTest
change of StageAll/UnstageAll button-picture: Manual
change of UnstageAll behaviour: Manual and IntegrationTest

Test environment(s)

GIT 3.4.2.9999+
GIT 2.30.0.windows.1
Windows 10 20H2

✒️ I contribute this code under The Developer Certificate of Origin.

codecov · 2021-01-08T17:01:46Z

Codecov Report

Merging #8731 (81c4612) into master (892ce08) will increase coverage by 0.39%.
The diff coverage is 78.07%.

@@            Coverage Diff             @@
##           master    #8731      +/-   ##
==========================================
+ Coverage   55.74%   56.13%   +0.39%     
==========================================
  Files         900      900              
  Lines       64990    65103     +113     
  Branches    11739    11808      +69     
==========================================
+ Hits        36226    36545     +319     
+ Misses      25911    25650     -261     
- Partials     2853     2908      +55

Flag	Coverage Δ
production	`43.27% <92.10%> (+0.50%)`	⬆️
tests	`94.80% <71.05%> (-0.13%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

keco2 · 2021-01-08T17:03:34Z

Btw, the icon with funnel - I'm open to suggestions - if it is not that good. I've created just using VS picture editor. Maybe someone has idea how to do it better.

I've tried funnel on: top-left / bottom-left / bottom-right :

Currently the B (bottom-left) is implemented (I think the A is a no-go as the arrows are less visible so).

pmiossec · 2021-01-08T18:40:52Z

Currently the funnel-down is implemented (because I think the arrows are more visible so).

I think bottom right would be better. What do you think?

keco2 · 2021-01-08T19:02:56Z

I've updated the picture with that option. I still like the B version a bit more :) but the C is Ok to me too. Let's wait for further votes - if no more vote comes, I'll update it with C version! 🥇

RussKie

👍

Please provide your sign off as well.

GitUI/CommandsDialogs/FormCommit.cs

GitUI/UserControls/FileStatusList.cs

UnitTests/CommonTestUtils/ReferenceRepository.cs

IntegrationTests/UI.IntegrationTests/CommandsDialogs/FormCommitTests.cs

keco2 · 2021-01-11T15:01:26Z

Great, thank you for the review! I've pushed the changes!

Please provide your sign off as well.

Done. Separate PR.

Currently the funnel-down is implemented (because I think the arrows are more visible so).

I think bottom right would be better.

Bottom right commited as promised ;)

RussKie · 2021-01-12T08:43:30Z

@msftbot merge in 24 hours

ghost · 2021-01-12T08:43:33Z

Hello @RussKie!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Wed, 13 Jan 2021 08:43:32 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

gerhardol · 2021-01-12T23:05:47Z

GitUI/CommandsDialogs/FormCommit.Designer.cs

@@ -967,7 +967,7 @@ private void InitializeComponent()
            this.toolStageAllItem.ImageTransparentColor = System.Drawing.Color.Magenta;
            this.toolStageAllItem.Name = "toolStageAllItem";
            this.toolStageAllItem.Size = new System.Drawing.Size(23, 23);
-            this.toolStageAllItem.Text = "Stage All";


These should be removed here and made sure they are explicitly assigned in init or load
The text is translated twice now

Agree. Good point. I can fix it together with the other change below.

gerhardol · 2021-01-12T23:12:02Z

GitUI/UserControls/FileStatusList.cs

@@ -1534,13 +1537,25 @@ private void FileStatusList_Enter(object sender, EventArgs e)
        private readonly Subject<string> _filterSubject = new Subject<string>();
        [CanBeNull] private Regex _filter;
        private bool _filterVisible = false;
+        private bool _filterActive = false;


Why is this used?
OnFilterChanged() may fire the event more than once, is that so bad?
(these status variablles are hard to maintain, someon else reuses it for a slightly separate use case or add a parallel variable...)

It fires on every character added/removed. But you're right, it would be no problem here without it so I can remove that check. Or maybe just rename it to something more meaningful like _filterActiveLastState or _filterActiveCheck? (and sorry for my delayed answer)

Just to be sure - Which one do you prefer?

remove the unnecessary check? or

rename to something more meaningful?

I prefer maintainability, so removing the variable if there is no noticeable effect is what I prefer.

Review feedback on PR #8731 (StageAll,UnstageAll)

ghost assigned keco2 Jan 8, 2021

keco2 marked this pull request as draft January 8, 2021 17:26

keco2 force-pushed the feature/issue8596_stageall branch from 0e82fa3 to 5314665 Compare January 8, 2021 18:38

keco2 marked this pull request as ready for review January 8, 2021 19:06

RussKie added the 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed label Jan 10, 2021

RussKie requested changes Jan 10, 2021

View reviewed changes

ghost added 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity and removed 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity labels Jan 10, 2021

keco2 force-pushed the feature/issue8596_stageall branch from 5314665 to 1f48d67 Compare January 11, 2021 14:46

keco2 marked this pull request as draft January 11, 2021 15:09

keco2 force-pushed the feature/issue8596_stageall branch from 1f48d67 to a797a9c Compare January 11, 2021 17:52

StageAll and UnstageAll text and icon improvement gitextensions#8596

81c4612

keco2 force-pushed the feature/issue8596_stageall branch from a797a9c to 81c4612 Compare January 11, 2021 18:07

keco2 requested a review from RussKie January 11, 2021 18:32

keco2 marked this pull request as ready for review January 11, 2021 18:34

RussKie added 🖊️ status: cla signed and removed 📜 status: needs cla The author needs to update contributors.txt before reviewing or merging can proceed labels Jan 12, 2021

RussKie approved these changes Jan 12, 2021

View reviewed changes

ghost added the status: auto merge label Jan 12, 2021

gerhardol reviewed Jan 12, 2021

View reviewed changes

ghost merged commit 20dd691 into gitextensions:master Jan 13, 2021

ghost added this to the 4.0 milestone Jan 13, 2021

keco2 added a commit to keco2/gitextensions that referenced this pull request Jan 17, 2021

Review feedback on StageAll,UnstageAll gitextensions#8731

797f269

keco2 mentioned this pull request Jan 17, 2021

Review feedback on PR #8731 (StageAll,UnstageAll) #8763

Merged

RussKie added a commit that referenced this pull request Jan 18, 2021

Merge pull request #8763 from keco2/feature/issue8596_stageall

37e7501

Review feedback on PR #8731 (StageAll,UnstageAll)

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StageAll and UnstageAll improvement when Filter active #8596 #8731

StageAll and UnstageAll improvement when Filter active #8596 #8731

keco2 commented Jan 8, 2021

codecov bot commented Jan 8, 2021 •

edited

keco2 commented Jan 8, 2021 •

edited

pmiossec commented Jan 8, 2021

keco2 commented Jan 8, 2021

RussKie left a comment

keco2 commented Jan 11, 2021 •

edited

RussKie commented Jan 12, 2021

ghost commented Jan 12, 2021

gerhardol Jan 12, 2021

keco2 Jan 16, 2021

gerhardol Jan 12, 2021

keco2 Jan 16, 2021

keco2 Jan 17, 2021

gerhardol Jan 17, 2021

StageAll and UnstageAll improvement when Filter active #8596 #8731

StageAll and UnstageAll improvement when Filter active #8596 #8731

Conversation

keco2 commented Jan 8, 2021

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

codecov bot commented Jan 8, 2021 • edited

Codecov Report

keco2 commented Jan 8, 2021 • edited

pmiossec commented Jan 8, 2021

keco2 commented Jan 8, 2021

RussKie left a comment

Choose a reason for hiding this comment

keco2 commented Jan 11, 2021 • edited

RussKie commented Jan 12, 2021

ghost commented Jan 12, 2021

gerhardol Jan 12, 2021

Choose a reason for hiding this comment

keco2 Jan 16, 2021

Choose a reason for hiding this comment

gerhardol Jan 12, 2021

Choose a reason for hiding this comment

keco2 Jan 16, 2021

Choose a reason for hiding this comment

keco2 Jan 17, 2021

Choose a reason for hiding this comment

gerhardol Jan 17, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 8, 2021 •

edited

keco2 commented Jan 8, 2021 •

edited

keco2 commented Jan 11, 2021 •

edited