Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx when dropdown menu is opened/closed #10260

TanukiSharp · 2022-10-15T03:40:41Z

This PR does:

remove changes made in PR Fixed superproject, stash and pull ToolStripSplitButton closing issue #3833, which was shitty, let's be honest
adds a suspend/resume of redraw of the ToolTipEx control when a child element of type ToolStripDropDownItem opens/closes a dropdown menus

See related issue #10254 (recent) and #3832 (old).

Previous PR as an attempt to fix this problem is #3833. It used to work but the fix was not satisfying (non-sense and dirty hack) and stopped being effective after a few versions of GE.

Also, see #10259 since this is my first PR from after 23 January 2019.

TanukiSharp · 2022-10-15T03:45:12Z

I think that CodeFactor issue at GitUI\CommandsDialogs\FormBrowse.cs:2952-2960 is unrelated, but let me know if I can do something.

GitUI/UserControls/FilterToolBar.cs

gerhardol

+1
Seem to work for me.
One comment on code removed in toolbar.
The core review is for the added code in ToolStripEx.cs, more reviews needed there.

RussKie

Could you please add animations clearly showing the old and the new behaviours?

RussKie · 2022-10-16T03:28:02Z

GitUI/UserControls/ToolStripEx.cs

+        [DllImport("user32.dll")]
+        private static extern int SendMessage(IntPtr hwnd, int wmsg, bool wparam, int lparam);
+
+        private const int WM_SETREDRAW = 11;


All interop code resides in https://github.com/gitextensions/gitextensions/tree/master/GitUI/Interops. We already have definitions for SendMessageW:

gitextensions/GitUI/Interops/User32/SendMessageW.cs

Line 8 in e78a4e1

public static extern IntPtr SendMessageW(

I added NativeMethods.WM_SETREDRAW with XML documentation taken from MSDN and slightly modified.

I also added NativeMethods.TRUE and NativeMethods.FALSE, I wonder why they are not already present, let me know if you see a better way.

RussKie · 2022-10-16T03:29:22Z

GitUI/UserControls/ToolStripEx.cs

+        private static void SuspendDrawing(Control control)
+        {
+            if (control != null)
+            {
+                SendMessage(control.Handle, WM_SETREDRAW, false, 0);
+            }
+        }
+
+        private static void ResumeDrawing(Control control)
+        {
+            if (control != null)
+            {
+                SendMessage(control.Handle, WM_SETREDRAW, true, 0);
+                control.Refresh();
+            }
+        }


These two methods have single call site each, and should be inlined. There's no need to create more stack frames.

I understand you point of view, but since source code is for humans and not for computers, I think it is better to have short and well named methods than saving a stack frame.

Let me know if this is a blocker for you and I will change as requested, but I'd like to make sure before.

but since source code is for humans and not for computers,

There are many different schools of thought and opinions on the subject, but small methods which are only called from one place impose maintenance and mental tax too. (Not to mention potential have perf implications).

For the sake of discussion, suppose, I follow your argument, let's compare the proposed code:

private static void SuspendDrawing(Control control) { if (control != null) { NativeMethods.SendMessageW(control.Handle, NativeMethods.WM_SETREDRAW, NativeMethods.FALSE, IntPtr.Zero); } } private void SplitButton_DropDownOpening(object? sender, EventArgs e) { if (sender is ToolStripDropDownItem item) { SuspendDrawing(item.Owner); } }

...and an alternative:

private void SplitButton_DropDownOpening(object? sender, EventArgs e) { if (sender is ToolStripDropDownItem item && item.Owner is Control control) { NativeMethods.SendMessageW(control.Handle, NativeMethods.WM_SETREDRAW, NativeMethods.FALSE, IntPtr.Zero); } }

Could you please articulate why the first snippet is better? It's more verbose, it has multiple if conditions and, thus, more complex.

It is more verbose if you take it in its whole, but let's just take the entry points:

private void SplitButton_DropDownOpening(object? sender, EventArgs e) { if (sender is ToolStripDropDownItem item) { SuspendDrawing(item.Owner); } }

It reads like a book, reading this I know that entering the if, it will suspend redraws, and most of the time that's all I need to know. If I choose or need to know how redraws are suspended, I can decide to visit the SuspendDrawing() method. I can also decide to not spend time for that and move on. Also this entry point code is shorter than the second snippet.

With the follwoing:

private void SplitButton_DropDownOpening(object? sender, EventArgs e) { if (sender is ToolStripDropDownItem item && item.Owner is Control control) { NativeMethods.SendMessageW(control.Handle, NativeMethods.WM_SETREDRAW, NativeMethods.FALSE, IntPtr.Zero); } }

First, the if condition is longer and more complex, so it's harder, when reading the code quickly, to get a grasp of how and when the if will enter.

Second, the statement in the if barely tells what it does by itself, and one will probably have to search MSDN or else to understand what it really does. You could add a comment, but then that will make the code longer and therefore more verbose. Also, comment may go desync the day one decide to send another type of message for some reason but forget to update the comment. You may argue that it's also possible to change the message and forget to rename the method (in the case of first snippet), but it's less likely to happen when the method does just one single thing. I believe code beats comments, so I prefer to have a short and well named method that's self-describing, than obscure code commented somewhere.

One more thing, the day someone else needs that feature too, the code is already factorized (partly) and ready to use. With the second snippet, it's easy to move the content of the if and forget about also taking the if for null check. With the first snippet, moving the method to a dedicated class (could be moved to NativeMethods) is all that's to be done as it is already self-sufficient and self-describing.

As for performance, it is also a very important topic for me and I agree with you, but here we are not in a hot path at all, so trying to save a stack frame is not really something that should drive this decision, IMHO.

Thank you for the explanation.

I inlined the methods.

RussKie · 2022-10-16T03:29:51Z

GitUI/UserControls/ToolStripEx.cs

+
+        private void SplitButton_DropDownOpening(object? sender, EventArgs e)
+        {
+            if (sender is ToolStripDropDownItem item)


Suggested change

if (sender is ToolStripDropDownItem item)

if (sender is ToolStripDropDownItem item && item.Owner is not null)

ditto below

Let's assume we keep SuspendDrawing() and ResumeDrawing() not inlined (cf comment above), and since the first thing they do is to check for null, would this change still be necessary ?

Would this change still be necessary?

only if the functions are merged (I tend to keep them separate but YAGNI.)

I added this check since methods have been inlined.

TanukiSharp · 2022-10-18T09:14:53Z

@RussKie

Could you please add animations clearly showing the old and the new behaviours?

Here is a video with the current fix.

gitextensions-issue10254-3.mp4

To compare with previous fixes and previous state, have a look at videos posted in #10254

Sorry for the huge black strips, not an expert in video edition :/

…oolStripEx when dropdown menu is opened/closed

gerhardol

Fine for me with RussKie changesto be decided on (I would like to see some comments at least and wish there were an inline in C#, not just advisory, even if I expect that the compiler should be able to inline automatically here).

mstv

have not run yet

mstv · 2022-10-19T22:07:02Z

GitUI/UserControls/ToolStripEx.cs

+
+        private static void SuspendDrawing(Control control)
+        {
+            if (control != null)


Suggested change

if (control != null)

if (control is not null)

is our coding style.

(If there was more code, rather:

Suggested change

if (control != null)

if (control is null)

{

return;

}

)

Methods are gone (inlined) so this code is gone too.

mstv · 2022-10-19T22:07:11Z

GitUI/UserControls/ToolStripEx.cs

+
+        private static void ResumeDrawing(Control control)
+        {
+            if (control != null)


mstv · 2022-10-19T22:10:10Z

GitUI/UserControls/ToolStripEx.cs

+
+        private void SplitButton_DropDownOpening(object? sender, EventArgs e)
+        {
+            if (sender is ToolStripDropDownItem item)


Would this change still be necessary?

only if the functions are merged (I tend to keep them separate but YAGNI.)

The reasons for the verbosity explained, and I'll agree to disagree.

TanukiSharp · 2022-10-22T01:34:02Z

Based on @RussKie disagreement, I inlined the calls and added the proper item.Owner check. Also added comments. I think this should address all the remaining issues, but let me know if there's something else.

gerhardol

Include in 4.0

gerhardol · 2022-10-23T21:04:08Z

Merging tomorrow

…en dropdown menu is opened/closed (#10260) (cherry picked from commit 8cfd4a9)

RussKie · 2022-10-25T12:29:50Z

Thank you

ghost assigned TanukiSharp Oct 15, 2022

gerhardol reviewed Oct 15, 2022

View reviewed changes

GitUI/UserControls/FilterToolBar.cs Show resolved Hide resolved

gerhardol reviewed Oct 15, 2022

View reviewed changes

RussKie previously requested changes Oct 16, 2022

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 Oct 16, 2022

gerhardol added this to the 4.0.0 milestone Oct 16, 2022

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

TanukiSharp added 2 commits October 18, 2022 18:29

Removed changes of PR gitextensions#3833, suspend/resume redrawn of T…

a95a091

…oolStripEx when dropdown menu is opened/closed

Reused existing SendMEssageW, added TRUE, FALSE and WM_SETREDRAW message

a739ace

TanukiSharp force-pushed the fix-10254 branch from 94917c5 to a739ace Compare October 18, 2022 09:29

TanukiSharp added 2 commits October 18, 2022 18:38

Fixed merge conflict resolution mistake

0990f2a

Removed now unnecessary using

eb6feae

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Oct 18, 2022

gerhardol approved these changes Oct 18, 2022

View reviewed changes

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

mstv reviewed Oct 19, 2022

View reviewed changes

Inlined SuspendDrawing and ResumeDrawing methods

2386ed2

TanukiSharp force-pushed the fix-10254 branch from bd6abc9 to 2386ed2 Compare October 22, 2022 01:32

gerhardol approved these changes Oct 22, 2022

View reviewed changes

gerhardol merged commit 8cfd4a9 into gitextensions:master Oct 24, 2022

ghost modified the milestones: 4.0.0, vNext Oct 24, 2022

gerhardol modified the milestones: vNext, 4.0.0 Oct 24, 2022

gerhardol pushed a commit that referenced this pull request Oct 24, 2022

Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx wh…

810d67b

…en dropdown menu is opened/closed (#10260) (cherry picked from commit 8cfd4a9)

TanukiSharp deleted the fix-10254 branch October 24, 2022 22:50

gerhardol mentioned this pull request Nov 5, 2022

Fix annoying collapsing of dropdown menus from tool bar buttons #10254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx when dropdown menu is opened/closed #10260

Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx when dropdown menu is opened/closed #10260

TanukiSharp commented Oct 15, 2022 •

edited

TanukiSharp commented Oct 15, 2022

gerhardol left a comment

RussKie left a comment

RussKie Oct 16, 2022

TanukiSharp Oct 18, 2022 •

edited

RussKie Oct 16, 2022

TanukiSharp Oct 18, 2022 •

edited

RussKie Oct 18, 2022

TanukiSharp Oct 18, 2022 •

edited

RussKie Oct 21, 2022

TanukiSharp Oct 22, 2022

RussKie Oct 16, 2022

TanukiSharp Oct 18, 2022 •

edited

mstv Oct 19, 2022

TanukiSharp Oct 22, 2022

TanukiSharp commented Oct 18, 2022

gerhardol left a comment

mstv left a comment

mstv Oct 19, 2022

TanukiSharp Oct 22, 2022

mstv Oct 19, 2022

mstv Oct 19, 2022

TanukiSharp commented Oct 22, 2022 •

edited

gerhardol left a comment

gerhardol commented Oct 23, 2022

RussKie commented Oct 25, 2022 via email •

edited by ghost

	if (sender is ToolStripDropDownItem item)
	if (sender is ToolStripDropDownItem item && item.Owner is not null)

Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx when dropdown menu is opened/closed #10260

Removed changes of PR #3833, suspend/resume redrawn of ToolStripEx when dropdown menu is opened/closed #10260

Conversation

TanukiSharp commented Oct 15, 2022 • edited

TanukiSharp commented Oct 15, 2022

gerhardol left a comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanukiSharp Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanukiSharp Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanukiSharp Oct 18, 2022 • edited

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

TanukiSharp Oct 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TanukiSharp commented Oct 18, 2022

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

TanukiSharp commented Oct 22, 2022 • edited

gerhardol left a comment

Choose a reason for hiding this comment

gerhardol commented Oct 23, 2022

RussKie commented Oct 25, 2022 via email • edited by ghost

TanukiSharp commented Oct 15, 2022 •

edited

TanukiSharp Oct 18, 2022 •

edited

TanukiSharp Oct 18, 2022 •

edited

TanukiSharp Oct 18, 2022 •

edited

TanukiSharp Oct 18, 2022 •

edited

TanukiSharp commented Oct 22, 2022 •

edited

RussKie commented Oct 25, 2022 via email •

edited by ghost