-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tweak repos dropdown #10906
base: master
Are you sure you want to change the base?
Tweak repos dropdown #10906
Conversation
fd3d686
to
bace125
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX in the video 👍
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
/// <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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Control.FindForm()
which way more exactly does what the name promises.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FindForm
is only available for Control
type and its descendants, which this is not. This is component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation is a technical debt yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to dd2b46c, too. We have a Control
. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a
Control
Unfortunately, the form is not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refer to dd2b46c
This is adding an layer of indirection (more IL) but I can't see any real benefits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
GitUI/CommandsDialogs/Menus/WorkingDirectoryToolStripSplitButton.cs
Outdated
Show resolved
Hide resolved
👍 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.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Have not run, not reviewed in detail (got similar questions as mstv when looking at it).
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 |
Can you please share an animation? |
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. Side note - should maybe an issue for 4.1: The dashboard seem to require more time to open. |
Can you please share an animation of it's autoclosing? |
Is it autoclosing because you happen to move the mouse cursor over other toolbar elements? 10906.mp4 |
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. |
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): |
@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: 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. |
10906-2.mp4 |
Then I suggest we leave it at this, open a placeholder issue that we point other issues too. |
I realised I forgot to add an icon, then I'll merge it.
|
An idea for perhaps a dirty hack: maybe move the mouse to ensure it is
still on top of the drop-down....
|
I really dislike the idea of moving the mouse...
|
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....
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider 23ef217
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:
***@***.***>
|
a727fd5
to
e2cea06
Compare
It does. This generates a lot of IL and requires additional memory: Class with events vs Class with inheritance.
@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,
Could you please elaborate on this @gerhardol? |
The mouse follow the text box when resizing |
Potential 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 |
@RussKie What are the plans for this PR? |
I need to find time for it.
|
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 don't have the time at the moment. And I don't know exactly when I will have... |
The hack works for me An enhancement would be to list matches in the file system too. |
Time to resurrect this? |
Yes, I'd love to have this functionality. But there's only so much free time I have :( |
@RussKie Could you include this little improvement over mouse hack? I just discovered that the 1st commit introduce a frustrating new behavior/regression if the mouse don't go straight down: |
…ndalone component
e2cea06
to
a3ba9c9
Compare
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
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) |
Resolves #10810
Proposed changes
Screenshots
Before
After
In action:
10810.mp4
Translations:
Test methodology
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.