Sync advanced filter and filters toolbar #9580

RussKie · 2021-09-16T10:45:59Z

⚠️ Do not squash

Fixes #9553 (review)

Proposed changes

Update FormRevisionFilter design and only refresh the revision grid when filter is accepted (i.e. OK button is clicked)
A major clean up of all filter related functionality in RevisionGrid control, move all filter-related info into a dedicated filter class AdvancedFilterInfo
Remove 5e1352b and memory-only filters

Screenshots

Before

After

Test methodology

a lot of manual tests
unit tests (pending)

RussKie · 2021-09-16T10:53:51Z

Current status:

The Advanced Filter dialog is sync'ed to the Filters toolbar
The Filters toolbar is sync'ed to the Advanced Filter dialog
The revision grid filtering remains correct
Cleanup the filter-related functionality is the revision grid to remove dependency on AppSettings.ShowXxx switches, and only rely on AdvancedFilterInfo instead.

gerhardol · 2021-09-16T11:12:20Z

Remove 5e1352b and memory-only filters

How did you check that this is no longer needed?
It was not obvious to me that this could be removed when I looked at it.

#9445 has a few changes related to the filter too.

RussKie · 2021-09-19T10:26:32Z

I believe I got to the point where I'm generally happy with the code and how it behaves, so feel free take it for a spit. I'll start working on tests next.

Remove 5e1352b and memory-only filters

How did you check that this is no longer needed?
It was not obvious to me that this could be removed when I looked at it.

This functionality was added many years ago for msysgit 1.7.3.1, which dates back to 2010. From the commit message:

As of msysgit 1.7.3.1 git-rev-list requires its search arguments
(--author, --committer, --regex) to be encoded with exactly the same
encoding as the one used at commit time.
This causes problems under Windows, where command line arguments are
passed as WideChars. Git uses argv, which contains strings
recoded into 8-bit system codepage, and that means searching for strings
outside ASCII range gets crippled, unless commit messages in git
are encoded according to system codepage.

I'll install the latest msysgit and see if I can test it, but in general I see no compelling reason to support such old tech.

RussKie · 2021-09-19T11:12:32Z

Installed the "latest" msysgit 1.9.5 (which is ~6 years old), everything appears to be working as expected:

Besides, I don't think we do actually support it

gerhardol · 2021-09-19T18:24:14Z

How did you check that this is no longer needed?

Installed the "latest" msysgit 1.9.5 (which is ~6 years old), everything appears to be working as expected:

Msysgit 1.x evolved into Git-for-windows 2.x so there is to my knowledge nothing special about msysgit 1.7.
A referral to msysgit it should just be Windows specific behavior.

This causes problems under Windows, where command line arguments are
passed as WideChars. Git uses argv, which contains strings
recoded into 8-bit system codepage, and that means searching for strings
outside ASCII range gets crippled,

The behavior could well be changed too, but this should be investigated. Not sure what chars to test with

gerhardol

Briefly reviewed, no major comments.
I wish #9445 was merged first, some conflicts.
This PR simplifies some foolow ups in to #9445 mentioned in #7598

Briefly run

gerhardol · 2021-09-19T20:29:29Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

-            }
-
-            OnFilterChanged();
+            // TODO: there's an opportunity to reduce refreshes by tracking the current state


How did you see that requireRefresh is not required?
I tried to backtrack this briefly and it could be OK to remove.

It was always set to true, except in some odd scenario when we reset the filter before switching the repo (or something like that). I have reworked the whole structure and the flow of filters, and this flag is superficial. We always need to refresh the grid whenever a filter changes.

I added the TODO note here because we can optimise the flow, and possibly skip a refresh if we detect that the branch filter value hasn't changed. But this is a function of the revision grid, and not the caller.

gerhardol · 2021-09-19T20:31:36Z

GitUI/UserControls/RevisionGrid/RevisionGridControl.cs

        {
-            AppSettings.BranchFilterEnabled = !string.IsNullOrWhiteSpace(filter);
+            // TODO: clean up and move all internals to FilterInfo


Will this and similar TODO be added to this PR?

Some of them - yes, some of them I'll likely leave for another time.

RussKie · 2021-09-22T08:58:38Z

The behavior could well be changed too, but this should be investigated. Not sure what chars to test with

I've tested with Russian text. If this removal becomes an issue we'll hear about it.

gerhardol · 2021-09-22T22:48:12Z

The behavior could well be changed too, but this should be investigated. Not sure what chars to test with

I've tested with Russian text. If this removal becomes an issue we'll hear about it.

I guess we will have to test with something submitted in *nix and that has some special characters
It could take time before someone reports such an issue

RussKie · 2021-09-23T01:44:03Z

To me this would only reinforce the idea this functionality is superfluous, and there's no need to have it.

gerhardol

To me this would only reinforce the idea this functionality is superfluous, and there's no need to have it.

You can say the same about any minor functionality. There may well be some teams where this functionality is something they rely on.
The consequences must be investigated.

There are some functionality changes currently, like if unchecking a filter box, the value is forgotten (then the box is not needed).
Applying fixes in the rebase I am pushing on my rebase on #9445 (not finished yet)

pmiossec · 2021-09-23T22:27:25Z

You can say the same about any minor functionality. There may well be some teams where this functionality is something they rely on.
The consequences must be investigated.

Exactly 😆 https://xkcd.com/1172/

RussKie · 2021-09-23T22:49:25Z

🤣 I'm happy to investigate when there's a real use case.

gerhardol · 2021-09-24T21:38:59Z

I pushed some updates to my repo, fixing some regressions in the form handling.
I am sure you will have comments...

Plan to merge #9445 tomorrow

🤣 I'm happy to investigate when there's a real use case.

Use case:

Commit in both Windows and Unix
Use non ascii characters in the areas to search

gerhardol

With the changes in my branch, the behavior iof the filter is similar to know.
Basically, the changes in this branch was applied in the merge, the filterbehavior was restored in the first fixup and the functionality adapted in second fixup. (info is stale, I cannot force-push to your repo).
Not too much usage myself.

The in-memory handling need to be investigated....

RussKie · 2021-09-26T12:37:32Z

Use case:

Commit in both Windows and Unix

Use non ascii characters in the areas to search

The in-memory handling need to be investigated....

Do you have a sample repo to try? I don't have any non Windows VMs readily available to test.
I am quite content with removing functionality with unknown usefulness and break users in this case.

With the changes in my branch, the behavior iof the filter is similar to know.
Basically, the changes in this branch was applied in the merge, the filterbehavior was restored in the first fixup and the functionality adapted in second fixup. (info is stale, I cannot force-push to your repo).
Not too much usage myself.

I rebased stomping over your filter-related changes, then restored what looked relevant in e9ee2cb with some tweaks. Please have a look, feel free to push into my branch, if there's anything I missed.

gerhardol · 2021-09-26T19:36:50Z

Do you have a sample repo to try? I don't have any non Windows VMs readily available to test.
I am quite content with removing functionality with unknown usefulness and break users in this case.

There is a clear use case - not so important to myself. I will try to setup a separate repo

With the changes in my branch, the behavior iof the filter is similar to know.
Basically, the changes in this branch was applied in the merge, the filterbehavior was restored in the first fixup and the functionality adapted in second fixup. (info is stale, I cannot force-push to your repo).
Not too much usage myself.

I rebased stomping over your filter-related changes, then restored what looked relevant in e9ee2cb with some tweaks. Please have a look, feel free to push into my branch, if there's anything I missed.

The first of my commits with most changes and the fixes to the form behavior was missing reapplied.
Some changes in the other were incorrect, reapplied some changes.

UnitTests/GitUI.Tests/UserControls/FilterToolBarTests.cs

RussKie · 2021-10-08T00:59:58Z

Pushed to my repo

My fork is open, are you not able to push in to this branch?

gerhardol · 2021-10-08T06:46:20Z

Pushed to my repo

My fork is open, are you not able to push in to this branch?

Stale branch, not even force push
(I am puzzled how Git could get the merges wrong too.)

RussKie · 2021-10-08T11:09:13Z

MC after #9619
I had to fix merge manually for four files; FormBrowse*, RevisionGridControl
(reset to this branch, cherry picked changes in master)
Pushed to my repo

Looking at your fork it seems that you took an outdated copy (I switched the order of commits). The MC was fixed with a auto-fix by the merge tool.

RussKie · 2021-10-08T11:13:21Z

GitUI/UserControls/RevisionGrid/FormRevisionFilter.cs

-            EnableFilters();
+            base.OnLoad(e);
+
+            FilterInfo rawFilterInfo = _filterInfo with { IsRaw = true };


@gerhardol .NET records make it really simple yet allow retaining the encapsulation.

RussKie · 2021-10-10T14:35:25Z

All green

RussKie · 2021-10-19T11:15:12Z

@gerhardol is there anything else outstanding?

gerhardol · 2021-10-19T13:11:46Z

@gerhardol is there anything else outstanding?

Filters, are not tested

There are also some git commands executed when starting with the dash board, that may occur due to my patches.

I have been busy the last weeks.

gerhardol

Git errors at startup
Occurs in my rebase on master, not in this PR. This is when starting to the dashboard and BuildIntegration is configured.
This need to be checked after rebase though.
Search
I can search when adding commits in Windows vs Linux (WSL) using 8 bit characters like åäöñ.
That indicates the functionality is good enough for me, encoding in WSL should be done the same in Linux/Windows now. (Changed in Git 2?).

mstv

nothing critical, have not run it yet but from now on

GitUI/CommandsDialogs/FormBrowse.cs

mstv · 2021-10-28T20:57:03Z

GitUI/CommandsDialogs/FormBrowse.cs

            revisionDiff.FallbackFollowedFile = pathFilter;
            fileTree.FallbackFollowedFile = pathFilter;
-            Text = _appTitleGenerator.Generate(Module.WorkingDir, Module.IsValidGitWorkingDir(), branchSelect.Text, TranslatedStrings.NoBranch, RevisionGrid.GetPathFilter());
-            RevisionGrid.ForceRefreshRevisions();
+            RevisionGrid.SetAndApplyPathFilter(pathFilter.QuoteNE());


In FormFileHistory, .QuoteNE() has been removed. Should SetAndApplyPathFilter quote itself if necessary, shouldn't it?

Quotes should be set in RevDiff and RevFileTree, from that on the field is just text, can be manipulated in the form.

@mstv I haven't spent any time looking at FormFileHistory, so I can't really comment on its behaviours.
@gerhardol is quoting here necessary?

The path must be quoted in the Git command, but the form has just free text that may not be quoted automatically, the user may enter smarter queries.
The path need to be added when converted from Path to string to be added to the filter. Will look into that later.

GitUI/UserControls/FilterToolBar.cs

GitUI/UserControls/RevisionGrid/Columns/RevisionGraphColumnProvider.cs

GitUI/UserControls/RevisionGrid/FilterInfo.cs

IntegrationTests/UI.IntegrationTests/CommandsDialogs/FormBrowseTests.cs

mstv · 2021-10-28T22:12:52Z

With fd36da6, the MEF encounters an ambiguity with IWindowsJumpListManager:

Microsoft.VisualStudio.Composition.CompositionFailedException: Es wurde(n) 1 Export(e) mit dem Vertragsnamen "GitUI.IWindowsJumpListManager" erwartet, nach der Anwendung geltender Einschränkungen wurden jedoch 2 gefunden.
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExports(ImportDefinition importDefinition)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExports(String contractName, ImportCardinality cardinality, Type type, Type metadataViewType, IMetadataViewProvider& metadataViewProvider)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExports[T,TMetadataView](String contractName, ImportCardinality cardinality)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T,TMetadataView](String contractName)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T](String contractName)
   at Microsoft.VisualStudio.Composition.ExportProvider.GetExport[T]()
   at GitUIPluginInterfaces.ManagedExtensibility.GetExport[T]() in D:\Build\gitextensions3\Plugins\GitUIPluginInterfaces\ManagedExtensibility.cs:line 123
   at GitUI.CommandsDialogs.FormBrowse..ctor(GitUICommands commands, BrowseArguments args) in D:\Build\gitextensions3\GitUI\CommandsDialogs\FormBrowse.cs:line 262
   at GitUI.GitUICommands.StartBrowseDialog(IWin32Window owner, BrowseArguments args) in D:\Build\gitextensions3\GitUI\GitUICommands.cs:line 1131
   at GitUI.GitUICommands.RunBrowseCommand(IReadOnlyList`1 args) in D:\Build\gitextensions3\GitUI\GitUICommands.cs:line 1566
   at GitUI.GitUICommands.RunCommandBasedOnArgument(IReadOnlyList`1 args, IReadOnlyDictionary`2 arguments) in D:\Build\gitextensions3\GitUI\GitUICommands.cs:line 1420
   at GitUI.GitUICommands.RunCommand(IReadOnlyList`1 args) in D:\Build\gitextensions3\GitUI\GitUICommands.cs:line 1393
   at GitExtensions.Program.RunApplication() in D:\Build\gitextensions3\GitExtensions\Program.cs:line 191
   at GitExtensions.Program.Main() in D:\Build\gitextensions3\GitExtensions\Program.cs:line 93

This functionality was added for msysgit 1.7.3.1, which dates back to 2010. msysgit 1.x is archived as of 2015. Tested against the last known version 1.9.5 and everything worked correctly.

* Apply the same design as for other updated dialogs * Only apply changes when users clicks [OK] button * Remove all filtering logic Contributes to gitextensions#6183

RussKie · 2021-10-30T00:16:56Z

With fd36da6, the MEF encounters an ambiguity with IWindowsJumpListManager:

I'm not observing any issues. Try git clan -xdf and rebuild?

RussKie · 2021-10-30T00:30:07Z

Git errors at startup
Occurs in my rebase on master, not in this PR. This is when starting to the dashboard and BuildIntegration is configured.
This need to be checked after rebase though.

I am no observing any issues after the rebase. I have also copied the artifacts to a separate location and run from there - observed no errors.
Please check again.

Search
I can search when adding commits in Windows vs Linux (WSL) using 8 bit characters like åäöñ.
That indicates the functionality is good enough for me, encoding in WSL should be done the same in Linux/Windows now. (Changed in Git 2?).

👍

gerhardol · 2021-10-30T11:34:21Z

I am no observing any issues after the rebase. I have also copied the artifacts to a separate location and run from there - observed no errors. Please check again.

The repo objects loaded (and for build info tab) is not due to this PR and master has more failed command.

With fd36da6, the MEF encounters an ambiguity with IWindowsJumpListManager:

I'm not observing any issues. Try git clan -xdf and rebuild?

Some previous commit (or just when rebasing?) loaded the plugins twice, not now.

Have not looked at the tests but I would like to see this merged.

gerhardol

SplitterPositionsShouldBeRestored() fails too

UnitTests/GitUI.Tests/UserControls/FilterInfoTests.cs

A major clean up of all filter related functionality in RevisionGrid control. * Move all filter-related info into a dedicated filter class FilterInfo * Move all filter-related AppSettings from RevisionGrid into FilterInfo * Move calculations of RefFilterOptions from RevisionGrid into FilterInfo Clean up FilterToolbar to react to FilterChanged events, and update its state from the event payload. Relates to gitextensions#9553

RussKie · 2021-10-31T13:27:02Z

The test coverage is not ideal, but I think we can take it as is. I'll look into improving the test coverage later.

ghost assigned RussKie Sep 16, 2021

RussKie force-pushed the sync_advanced_filter branch from d6e3cbb to c1971fc Compare September 19, 2021 15:03

gerhardol reviewed Sep 19, 2021

View reviewed changes

RussKie force-pushed the sync_advanced_filter branch 2 times, most recently from 45c4d56 to 70f6f47 Compare September 22, 2021 13:13

RussKie marked this pull request as ready for review September 22, 2021 13:43

RussKie force-pushed the sync_advanced_filter branch from 38228b9 to 034af29 Compare September 22, 2021 13:52

gerhardol mentioned this pull request Sep 22, 2021

Blame/filter in Browse, replace formfilehistory #9445

Merged

gerhardol requested changes Sep 23, 2021

View reviewed changes

ghost added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 23, 2021

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Sep 23, 2021

gerhardol reviewed Sep 25, 2021

View reviewed changes

RussKie force-pushed the sync_advanced_filter branch from 034af29 to d6f25e6 Compare September 26, 2021 11:57

RussKie force-pushed the sync_advanced_filter branch 4 times, most recently from 1b294da to 8fe483a Compare October 3, 2021 07:42

gerhardol reviewed Oct 7, 2021

View reviewed changes

UnitTests/GitUI.Tests/UserControls/FilterToolBarTests.cs Show resolved Hide resolved

RussKie force-pushed the sync_advanced_filter branch from 71d5346 to 9733dca Compare October 8, 2021 11:04

RussKie commented Oct 8, 2021

View reviewed changes

RussKie force-pushed the sync_advanced_filter branch from 9733dca to 36431db Compare October 10, 2021 14:06

gerhardol approved these changes Oct 25, 2021

View reviewed changes

mstv reviewed Oct 28, 2021

View reviewed changes

RussKie added 2 commits October 30, 2021 10:40

Revert 5e1352b

4330f6d

This functionality was added for msysgit 1.7.3.1, which dates back to 2010. msysgit 1.x is archived as of 2015. Tested against the last known version 1.9.5 and everything worked correctly.

Update FormRevisionFilter

4a8ad8d

* Apply the same design as for other updated dialogs * Only apply changes when users clicks [OK] button * Remove all filtering logic Contributes to gitextensions#6183

RussKie force-pushed the sync_advanced_filter branch from 36431db to f0ed43e Compare October 30, 2021 00:16

gerhardol mentioned this pull request Oct 30, 2021

GitVersion: Remove support for obsolete versions #9680

Merged

gerhardol reviewed Oct 30, 2021

View reviewed changes

UnitTests/GitUI.Tests/UserControls/FilterInfoTests.cs Show resolved Hide resolved

RussKie force-pushed the sync_advanced_filter branch from 265783a to 0373c5d Compare October 31, 2021 13:05

RussKie force-pushed the sync_advanced_filter branch from 0373c5d to 90951db Compare October 31, 2021 13:25

RussKie merged commit 9a59d3d into gitextensions:master Oct 31, 2021

RussKie deleted the sync_advanced_filter branch October 31, 2021 13:34

ghost added this to the vNext milestone Oct 31, 2021

RussKie removed the hacktoberfest-accepted label Oct 31, 2021

pmiossec mentioned this pull request Nov 8, 2021

Filter (only) sometimes show a commit preceding commit containing the search text #9618

Closed

Sync advanced filter and filters toolbar #9580

Sync advanced filter and filters toolbar #9580

Conversation

RussKie commented Sep 16, 2021 • edited

Proposed changes

Screenshots

Before

After

Test methodology

RussKie commented Sep 16, 2021 • edited

Current status:

gerhardol commented Sep 16, 2021

RussKie commented Sep 19, 2021

RussKie commented Sep 19, 2021 • edited

gerhardol commented Sep 19, 2021

gerhardol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Sep 22, 2021

gerhardol commented Sep 22, 2021

RussKie commented Sep 23, 2021 via email • edited by ghost

gerhardol left a comment

Choose a reason for hiding this comment

pmiossec commented Sep 23, 2021

RussKie commented Sep 23, 2021 via email • edited by ghost

gerhardol commented Sep 24, 2021

gerhardol left a comment

Choose a reason for hiding this comment

RussKie commented Sep 26, 2021

gerhardol commented Sep 26, 2021

RussKie commented Oct 8, 2021

gerhardol commented Oct 8, 2021

RussKie commented Oct 8, 2021

Choose a reason for hiding this comment

RussKie commented Oct 10, 2021

RussKie commented Oct 19, 2021

gerhardol commented Oct 19, 2021

gerhardol left a comment

Choose a reason for hiding this comment

mstv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv commented Oct 28, 2021

RussKie commented Oct 30, 2021

RussKie commented Oct 30, 2021

gerhardol commented Oct 30, 2021

gerhardol left a comment

Choose a reason for hiding this comment

RussKie commented Oct 31, 2021

RussKie commented Sep 16, 2021 •

edited

RussKie commented Sep 16, 2021 •

edited

RussKie commented Sep 19, 2021 •

edited

RussKie commented Sep 23, 2021 via email •

edited by ghost

RussKie commented Sep 23, 2021 via email •

edited by ghost