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

Enable DarkMode Theming #10985

Closed
wants to merge 71 commits into from
Closed

Conversation

KlausLoeffelmann
Copy link
Member

@KlausLoeffelmann KlausLoeffelmann commented Mar 3, 2024

Fixes #7641.

Ready for another review pass. We're still aiming for RC1.
There is one test issue left, which will be addressed during next week.

@elachlan
Copy link
Contributor

elachlan commented Mar 4, 2024

Test failures are caused by white space analyzer errors.

##[error]src\System.Windows.Forms\src\System\Windows\Forms\Controls\ToolStrips\StatusStrip.cs(32,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md)
D:\a\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Controls\ToolStrips\ToolStripManager.cs(573,1): error SA1028: Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md) [D:\a\_work\1\s\src\System.Windows.Forms\src\System.Windows.Forms.csproj]
##[error]src\System.Windows.Forms\src\System\Windows\Forms\Controls\ToolStrips\ToolStripManager.cs(573,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md)
D:\a\_work\1\s\src\System.Windows.Forms\src\System\Windows\Forms\Controls\ToolStrips\ToolStripManager.cs(575,1): error SA1028: Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md) [D:\a\_work\1\s\src\System.Windows.Forms\src\System.Windows.Forms.csproj]
##[error]src\System.Windows.Forms\src\System\Windows\Forms\Controls\ToolStrips\ToolStripManager.cs(575,1): error SA1028: (NETCORE_ENGINEERING_TELEMETRY=Build) Code should not contain trailing whitespace (https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SA1028.md)

@KlausLoeffelmann KlausLoeffelmann changed the title [Proposal] Provide DarkMode Theming Enable DarkMode Theming Mar 6, 2024
@kirsan31
Copy link
Contributor

kirsan31 commented Mar 7, 2024

@KlausLoeffelmann
First of all - thank you 🙏

Problematic controls maybe MonthCalendar, DateTimePicker and definitely TabControl at this point.

I understand that I've already annoyed everyone with this question, but still... At this stage, what can you say about Theming and MDI (#3691)?

@KlausLoeffelmann
Copy link
Member Author

KlausLoeffelmann commented Mar 7, 2024

I understand that I've already annoyed everyone with this question, but still... At this stage, what can you say about Theming and MDI

That it's not at all likely that it will be addressed in the underlaying layer, which WinForms "just" wraps around. I know it's a disappointing condition, and we're affected ourselves: namely the Designer document (Form) Window. We might be able to brush-up the rendering of the Window title in the WinForms Designer, but it'll only be possible with NC-custom painting. That might work in this case, because the Window is pretty static. But I wouldn't touch it, to be honest, for moving parts.

@KlausLoeffelmann KlausLoeffelmann changed the base branch from main to Feature/DarkMode March 7, 2024 18:34
@KlausLoeffelmann KlausLoeffelmann changed the base branch from Feature/DarkMode to main March 7, 2024 19:17
@AraHaan
Copy link
Member

AraHaan commented Mar 19, 2024

I personally do not like the extensive usage of SystemColor in this change. I would prefer the ability to instead pass in (optionally) a type derived from ProfessionalColorTable instead of that. This way not only does it integrate the the existing Renderer types in the API, but also could be used to theme everything as well. Say for example one wants to be able to have a switch from Light, Dark, and Pink mode in their application. With that change they can and besides if we are already exploring theming support might as well invest a little bit extra work to allow custom app defined themes as well 😄. I have such an application where such a change would eliminate thousands of lines of code where I have to manually draw everything or make "fake" versions of controls within winforms today using picture boxes and svg images that I manipulate the colors on using System.Drawing.Common apis. Of which is a pain in the butt to maintain.

Also if by default the non-client area used said type derived from ProfessionalColorTable to draw itself within winforms I would be happy as well as it is then LESS work that I would need to do for my application as well. Same for scrollbars and other not so friendly controls as well that I would need to "fake" in my application that lies inside of any component in winforms that derive from ScrollableControl (just about everything basically).

Also having it like this would promote people to ship special nuget packages that people can install and pass in to their application to use a theme they created using a type derived from ProfessionalColorTable that said packages make public in order to be used with this type of API to then properly render it's theme everywhere that is possible.

@KlausLoeffelmann
Copy link
Member Author

Please read my comments on theming in the respective issue.

Thanks!

@AraHaan
Copy link
Member

AraHaan commented Mar 21, 2024

Please read my comments on theming in the respective issue.

Thanks!

The SystemColor palette could use the passed in ProfessionalColorTable derived type to provide the colors to use. Doing that could simplify things a bit more.

@@ -2425,37 +2430,20 @@ private unsafe void SetFormCornerPreferenceInternal(FormCornerPreference cornerP
[Experimental(DiagnosticIDs.ExperimentalDarkMode, UrlFormat = Application.WinFormsExperimentalUrl)]
public Color FormBorderColor

Choose a reason for hiding this comment

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

How to return to the orignal System default Color with this api?
(DWMWA_COLOR_DEFAULT with value 0xFFFFFFFF)

How to remove 1px border color around Form with this api ?
(DWMWA_COLOR_NONE with value 0xFFFFFFFE)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! I will test and add this to the docs! Nice one - adds real value!


/// <summary>
/// The appearance of a Modern UI toggle switch.
/// This setting is not taken into account, when <see cref="VisualStylesMode"/> is set
Copy link
Member

Choose a reason for hiding this comment

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

The second part of this should be a remarks section.

@@ -633,14 +633,14 @@ internal static ApplyGraphicsProperties GetApplyStateFlags(IDeviceContext device
// flag when this was originally written.

Debug.Assert(apply.HasFlag(ApplyGraphicsProperties.Clipping)
|| graphics.Clip is null
Copy link
Member

Choose a reason for hiding this comment

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

This file needs undone.

float radius = rect.Height / 2f;

using var path = new GraphicsPath();
path.AddArc(rect.X, rect.Y, radius * 2, radius * 2, 180, 90);
Copy link
Member

Choose a reason for hiding this comment

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

Is this a rounded rectangle?


private readonly ConcurrentDictionary<AnimatedControlRenderer, AnimationRendererItem> _renderer = [];

private readonly WindowsFormsSynchronizationContext? _syncContext = (WindowsFormsSynchronizationContext?)SynchronizationContext.Current;
Copy link
Member

Choose a reason for hiding this comment

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

You're going to be grabbing this for whatever the thread happens to be that the static Instance runs on. Is this really what you want? I presume you'd be better off just calling SynchronizationContext.Currrent?.Post() inline.

@dotnet dotnet locked and limited conversation to collaborators Aug 9, 2024
@KlausLoeffelmann
Copy link
Member Author

Locked the discussion, since we need to separate this out into different PRs.
More background info to come soon.

@merriemcgaw
Copy link
Member

Important!

I've made the extremely hard decision to bump this feature's release to 10. This was coming in awfully hot for .NET 9 RC1 and wasn't looking like everything we needed to do internally (t's crossed and i's dotted) could be addressed in time to meet code complete, which is in just a couple of days. Rather than try to force it into RC1 I decided the best course of action will be to move it to 10 and have it ready to go well before the first preview. Given the bake time it will have in the next release, I think we can be confident of a successful release in 10.

Those of you who are invested in the Dark Mode and VisualStyles work - we want all of the feedback that we can get. You will be able to start seeing builds with this feature work in a few weeks at most when .NET repos move Main to target .NET 10. We have every confidence in this feature and are looking forward to having it ready for you with the very first builds of .NET 10 for your evaluation.

If you have any questions, please don't hesitate to ask on the main issue #7641.

@RussKie
Copy link
Member

RussKie commented Aug 12, 2024

Looks like the dark-mode landed in .NET 9 afterall --> #11857
🎉

@merriemcgaw
Copy link
Member

Indeed it did! We were able to separate it from the bigger Visual Styles work and get it in for .NET 9. Huge Kudos to @KlausLoeffelmann and @JeremyKuhne for the heroics over the weekend to make sure we had this for .NET 9. Without their above and beyond efforts we would not have been able to make code-complete. Please use and provide feedback!

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.

[API Suggestion] Introduce Dark Mode and A11Y compatible Visual Styles for Apps targeting .NET 9 and Win 11