Tweak repos dropdown #10906

RussKie · 2023-04-22T13:19:20Z

Resolves #10810

Proposed changes

Extract the working directory / recent repositories dropdown as a standalone component
Add "Close (go to Dashboard) to repos dropdown
Allow to filter the recent repositories dropdown

Screenshots

Before

After

In action:

10810.mp4
Translations:

Test methodology

manual
automated TBD

Merge strategy

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

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

mstv

UX in the video 👍

GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs

mstv · 2023-04-22T18:02:08Z

GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs

+        /// <summary>
+        ///  Gets the form that is displaying the menu item.
+        /// </summary>
+        protected static Form? OwnerForm
+            => Form.ActiveForm ?? (Application.OpenForms.Count > 0 ? Application.OpenForms[0] : null);


Please use Control.FindForm() which way more exactly does what the name promises.

FindForm is only available for Control type and its descendants, which this is not. This is component.

The implementation is a technical debt yet.

I don't disagree, if we had DI we wouldn't have had these hacks... Related though OT - I have a working DI in master...RussKie:gitextensions:add_shell_take2 but struggle to plug the menu commands in...

Refer to dd2b46c, too. We have a Control. :)

We have a Control

Unfortunately, the form is not found.

Refer to dd2b46c

This is adding an layer of indirection (more IL) but I can't see any real benefits.

This is adding an layer of indirection (more IL) but I can't see any real benefits.

As I wrote

I have not sorted the members yet. Some functionality could be moved to / from the Implementation class, too.
I have kept the sequence in the file in order to ease diffing.

refer to 23ef217
There is almost no more null check necessary, no _use_property_instead_ and much less violation of the SRP.
The dropdown can be closed again by clicking the button.
OwnerForm renamed to ActiveOrOpenForm. This suffices since we do not need the actual owning form.

GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs

pmiossec · 2023-04-22T20:27:07Z

👍 because I was thinking for the first time about this exact feature yesterday 😅

I will try to have a look at the code the next days but I can't really guarantee that the review will be easy from my phone....

gerhardol

+1
Have not run, not reviewed in detail (got similar questions as mstv when looking at it).

gerhardol · 2023-04-23T20:37:18Z

When typing in the filter, the filter closes after entering the first character, filtering the list (but not selecting in the list).
So I cannot use the filter.

mstv · 2023-04-23T21:07:28Z

When typing in the filter, the filter closes after entering the first character, filtering the list (but not selecting in the list).

works for me

RussKie · 2023-04-23T23:52:48Z

When typing in the filter, the filter closes after entering the first character, filtering the list (but not selecting in the list).
So I cannot use the filter.

Can you please share an animation?

gerhardol · 2023-04-25T21:56:03Z

This occur if the list is close to be as long as the window. If I have 50 items in the list, I have the problem but not with 45 with this size of the app.
I would like to add 100 items in the list if it is searchable.
App is maximized, if I minimize it I got to search occasionally.

Side note - should maybe an issue for 4.1: The dashboard seem to require more time to open.
Edit: BindRepositories() requires 0:30+0:45, (some repos are evaluated twice).

RussKie · 2023-04-27T02:14:02Z

Can you please share an animation of it's autoclosing?

RussKie · 2023-04-27T02:21:27Z

Is it autoclosing because you happen to move the mouse cursor over other toolbar elements?

10906.mp4

gerhardol · 2023-04-27T21:09:22Z

Is it autoclosing because you happen to move the mouse cursor over other toolbar elements?

I recorded a gif but pasted an image only.

The problem is that when typing, the menu change size so if the mouse pointer is not in the new area, focus is lost.
You have guess where the resized menu will end up.

RussKie · 2023-04-27T23:38:11Z

I see what's happening, and it is, essentially, described in my earlier comment: #10906 (comment).

The dropdown is opened to the side with the mouse essentially hovering over the toolbar (frame: 50):

As you type, the dropdown is filtered and is getting moved under the split button... (frame: 51):

...leaving the mouse hovering the toolbar, which initiates the "close split button" sequence (frame: 59):

RussKie · 2023-04-28T08:16:06Z

@gerhardol I spent some time investigating how the issue you highlighted can be mitigated, but I don't think it's feasible.

Windows Forms doesn't allow to cancel closing of the dropdown:
https://github.com/dotnet/winforms/blob/3e18b18c/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripDropDownItem.cs#L411

Without this ability, the dropdown's close logic is inaccessible to us. When the dropdown portion and the filter textbox is above or below the toolbar area - everything is working as expected - i.e., the dropdown is getting filtered which leads to it being closed and reopened. However, if the filter textbox is within the toolbar area - when then dropdown is getting filtered, it gets closed and reopened, but the mouse remains within the toolbar area, and that initiates the "selection changed" sequence of events that closes the dropdown...

I tried to hack around this, trying to capture when the filter is being performed, and then reopen the dropdown. However, this breaks the normal flow of events when I click away from the dropdown wanting to close it, but it remains reopened until I manually clear the filter.

The only viable workaround for your usecase (and anyone else hitting this scenario) is to resize the app so that the filter portion is NOT within the toolbar area when shown.

RussKie · 2023-04-28T08:26:54Z

10906-2.mp4

gerhardol · 2023-04-28T08:49:11Z

I spent some time investigating how the issue you highlighted can be mitigated, but I don't think it's feasible.

Then I suggest we leave it at this, open a placeholder issue that we point other issues too.
You need to learn how to use this feature.

RussKie · 2023-04-28T09:25:11Z

I realised I forgot to add an icon, then I'll merge it.

pmiossec · 2023-04-28T09:28:32Z

An idea for perhaps a dirty hack: maybe move the mouse to ensure it is still on top of the drop-down....

RussKie · 2023-04-28T09:30:44Z

I really dislike the idea of moving the mouse...

pmiossec · 2023-04-28T13:33:54Z

As you want but I'm pretty sure it will never be noticed by the user (because he is using the keyboard at the moment) whereas the closing of the drop-down....

mstv

Please consider 23ef217

RussKie · 2023-04-29T07:53:40Z

Please consider 23ef217

<23ef217> I've looked at your suggestion again, thanks. But I am unconvinced it is something we should take. It introduces an internal layer of abstraction for a component that has a single use, and I don't expect anyone to pretty much ever venture inside once the implementation is committed. _If_ we were talking about a public API of a component that had a high(er) reuse then your proposal would be more warranted. Besides, the implementation is in line with other similar implementations (e.g., other toolbarex components or where we pass Func<IGitModule>), only here I decided to use purposefully "ugly" variable names to better communicate the purpose of those variables. Again, this decision isn't new or very inconsistent with another naming convention - `NO_TRANSLATE_*`...

…

On Sat, 29 Apr 2023 at 02:11, Michael Seibt ***@***.***> wrote: ***@***.**** requested changes on this pull request. Please consider 23ef217 <23ef217> — Reply to this email directly, view it on GitHub <#10906 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBTEXQWW5IM27GFD3LDRUTXDPT2BANCNFSM6AAAAAAXH22JIE> . You are receiving this because you were assigned.Message ID: ***@***.***>

RussKie · 2023-04-29T10:25:31Z

Side note, not related to this PR but preventing me to setting recents to a 200 to keep more than a week of recents repos: The list is displayed very slowly when all items cannot be included.

Can you please open a new issue so we can discuss and investigate?

RussKie · 2023-04-29T10:38:10Z

23ef217 does not add an additional layer.

It does. This generates a lot of IL and requires additional memory:

Class with events vs Class with inheritance.

avoids lots of null-checks all the time as in

@mstv you seem to be fixated on null checks, and I'm having difficulties understand the reason behind that. Runtime-wise those checks are very performant, and quite often can be elided by the compiler. As for the development, this type is nullable annotated, null! is a feature of the language and it is used in .NET codebases too.

Also, it does not seem to need the hack for the mouse pointer in e2cea06

Could you please elaborate on this @gerhardol?

gerhardol · 2023-04-29T10:44:50Z

Also, it does not seem to need the hack for the mouse pointer in e2cea06

Could you please elaborate on this @gerhardol?

The mouse follow the text box when resizing

mstv · 2023-04-30T10:16:12Z

Potential null values always alert me when reading the code. I have seen too many bugs. I get less triggers to think about error conditions from code with non-null instances and a clear object life-time.

            private readonly Func<GitUICommands> _getUICommands;

            private readonly RepositoryHistoryUIService _repositoryHistoryUIService;

            private readonly ToolStripMenuItem _tsmiCategorisedRepos;

is much more self-explaining than

        protected RepositoryHistoryUIService RepositoryHistoryUIService
            => _use_property_instead_repositoryHistoryUIService ?? throw new InvalidOperationException("The button is not initialized");

        protected GitUICommands UICommands
            => (_use_property_instead_getUICommands ?? throw new InvalidOperationException("The button is not initialized")).Invoke();

        private Func<GitUICommands>? _use_property_instead_getUICommands;

        private RepositoryHistoryUIService? _use_property_instead_repositoryHistoryUIService;

        private ToolStripMenuItem _tsmiCategorisedRepos = null!;

But that's not the only point. There is even no need to subclass the ToolStripSplitButton.
Aggregation should be preferred over inheritance / subclassing.
It is good that the .NET codebase considers code-size and performance optimizations. That it requires a special language feature to do this, may be acceptable. Its use outside the framework needs to have a very high benefit though IMO.

gerhardol · 2023-06-23T21:40:17Z

@RussKie What are the plans for this PR?

RussKie · 2023-06-23T22:33:04Z

I need to find time for it.

pmiossec · 2023-06-23T22:36:00Z

I have started to test it weeks ago (because I really like the idea) but the 1st test I made on the mouse hack was not working well.
I has started to investigate but had to stop temporary to work on it and I forgot even the existence of it 😕

I don't have the time at the moment. And I don't know exactly when I will have...

gerhardol · 2023-06-24T07:08:35Z

The hack works for me
mstv implementation works too

An enhancement would be to list matches in the file system too.

gerhardol · 2023-11-26T12:04:12Z

Time to resurrect this?
Still working fine, either with the hacks or mstv rewrite (have not tried the rewrite after .net8)

RussKie · 2023-11-26T23:09:17Z

Yes, I'd love to have this functionality. But there's only so much free time I have :(

pmiossec · 2024-05-21T11:03:02Z

@RussKie Could you include this little improvement over mouse hack?
98beaf7

I just discovered that the 1st commit introduce a frustrating new behavior/regression if the mouse don't go straight down:
If you hover an element of the toolbar, the dropdown disappear immediately (which is not the case in last master where the dropdown stay open):

pmiossec · 2024-05-21T11:15:02Z

Other regression:
the content of "Favorite repositories" is duplicated each times the dropdown is opened:

…ndalone component

Resolves gitextensions#10810

RussKie · 2024-05-24T02:34:31Z

@RussKie Could you include this little improvement over mouse hack? 98beaf7

I rebased this on top of the latest master. Feel free to push your fix directly in this branch.

RussKie · 2024-05-24T02:42:00Z

I extracted the first two commits as #11751 as those should be contentious-free. Once that PR is merged, I'll rebase this one.

* Place the mouse a little better (just under the filter textbox after filtering is effective) * Enable hack to be run everytimes list is unfiltered then filtered again

pmiossec · 2024-05-24T07:25:42Z

Other regression: the content of "Favorite repositories" is duplicated each times the dropdown is opened:

Fixed.

The content of "Favorite repositories" is not filtered. So maybe it's better to move the search box after the favorites (or filter also the favorites)

ghost assigned RussKie Apr 22, 2023

RussKie added this to the 4.1 milestone Apr 22, 2023

RussKie force-pushed the tweak_repos_dropdown branch from fd3d686 to bace125 Compare April 22, 2023 13:23

RussKie requested review from mstv, gerhardol and pmiossec April 22, 2023 14:38

mstv reviewed Apr 22, 2023

View reviewed changes

gerhardol reviewed Apr 22, 2023

View reviewed changes

RussKie linked an issue Apr 23, 2023 that may be closed by this pull request

Enhance the recent repo dropdown with search capabilities #10810

Open

mstv requested changes Apr 28, 2023

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 Apr 28, 2023

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

RussKie force-pushed the tweak_repos_dropdown branch from a727fd5 to e2cea06 Compare April 29, 2023 08:56

RussKie modified the milestones: 4.1, vNext Apr 30, 2023

RussKie changed the base branch from release/4.1 to master April 30, 2023 06:33

RussKie modified the milestones: 4.2, vNext Oct 7, 2023

RussKie marked this pull request as draft November 26, 2023 23:09

gerhardol mentioned this pull request May 20, 2024

Restructure repository #11744

Merged

RussKie added 4 commits May 24, 2024 12:31

Extract the working directory / recent repositories dropdown as a sta…

2dfbbcf

…ndalone component

Add "Close (go to Dashboard) to repos dropdown

20fc939

Allow to filter the recent repositories dropdown

d955805

Resolves gitextensions#10810

HACK: move the mouse if the DropDown is opened besides the split button

a3ba9c9

RussKie force-pushed the tweak_repos_dropdown branch from e2cea06 to a3ba9c9 Compare May 24, 2024 02:31

pmiossec mentioned this pull request May 24, 2024

Add "Close (go to Dashboard) to repos dropdown #11751

Open

pmiossec added 2 commits May 24, 2024 09:00

Improve mouse hack:

58441c6

* Place the mouse a little better (just under the filter textbox after filtering is effective) * Enable hack to be run everytimes list is unfiltered then filtered again

fix duplication of favorites

9bc6f86

Tweak repos dropdown #10906

Are you sure you want to change the base?

Tweak repos dropdown #10906

Conversation

RussKie commented Apr 22, 2023 • edited

Proposed changes

Screenshots

Before

After

Test methodology

Merge strategy

mstv left a comment

Choose a reason for hiding this comment

mstv Apr 22, 2023

Choose a reason for hiding this comment

RussKie Apr 23, 2023

Choose a reason for hiding this comment

mstv Apr 23, 2023

Choose a reason for hiding this comment

RussKie Apr 23, 2023 • edited

Choose a reason for hiding this comment

mstv Apr 23, 2023

Choose a reason for hiding this comment

mstv Apr 23, 2023

Choose a reason for hiding this comment

RussKie Apr 23, 2023

Choose a reason for hiding this comment

mstv Apr 24, 2023 • edited

Choose a reason for hiding this comment

pmiossec commented Apr 22, 2023

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol commented Apr 23, 2023

mstv commented Apr 23, 2023

RussKie commented Apr 23, 2023

gerhardol commented Apr 25, 2023 • edited

RussKie commented Apr 27, 2023

RussKie commented Apr 27, 2023

gerhardol commented Apr 27, 2023 • edited

RussKie commented Apr 27, 2023

RussKie commented Apr 28, 2023

RussKie commented Apr 28, 2023

gerhardol commented Apr 28, 2023

RussKie commented Apr 28, 2023 via email • edited by ghost

pmiossec commented Apr 28, 2023 via email • edited by ghost

RussKie commented Apr 28, 2023 via email • edited by ghost

pmiossec commented Apr 28, 2023 via email

mstv left a comment

Choose a reason for hiding this comment

RussKie commented Apr 29, 2023 via email

RussKie commented Apr 29, 2023 • edited

RussKie commented Apr 29, 2023

gerhardol commented Apr 29, 2023

mstv commented Apr 30, 2023

gerhardol commented Jun 23, 2023

RussKie commented Jun 23, 2023 via email • edited by ghost

pmiossec commented Jun 23, 2023

gerhardol commented Jun 24, 2023

gerhardol commented Nov 26, 2023

RussKie commented Nov 26, 2023

pmiossec commented May 21, 2024

pmiossec commented May 21, 2024

RussKie commented May 24, 2024

RussKie commented May 24, 2024

pmiossec commented May 24, 2024

RussKie commented Apr 22, 2023 •

edited

RussKie Apr 23, 2023 •

edited

mstv Apr 24, 2023 •

edited

gerhardol commented Apr 25, 2023 •

edited

gerhardol commented Apr 27, 2023 •

edited

RussKie commented Apr 28, 2023 via email •

edited by ghost

pmiossec commented Apr 28, 2023 via email •

edited by ghost

RussKie commented Apr 28, 2023 via email •

edited by ghost

RussKie commented Apr 29, 2023 •

edited

RussKie commented Jun 23, 2023 via email •

edited by ghost