Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Recalculate the node position of the TreeView when DrawMode = TreeViewDrawMode.OwnerDrawText #12698

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Recalculate the node position of the TreeView when DrawMode = TreeViewDrawMode.OwnerDrawText #12698

wants to merge 2 commits into from

Conversation

LeafShi1
Copy link
Member

@LeafShi1 LeafShi1 commented Dec 31, 2024 •
edited by dotnet-policy-service bot
Loading

Fixes #12681

Root Cause

When treeView.DrawMode = TreeViewDrawMode.OwnerDrawText, the node's FillRectangle and FocusRectangle positions are calculated incorrectly

Proposed changes

  • Update function CustomDraw of the TreeView.cs, When RightToLeft == RightToLeft.Yes && RightToLeftLayout reverses the X drawing coordinates of FillRectangle and FocusRectangle

Customer Impact

  • When setting RightToLeft = Yes and RightToLeftLayout = True, the selected node box is fully rendered

Regression?

  • No

Risk

  • Minimal

Screenshots

Before

image

After

When setting RightToLeft = Yes, RightToLeftLayout = True and .DrawMode = TreeViewDrawMode.OwnerDrawText, the selected node box is fully rendered
image

Test methodology

  • Manually

Test environment(s)

  • .net 10.0.0-alpha.1.24628.1
Microsoft Reviewers: Open in CodeFlow

@LeafShi1 LeafShi1 requested a review from a team as a code owner December 31, 2024 08:05
@codecov Codecov
Copy link

codecov bot commented Dec 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 52 lines in your changes missing coverage. Please review.

Project coverage is 76.13409%. Comparing base (d1bbb32) to head (b4fc154).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main      #12698         +/-   ##
===================================================
- Coverage   76.13468%   76.13409%   -0.00060%     
===================================================
  Files           3242        3242                 
  Lines         642363      642410         +47     
  Branches       47271       47279          +8     
===================================================
+ Hits          489061      489093         +32     
- Misses        149751      149776         +25     
+ Partials        3551        3541         -10
Flag Coverage Δ
Debug 76.13409% <0.00000%> (-0.00060%) ⬇️
integration 18.11281% <0.00000%> (+0.01596%) ⬆️
production 50.08090% <0.00000%> (+0.00050%) ⬆️
test 96.97854% <ø> (+0.00197%) ⬆️
unit 47.48118% <0.00000%> (-0.03829%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@LeafShi1 LeafShi1 added the waiting-review This item is waiting on review by one or more members of team label Jan 3, 2025
@ricardobossan
Copy link
Member

I proceeded to test the PR and the issue seems to have been solved:

image

The code LGTM!

Tanya-Solyanik
Tanya-Solyanik reviewed Jan 7, 2025
View reviewed changes
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had you tested this change at different scaling factors and in different level nodes in a tree view? For example 3rd level child?
It will be helpful to add code comments that describe what the constants are for.

src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs Outdated Show resolved Hide resolved
src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs Outdated Show resolved Hide resolved
@Tanya-Solyanik Tanya-Solyanik added waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 7, 2025
@Tanya-Solyanik Tanya-Solyanik added waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) waiting-author-feedback The team requires more information from the author labels Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 8, 2025
@LeafShi1 LeafShi1 removed the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Jan 10, 2025
@Tanya-Solyanik Tanya-Solyanik added the waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Jan 16, 2025
Copilot
Copilot AI reviewed Jan 21, 2025
View reviewed changes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs:2907

  • [nitpick] The method name 'PreferredHeight' is not very descriptive. Consider renaming it to 'CalculatePreferredHeight'.
private int PreferredHeight

src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs:2921

  • [nitpick] The method name 'GetNodeHeight' is not very descriptive. Consider renaming it to 'CalculateNodeHeight'.
private int GetNodeHeight(TreeNode node)

src/System.Windows.Forms/src/System/Windows/Forms/Controls/TreeView/TreeView.cs:2741

  • Using 'goto' is generally considered bad practice. Consider refactoring the code to eliminate the need for 'goto'.
goto default;
@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Jan 22, 2025
@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback The team requires more information from the author label Jan 23, 2025
@LeafShi1 LeafShi1 force-pushed the Issue_12681_fix_recalculate_node_position branch from 13f6c73 to 3841b91 Compare January 23, 2025 06:47
@LeafShi1 LeafShi1 force-pushed the Issue_12681_fix_recalculate_node_position branch from 8ecf0b8 to c5f0f04 Compare January 26, 2025 01:47
@LeafShi1 LeafShi1 added waiting-review This item is waiting on review by one or more members of team and removed waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) labels Jan 27, 2025
@LeafShi1
Copy link
Member Author

No regressions found in private PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@Tanya-Solyanik Tanya-Solyanik Tanya-Solyanik left review comments

@ricardobossan ricardobossan Awaiting requested review from ricardobossan

@Epica3055 Epica3055 Awaiting requested review from Epica3055

At least 1 approving review is required to merge this pull request.

Assignees

@LeafShi1 LeafShi1

Labels
waiting-review This item is waiting on review by one or more members of team
Projects
None yet
Milestone
No milestone
3 participants
@LeafShi1 @ricardobossan @Tanya-Solyanik