Make the TreeView respect the DoubleBuffered property #7403

0xC0000054 · 2022-07-13T11:28:13Z

Fixes #4883

Proposed changes

The TreeView will respect the DoubleBuffered property.

Customer Impact

Double buffering can be enabled in a derived class without needing to use P/Invoke.

Regression?

No

Risk

Probably minimal.

Test methodology

Manually tested the change in a winforms app

Test environment(s)

7.0.100-preview.5.22307.18 (x64)

Microsoft Reviewers: Open in CodeFlow

Fixes dotnet#4883

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

dreddy-work · 2022-07-19T00:47:17Z

@0xC0000054 , can you share gif of how it looks now after the fix?

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

dreddy-work · 2022-07-19T00:54:59Z

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

+
+            TVS_EX extendedStyles = (TVS_EX)User32.SendMessageW(this, (User32.WM)TVM.GETEXTENDEDSTYLE);
+
+            SetExtendedStyle(ref extendedStyles, TVS_EX.DOUBLEBUFFER, DoubleBuffered);


What are the default extended styles for TreeView here? why do we have to negate when DoubleBuffered is not enabled? It would default back to current behavior. Did you notice any change in behavior here?

The default extended styles appeared to be 0 (None) when I was debugging.

But now that I think about it the code is incorrect and would not clear the style anyway, the first SendMessage parameter is a mask.

I removed the GETEXTENDEDSTYLE call and the negation code.
Negating the extended styles would have been a regression for callers that set the values themselves.

Callers may override the TreeView OnHandleCreated method and set their own extended styles.

0xC0000054 · 2022-07-19T02:54:18Z

can you share gif of how it looks now after the fix?

The TreeView on the left uses the code in this PR and sets the DoubleBufferered property to true.
The TreeView on the right overrides the OnHandleCreated method to set the TVS_EX_DOUBLEBUFFER style.

The test project is attached, it is based on the minimal repo posted in the linked issue.
TreeViewFlickering.zip

RussKie

LGTM

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

RussKie · 2022-07-19T06:22:04Z

/cc: @kirsan31

kirsan31 · 2022-07-19T07:52:24Z

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

+                extendedStyles |= TVS_EX.DOUBLEBUFFER;
+            }
+
+            if (extendedStyles != 0)


We need to be able to remove TVS_EX.DOUBLEBUFFER so I think this condition is wrong (redundant).

Ups, I miss a discussion above 🙄
Everything gets a bit complicated...

We need to be able to remove TVS_EX.DOUBLEBUFFER with DoubleBuffered = false; if we set it before with DoubleBuffered = true;

We need not to broke users code if they fixed the flickering by themselves...

So I think we need to introduce some flag for it. But it will be a breaking change for some ppl any way :(

I think you missed my update (edit above) on this :(
For example with this realization my workaround from #4883 will stop working...

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

kirsan31 · 2022-07-19T09:18:51Z

Just in case, so as not to miss, we still have an unresolved issue (possibility to break some users):
#7403 (comment)
#7403 (comment)

This stops the TVS_EX.DOUBLEBUFFER style from being removed unless the calling code previously set the DoubleBuffered property to true. Unconditionally removing the TreeView extended styles would break any derived classes that set them using P/Invoke.

kirsan31 · 2022-07-19T10:12:40Z

👍
I think it's all we can do with DoubleBuffered here. Now we will break ppl only in very specific situations 😁

RussKie · 2022-07-19T10:17:51Z

Now we will break ppl only in very specific situations 😁

Can you think what these situations may be?

RussKie · 2022-07-19T10:18:50Z

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs

+            // Only set the TVS_EX_DOUBLEBUFFER style if the DoubleBuffered property setter has been executed.
+            // This stops the style from being removed for any derived classes that set it using P/Invoke.


RussKie

👍

kirsan31 · 2022-07-19T10:25:07Z

Can you think what these situations may be?

Only if some one for any reason will call DoubleBuffered = true; and then DoubleBuffered = false; before HandleCreated.

RussKie · 2022-07-19T10:46:05Z

Only if some one for any reason will call DoubleBuffered = true; and then DoubleBuffered = false; before HandleCreated.

In that case, UpdateTreeViewExtendedStyles would bail in the first return, no?
However, this sequence may result in unexpected behaviour before handle is created:

DoubleBuffered = true;
DoubleBuffered = false;
SendMessage(this.Handle, TVM_SETEXTENDEDSTYLE, (IntPtr)TVS_EX_DOUBLEBUFFER, (IntPtr)TVS_EX_DOUBLEBUFFER);

kirsan31 · 2022-07-19T11:00:23Z

In that case, UpdateTreeViewExtendedStyles would bail in the first return, no? However, this sequence may result in unexpected behaviour before handle is created:
DoubleBuffered = true;
DoubleBuffered = false;
SendMessage(this.Handle, TVM_SETEXTENDEDSTYLE, (IntPtr)TVS_EX_DOUBLEBUFFER, (IntPtr)TVS_EX_DOUBLEBUFFER);

Yes, of course, all this makes sense only if we called

SendMessage(this.Handle, TVM_SETEXTENDEDSTYLE, (IntPtr)TVS_EX_DOUBLEBUFFER, (IntPtr)TVS_EX_DOUBLEBUFFER);

before base.OnHandleCreated.

So, if user have this call before base.OnHandleCreated:

SendMessage(this.Handle, TVM_SETEXTENDEDSTYLE, (IntPtr)TVS_EX_DOUBLEBUFFER, (IntPtr)TVS_EX_DOUBLEBUFFER);

and this call before handle is created:

DoubleBuffered = true;

and this call before handle is created and after DoubleBuffered = true;:

DoubleBuffered = false;

We will break him.

dreddy-work · 2022-07-27T00:19:39Z

We are taking this and listen the feedback if we break any existing customers.

Make the TreeView respect the DoubleBuffered property

c36293f

Fixes dotnet#4883

0xC0000054 requested a review from a team as a code owner July 13, 2022 11:28

ghost assigned 0xC0000054 Jul 13, 2022

runfoapp bot mentioned this pull request Jul 13, 2022

Flaky System.Windows.Forms.UITests.ButtonTests.Button_Anchor_ResizeOnWindowSizeWiderAsync #7297

Closed

dreddy-work reviewed Jul 18, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs Show resolved Hide resolved

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Jul 18, 2022

Only update the extended styles if the value has changed

f4f00b3

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 19, 2022

dreddy-work reviewed Jul 19, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs Outdated Show resolved Hide resolved

dreddy-work reviewed Jul 19, 2022

View reviewed changes

dreddy-work added the 📭 waiting-author-feedback The team requires more information from the author label Jul 19, 2022

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 19, 2022

Do not change the styles that were set by external code

0de6fb0

Callers may override the TreeView OnHandleCreated method and set their own extended styles.

RussKie previously approved these changes Jul 19, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs Outdated Show resolved Hide resolved

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jul 19, 2022

kirsan31 reviewed Jul 19, 2022

View reviewed changes

src/System.Windows.Forms/src/System/Windows/Forms/TreeView.cs Outdated Show resolved Hide resolved

Allow the double buffering to be disabled

052d21a

0xC0000054 dismissed RussKie’s stale review via 052d21a July 19, 2022 08:20

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 19, 2022

Fix the method ordering

89db47d

RussKie reviewed Jul 19, 2022

View reviewed changes

RussKie approved these changes Jul 19, 2022

View reviewed changes

RussKie added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jul 20, 2022

dreddy-work merged commit 6379435 into dotnet:main Jul 27, 2022

ghost added this to the 7.0 RC1 milestone Jul 27, 2022

RussKie removed the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Jul 27, 2022

dotnet locked as resolved and limited conversation to collaborators Aug 26, 2022

0xC0000054 deleted the TreeView-double-buffer branch September 15, 2022 01:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the TreeView respect the DoubleBuffered property #7403

Make the TreeView respect the DoubleBuffered property #7403

0xC0000054 commented Jul 13, 2022 •

edited by ghost

dreddy-work commented Jul 19, 2022

dreddy-work Jul 19, 2022 •

edited

0xC0000054 Jul 19, 2022 •

edited

0xC0000054 Jul 19, 2022

0xC0000054 commented Jul 19, 2022

RussKie left a comment

RussKie commented Jul 19, 2022

kirsan31 Jul 19, 2022 •

edited

0xC0000054 Jul 19, 2022

kirsan31 Jul 19, 2022 •

edited

kirsan31 commented Jul 19, 2022

kirsan31 commented Jul 19, 2022

RussKie commented Jul 19, 2022

RussKie Jul 19, 2022

RussKie left a comment

kirsan31 commented Jul 19, 2022

RussKie commented Jul 19, 2022

kirsan31 commented Jul 19, 2022 •

edited

dreddy-work commented Jul 27, 2022


		TVS_EX extendedStyles = (TVS_EX)User32.SendMessageW(this, (User32.WM)TVM.GETEXTENDEDSTYLE);

		SetExtendedStyle(ref extendedStyles, TVS_EX.DOUBLEBUFFER, DoubleBuffered);

		// Only set the TVS_EX_DOUBLEBUFFER style if the DoubleBuffered property setter has been executed.
		// This stops the style from being removed for any derived classes that set it using P/Invoke.

Make the TreeView respect the DoubleBuffered property #7403

Make the TreeView respect the DoubleBuffered property #7403

Conversation

0xC0000054 commented Jul 13, 2022 • edited by ghost

Proposed changes

Customer Impact

Regression?

Risk

Test methodology

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

dreddy-work commented Jul 19, 2022

dreddy-work Jul 19, 2022 • edited

Choose a reason for hiding this comment

0xC0000054 Jul 19, 2022 • edited

Choose a reason for hiding this comment

0xC0000054 Jul 19, 2022

Choose a reason for hiding this comment

0xC0000054 commented Jul 19, 2022

RussKie left a comment

Choose a reason for hiding this comment

RussKie commented Jul 19, 2022

kirsan31 Jul 19, 2022 • edited

Choose a reason for hiding this comment

0xC0000054 Jul 19, 2022

Choose a reason for hiding this comment

kirsan31 Jul 19, 2022 • edited

Choose a reason for hiding this comment

kirsan31 commented Jul 19, 2022

kirsan31 commented Jul 19, 2022

RussKie commented Jul 19, 2022

RussKie Jul 19, 2022

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

kirsan31 commented Jul 19, 2022

RussKie commented Jul 19, 2022

kirsan31 commented Jul 19, 2022 • edited

dreddy-work commented Jul 27, 2022

0xC0000054 commented Jul 13, 2022 •

edited by ghost

dreddy-work Jul 19, 2022 •

edited

0xC0000054 Jul 19, 2022 •

edited

kirsan31 Jul 19, 2022 •

edited

kirsan31 Jul 19, 2022 •

edited

kirsan31 commented Jul 19, 2022 •

edited