Improve pinned repos handling #11779

pmiossec · 2024-06-04T13:12:23Z

Based on PR #11790 that should be merged first.

Add a icon to clearly see anchored repositories
Add an option to remove pinned repositories from recent repositories (to not have duplicate repositories and save space --and user brain CPU cycles--)
~~Remove "Maximum number of pinned recent repositories" because if user add a repository to the pinned list, it's to see it (otherwise he will remove it from the list)~~

Screenshots

Before

no "pinned" icons & repos duplicated

After

Settings:

Pinned repos with icon:

With pinned repos not deduplicated (as before):

With pinned repos deduplicated (so less repo lines displayed) :

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build 392712e
Git 2.45.0.windows.1
Microsoft Windows NT 10.0.22631.0
.NET 8.0.5
DPI 96dpi (no scaling)
Portable: False
Microsoft.WindowsDesktop.App Versions

    Microsoft.WindowsDesktop.App 6.0.26 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 6.0.30 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 7.0.19 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 8.0.5 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

I agree that the maintainer rebase merge this PR (if the commit message is clear).

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

mstv

🤔 As I complained about when the regarding config form was modified, the term "pinned" may apply for the dashboard. But it does not correctly describe the behavior of the repo dropdown - at least not for the mode I use:

(Yes, this is how it looks. And the single column is not resizable!)

In this dropdown, the first list is just the MRU entries (sorted by usage), the second list is - more or less - the list of all repos I use - in alphabetical order.

Deduplication really needs to optional. A regarding screenshot of the setting is missing in the PR description.

The "Maximum number of pinned recent repositories" needs to be kept as I do not pin repos and do not want to do it (as well as I do not know how to pin them except using the dashboard).

Failing tests need to be adapted.

pmiossec · 2024-06-05T10:19:12Z

(Yes, this is how it looks. And the single column is not resizable!)

This is due to the value 40 that you put in "Combobox minimum width" setting. I don't really know the goal of this setting and always untouched the default 0 value.

And investigating the code, that's the reason of your not readable display in #11768

pmiossec · 2024-06-12T13:28:44Z

🤔 As I complained about when the regarding config form was modified, the term "pinned" may apply for the dashboard. But it does not correctly describe the behavior of the repo dropdown - at least not for the mode I use:

In this dropdown, the first list is just the MRU entries (sorted by usage), the second list is - more or less - the list of all repos I use - in alphabetical order.

Deduplication really needs to optional. A regarding screenshot of the setting is missing in the PR description.

The "Maximum number of pinned recent repositories" needs to be kept as I do not pin repos and do not want to do it (as well as I do not know how to pin them except using the dashboard).

Failing tests need to be adapted.

2nd version a lot simplified that handle all your remarks...

mstv

"number of pinned repos" is the wrong term for this dropdown.
Have a look at the screenshot of deduplication in the PR description.

mstv · 2024-06-12T21:13:13Z

src/app/GitCommands/Settings/AppSettings.cs

+        public static bool HidePinnedFromRecentList
+        {
+            get => GetBool("HidePinnedFromRecentList", false);
+            set => SetBool("HidePinnedFromRecentList", value);
+        }
+


Please do it like ShowSearchCommit.

mstv · 2024-06-12T21:13:23Z

tests/app/UnitTests/GitCommands.Tests/Settings/AppSettingsTests.cs

@@ -331,6 +331,7 @@ static IEnumerable<(PropertyInfo property, object defaultValue, bool isNullable,
                yield return (properties[nameof(AppSettings.RecursiveSubmodules)], 1, false, false);
                yield return (properties[nameof(AppSettings.ShorteningRecentRepoPathStrategy)], ShorteningRecentRepoPathStrategy.None, false, false);
                yield return (properties[nameof(AppSettings.MaxPinnedRepositories)], 0, false, false);
+                yield return (properties[nameof(AppSettings.HidePinnedFromRecentList)], false, false, false);


pmiossec · 2024-06-13T08:09:47Z

"number of pinned repos" is the wrong term for this dropdown. Have a look at the screenshot of deduplication in the PR description.

Honestly, I was wrong at renaming MRU repositories to "Pinned repositories". So at the moment it is literally the good description but indeed it is not describing what it is doing. And I agree that the consequence is that now the settings lables are not good nor consistent.

So if you give me the green light, I will rename back the "pinned" to either "MRU repositories" or "top repositories".
Also, I don't know if we prefer to keep "Anchor" (like in "Anchor to recent repositories") or if we choose to switch to "Pin to recent repositories" so that it will be better aligned with the new icon displayed.

mstv · 2024-06-13T17:38:53Z

My 2ct: Yes, "top repositories" and "pin to ..." feel much clearer to me.

gerhardol · 2024-06-13T18:04:10Z

"top repositories" is better (major change to rename all variables/methods if that is done though)
top/recent makes sense.

I have a minor preference for anchor vs pin and am not voting for to have an icon for top repos at all

--
I usually have a short list with 5 top repos in MRU order and 50 in alphabetic order, including the top repos as I may not look there at all.
50 is too few to keep in the big list for me, I would like to have 500. But this requires the filtering RussKie started implementing, and to limit the size of the dropdown as larger than window is behaving very weird.

pmiossec · 2024-06-17T09:44:24Z

I have a minor preference for anchor vs pin and am not voting for to have an icon for top repos at all

To clarify a little, the icon is not "top" repos but for "Anchored" repositories. You can have anchored repos in top or recent repos so you can potentially see pin icon in both lists. If you don't anchor a repository from this "Recent repositories settings" lists (repos displayed in bold), you will never see this icon.

For me the goal of this pin icon is meet to goals:

letting the user to find much more quickly "anchored" repositories which are more likely the ones he want to open (because at the moment there are no ways for the user to see it)
it adds some kind of "landmarks" so it is for me easier to read this list and find a given repo.

I think it's better I create a new PR about doing the renaming (I already did the changes) and once merged we continue our discussion on this one because the label will be impacted.

to make the list shorter and more convenient for user

mstv · 2024-06-26T17:58:43Z

src/app/GitCommands/UserRepositoryHistory/RecentRepoInfo.cs

@@ -73,13 +77,16 @@ public void SplitRecentRepos(IList<Repository> repositories, List<RecentRepoInfo
            {
                bool mostRecent = (topRepos.Count < n && repository.Anchor == Repository.RepositoryAnchor.None) ||
                    repository.Anchor == Repository.RepositoryAnchor.AnchoredInTop;
-                RecentRepoInfo ri = new(repository, mostRecent);
+                RecentRepoInfo ri = new(repository, mostRecent, repository.Anchor == Repository.RepositoryAnchor.AnchoredInTop || repository.Anchor == Repository.RepositoryAnchor.AnchoredInRecent);


Suggested change

RecentRepoInfo ri = new(repository, mostRecent, repository.Anchor == Repository.RepositoryAnchor.AnchoredInTop || repository.Anchor == Repository.RepositoryAnchor.AnchoredInRecent);

RecentRepoInfo ri = new(repository, mostRecent, repository.Anchor is Repository.RepositoryAnchor.AnchoredInTop or Repository.RepositoryAnchor.AnchoredInRecent);

mstv · 2024-06-26T18:08:50Z

src/app/GitCommands/UserRepositoryHistory/RecentRepoInfo.cs

@@ -118,7 +125,7 @@ void AddSortedRepos(bool mostRecent, List<RecentRepoInfo> addToList)
                addToList.AddRange(
                    from caption in orderedRepos.Keys
                    from repo in orderedRepos[caption]
-                    where !mostRecent || repo.MostRecent == mostRecent
+                    where (!mostRecent && !HideTopRepositoriesFromRecentList) || repo.MostRecent == mostRecent


mostRecent and MostRecent should be renamed to TopRepo.

Suggested change

where (!mostRecent && !HideTopRepositoriesFromRecentList) || repo.MostRecent == mostRecent

where repo.TopRepo == topRepo || (!topRepo && !HideTopRepositoriesFromRecentList)

mstv · 2024-06-26T18:11:14Z

tests/app/UnitTests/GitCommands.Tests/Settings/AppSettingsTests.cs

@@ -332,6 +332,7 @@ static IEnumerable<(PropertyInfo property, object defaultValue, bool isNullable,
                yield return (properties[nameof(AppSettings.ShorteningRecentRepoPathStrategy)], ShorteningRecentRepoPathStrategy.None, false, false);
                yield return (properties[nameof(AppSettings.MaxTopRepositories)], 0, false, false);
                yield return (properties[nameof(AppSettings.RecentRepositoriesHistorySize)], 30, false, false);
+                yield return (properties[nameof(AppSettings.HideTopRepositoriesFromRecentList)], false, false, false);


Suggested change

yield return (properties[nameof(AppSettings.HideTopRepositoriesFromRecentList)], false, false, false);

yield return (properties[nameof(AppSettings.HideTopRepositoriesFromRecentList)], false, isNotNullable, isISetting);

mstv requested changes Jun 4, 2024

View reviewed changes

pmiossec mentioned this pull request Jun 5, 2024

Replace tiles in dashboard? #11768

Closed

This comment was marked as off-topic.

Sign in to view

pmiossec marked this pull request as draft June 7, 2024 15:16

pmiossec force-pushed the improve_pinned_repos_handling branch from 392712e to d186804 Compare June 12, 2024 12:51

pmiossec marked this pull request as ready for review June 12, 2024 13:28

pmiossec force-pushed the improve_pinned_repos_handling branch 2 times, most recently from 2efcbd7 to bf916d6 Compare June 12, 2024 13:56

mstv reviewed Jun 12, 2024

View reviewed changes

pmiossec marked this pull request as draft June 17, 2024 09:47

pmiossec mentioned this pull request Jun 17, 2024

RecentReposSettings: Streamline naming #11790

Merged

pmiossec force-pushed the improve_pinned_repos_handling branch 2 times, most recently from b007011 to 44f7cfe Compare June 17, 2024 11:28

pmiossec added 2 commits June 22, 2024 22:43

Add an icon in the menu for anchored repositories

4077ae5

RecentRepos: add new setting to "Hide top repos from recent list"

0479187

to make the list shorter and more convenient for user

pmiossec force-pushed the improve_pinned_repos_handling branch from 44f7cfe to 0479187 Compare June 22, 2024 20:44

mstv self-requested a review June 26, 2024 18:13

mstv reviewed Jun 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve pinned repos handling #11779

Improve pinned repos handling #11779

pmiossec commented Jun 4, 2024 •

edited

Loading

mstv left a comment •

edited

Loading

pmiossec commented Jun 5, 2024

This comment was marked as off-topic.

pmiossec commented Jun 12, 2024

mstv left a comment

mstv Jun 12, 2024

mstv Jun 12, 2024

pmiossec commented Jun 13, 2024

mstv commented Jun 13, 2024

gerhardol commented Jun 13, 2024

pmiossec commented Jun 17, 2024

mstv Jun 26, 2024

mstv Jun 26, 2024

mstv Jun 26, 2024

	RecentRepoInfo ri = new(repository, mostRecent, repository.Anchor == Repository.RepositoryAnchor.AnchoredInTop \|\| repository.Anchor == Repository.RepositoryAnchor.AnchoredInRecent);
	RecentRepoInfo ri = new(repository, mostRecent, repository.Anchor is Repository.RepositoryAnchor.AnchoredInTop or Repository.RepositoryAnchor.AnchoredInRecent);

	where (!mostRecent && !HideTopRepositoriesFromRecentList) \|\| repo.MostRecent == mostRecent
	where repo.TopRepo == topRepo \|\| (!topRepo && !HideTopRepositoriesFromRecentList)

	yield return (properties[nameof(AppSettings.HideTopRepositoriesFromRecentList)], false, false, false);
	yield return (properties[nameof(AppSettings.HideTopRepositoriesFromRecentList)], false, isNotNullable, isISetting);

Improve pinned repos handling #11779

Are you sure you want to change the base?

Improve pinned repos handling #11779

Conversation

pmiossec commented Jun 4, 2024 • edited Loading

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

mstv left a comment • edited Loading

Choose a reason for hiding this comment

pmiossec commented Jun 5, 2024

This comment was marked as off-topic.

pmiossec commented Jun 12, 2024

mstv left a comment

Choose a reason for hiding this comment

mstv Jun 12, 2024

Choose a reason for hiding this comment

mstv Jun 12, 2024

Choose a reason for hiding this comment

pmiossec commented Jun 13, 2024

mstv commented Jun 13, 2024

gerhardol commented Jun 13, 2024

pmiossec commented Jun 17, 2024

mstv Jun 26, 2024

Choose a reason for hiding this comment

mstv Jun 26, 2024

Choose a reason for hiding this comment

mstv Jun 26, 2024

Choose a reason for hiding this comment

pmiossec commented Jun 4, 2024 •

edited

Loading

mstv left a comment •

edited

Loading