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.

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

Add a new virtual property StatusStripBorder to ToolStripProfessional… #4739

Merged
merged 4 commits into from
Aug 16, 2021

Conversation

Balkoth
Copy link
Contributor

@Balkoth Balkoth commented Mar 29, 2021

Resolves #4643

Summary

Currently, if you want to theme your application with a ToolStripProfessionalRenderer, there is an issue with the StatusStrip border, as the drawing routine uses a hardcoded value for the border color. A new virtual property is added to the ProfessionalColorTable to customize that color.

@ghost ghost assigned Balkoth Mar 29, 2021
@dnfadmin
Copy link

dnfadmin commented Mar 29, 2021

CLA assistant check
All CLA requirements met.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👀

@RussKie RussKie added the waiting-review This item is waiting on review by one or more members of team label Apr 13, 2021
Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

We also need to see whether we can add a rendering test to assert correct colours are applied (e.g. something similar to ToolStrip_RendersBackgroundCorrectly).

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label Apr 15, 2021
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Apr 15, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 15, 2021
@RussKie
Copy link
Member

RussKie commented Apr 15, 2021

You can grab it here https://visualstudio.microsoft.com/vs/preview/ 😉

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Apr 20, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Apr 23, 2021

Is there an updated developer guide, what i have to have installed on my system in order to build this. The one here https://github.com/dotnet/winforms/blob/main/docs/developer-guide.md is outdated.

If i have to install all this preview stuff, i don't know if i can complete this. My machine is a production machine and i do not want to mess it up like this. What was once a simple change escalates way beyond my initial intent. Maybe someone at Microsoft with a working dev environment might pick this up...

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 23, 2021
@RussKie
Copy link
Member

RussKie commented Apr 23, 2021

The one here https://github.com/dotnet/winforms/blob/main/docs/developer-guide.md is outdated.

What are you finding in the guide that appears outdated?

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Apr 23, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Apr 23, 2021

Windows Forms requires the following workloads and components be selected when installing Visual Studio 2019 (16.8+):

Required Workloads:

.NET Desktop Development
Desktop development with C++

This does not match what you really need in order to build the project.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Apr 23, 2021
@RussKie
Copy link
Member

RussKie commented Jun 11, 2021

This does not match what you really need in order to build the project.

What isn't matching, and what's missing?

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jun 11, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Jun 11, 2021

You need preview versions of .NET an Visual Studio. I don't know what the exact versions are.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jun 11, 2021
@RussKie
Copy link
Member

RussKie commented Jun 13, 2021

.NET 6.0 requires VS 16.11.
Strictly speaking VS isn't not need to build from command line (I think it is needed for VC++ projects, but C++ isn't essential for your changes). Both build.cmd and restore.cmd download the required version of .NET 6.0 nightlies. The build procedure is described in the docs.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Jun 13, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 9, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Jul 9, 2021

So today i took the chance to give building winforms another try, and finally succeeded....
I did the changes mentioned here earlier, so feel free to review now.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

👍
We just need to add a test.

Please have a look at the docs and at ToolStrip_RendersBackgroundCorrectly, it should be reasonably straight forward to add a test to assert the border colour.

src/System.Windows.Forms/src/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved
@Balkoth
Copy link
Contributor Author

Balkoth commented Jul 14, 2021

Relocated the API changes, and added a rendering test for the border color.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

🚀 LGTM

@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

@Balkoth a friendly nudge. If you think it is ready, set it ready for review, so we can take it. The 6.0 branches are getting closed in few days.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@Balkoth Balkoth marked this pull request as ready for review August 16, 2021 04:46
@Balkoth Balkoth requested a review from a team as a code owner August 16, 2021 04:46
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

Please rebase and resolve the MC. Thanks

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Aug 16, 2021

Rebased. What is the MC and how do i resolve it?

@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

MC stands for "merge conflict". Looks like your rebase didn't quite work, the build is failing.

D:\workspace\_work\1\s\src\System.Windows.Forms\src\PublicAPI.Unshipped.txt(1,1): error RS0025: The symbol '~override System.Windows.Forms.TrackBar.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject' appears more than once in the public API files. [D:\workspace\_work\1\s\src\System.Windows.Forms\src\System.Windows.Forms.csproj]
##[error]src\System.Windows.Forms\src\PublicAPI.Unshipped.txt(1,1): error RS0025: (NETCORE_ENGINEERING_TELEMETRY=Build) The symbol '~override System.Windows.Forms.TrackBar.CreateAccessibilityInstance() -> System.Windows.Forms.AccessibleObject' appears more than once in the public API files.

Build FAILED.

@RussKie RussKie added the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Aug 16, 2021
@Balkoth
Copy link
Contributor Author

Balkoth commented Aug 16, 2021

Done.

@RussKie RussKie merged commit 1384898 into dotnet:main Aug 16, 2021
@ghost ghost added this to the 6.0 rc1 milestone Aug 16, 2021
@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

Thank you

@ghost ghost locked as resolved and limited conversation to collaborators Jan 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new virtual property StatusStripBorder to ToolStripProfessionalRenderer
3 participants