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 Dark Mode Visual Style Theme Subclass #11985

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

memoarfaa
Copy link

@memoarfaa memoarfaa commented Aug 25, 2024

This is an improvement to new Dark Mode experimental feature
#11857
see
https://learn.microsoft.com/en-us/windows/win32/controls/theme-subclasses#two-ways-to-use-a-theme-subclass

Fixes #11893.
Fixes #11953.

Microsoft Reviewers: Open in CodeFlow

…orted

Windows Registry Key may be deleted and never restored and may be exist and there is no dark mode subclass in aero.msstyles
@memoarfaa memoarfaa requested a review from a team as a code owner August 25, 2024 17:48
@memoarfaa memoarfaa marked this pull request as draft August 25, 2024 17:49
@dotnet-policy-service dotnet-policy-service bot added the draft draft PR label Aug 25, 2024
@memoarfaa
Copy link
Author

memoarfaa commented Aug 25, 2024 via email

@memoarfaa memoarfaa removed their assignment Aug 25, 2024
@@ -17,7 +17,11 @@ public partial class DataGridViewButtonCell : DataGridViewCell
private static readonly int s_propButtonCellFlatStyle = PropertyStore.CreateKey();
private static readonly int s_propButtonCellState = PropertyStore.CreateKey();
private static readonly int s_propButtonCellUseColumnTextForButtonValue = PropertyStore.CreateKey();
private static readonly VisualStyleElement s_buttonElement = VisualStyleElement.Button.PushButton.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_darkButtonElement = VisualStyleElement.CreateElement("DarkMode_Explorer::BUTTON", 1, 1);
Copy link
Member

Choose a reason for hiding this comment

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

I think, instead of hardcoding "DarkMode_Explorer" it should be an interpolated const, e.g.: {DarkModeIdentifier}_{Explorer}.
See:

internal const string DarkModeIdentifier = "DarkMode";
internal const string ExplorerThemeIdentifier = "Explorer";
internal const string ItemsViewThemeIdentifier = "ItemsView";
internal const string ComboBoxButtonThemeIdentifier = "CFD";

If any more consts need adding - they should be added here.

Copy link
Author

Choose a reason for hiding this comment

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

Done, does that look good now?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I do not see that you changed anything.
Can you make the changes?

@RussKie
Copy link
Member

RussKie commented Aug 26, 2024

@KlausLoeffelmann @JeremyKuhne @merriemcgaw @lonitra - if this fixes the rendering issue, this change would be a great addition to the .NET 9 release.

@AraHaan
Copy link
Member

AraHaan commented Aug 26, 2024

As long as I can use the new API to set color mode on my entire application from my settings form at runtime at any time to switch between light and dark mode I would be happy in the .NET 9 release.

Also what about all of the system menus in all the possible controls as well? Are they dark themed with this change now or do I have to go into black magic to convert them to ContextMenuStrips that I set then a custom dark theme renderer to that is not rounded. Yes, I feel like an easier way of doing this is inside of Winforms would be a good idea (where winforms converts them to a ContextMenuStrip that auto themes it to light or dark) for the application and to a global setting for if they want all ContextMenuStrips to be rounded or not would make me happy as well but that can come in .NET 10 on that change.

@@ -379,21 +377,9 @@ private static int GetSystemColorModeInternal()
return SystemDarkModeDisabled;
}

int systemColorMode = SystemDarkModeDisabled;
Copy link
Member

Choose a reason for hiding this comment

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

We're still discussing, if we can take this for .NET 9 RC2 (the bar is service, and we would need to consider this a bug fix). So, this particular change, we would probably not want to take. @merriemcgaw, @lonitra.

Copy link
Member

Choose a reason for hiding this comment

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

This needs to be as it is, please revert this.

@KlausLoeffelmann KlausLoeffelmann added the 📭 waiting-author-feedback The team requires more information from the author label Aug 27, 2024
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

Could you please also update the description to provide a visual of the before/after of this change?

@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 3, 2024
Copy link

codecov bot commented Sep 3, 2024

Codecov Report

Attention: Patch coverage is 25.80645% with 46 lines in your changes missing coverage. Please review.

Project coverage is 75.06842%. Comparing base (8cdb3e0) to head (2ac8c87).
Report is 8 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #11985         +/-   ##
===================================================
+ Coverage   75.03366%   75.06842%   +0.03476%     
===================================================
  Files           3063        3063                 
  Lines         632075      632114         +39     
  Branches       46784       46800         +16     
===================================================
+ Hits          474269      474518        +249     
+ Misses        154431      154218        -213     
- Partials        3375        3378          +3     
Flag Coverage Δ
Debug 75.06842% <25.80645%> (+0.03476%) ⬆️
integration 17.94234% <16.12903%> (+0.08534%) ⬆️
production 48.22065% <25.80645%> (+0.08131%) ⬆️
test 97.01607% <ø> (-0.00029%) ⬇️
unit 45.24617% <22.58065%> (-0.00481%) ⬇️

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

@@ -17,7 +17,11 @@ public partial class DataGridViewButtonCell : DataGridViewCell
private static readonly int s_propButtonCellFlatStyle = PropertyStore.CreateKey();
private static readonly int s_propButtonCellState = PropertyStore.CreateKey();
private static readonly int s_propButtonCellUseColumnTextForButtonValue = PropertyStore.CreateKey();
private static readonly VisualStyleElement s_buttonElement = VisualStyleElement.Button.PushButton.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_darkButtonElement = VisualStyleElement.CreateElement("DarkMode_Explorer::BUTTON", 1, 1);
Copy link
Member

Choose a reason for hiding this comment

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

I believe this was missed

Suggested change
private static readonly VisualStyleElement s_darkButtonElement = VisualStyleElement.CreateElement("DarkMode_Explorer::BUTTON", 1, 1);
private static readonly VisualStyleElement s_darkButtonElement = VisualStyleElement.CreateElement($"{Control.DarkModeIdentifier}_{Control.ExplorerThemeIdentifier}::BUTTON", 1, 1);

Comment on lines +69 to +82
/// <summary>
/// <para>
/// Gets a value specifying whether the operating system has Dark Mode visual styles subclass and the Application can use this subclass to draw controls with Dark Mode theme.
///</para>
///<para>
///<return>
///<para> Returns true if Visual Style Dark Mode subclass is: </para>
/// <para> 1) Supported by the operating system.</para>
/// <para> 2) Enabled in the client area.</para>
/// <para> 3) operating system not ruining in high contrast mode</para>
/// <para> Otherwise, returns false. Note that if false is returned, attempts to create/use objects of this class will throw exceptions.</para>
///</return>
///</para>
/// </summary>
Copy link
Member

@lonitra lonitra Sep 4, 2024

Choose a reason for hiding this comment

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

Suggested change
/// <summary>
/// <para>
/// Gets a value specifying whether the operating system has Dark Mode visual styles subclass and the Application can use this subclass to draw controls with Dark Mode theme.
///</para>
///<para>
///<return>
///<para> Returns true if Visual Style Dark Mode subclass is: </para>
/// <para> 1) Supported by the operating system.</para>
/// <para> 2) Enabled in the client area.</para>
/// <para> 3) operating system not ruining in high contrast mode</para>
/// <para> Otherwise, returns false. Note that if false is returned, attempts to create/use objects of this class will throw exceptions.</para>
///</return>
///</para>
/// </summary>
/// <summary>
/// Gets a value specifying whether the operating system has Dark Mode visual styles subclass and the Application can use this subclass to draw controls with Dark Mode theme.
///</summary>
/// <return>
/// <see langword="true"/> if Visual Style Dark Mode subclass is supported by the operating system,
/// enabled in the client area, and operating system not running high contrast mode.
/// Otherwise, returns <see langword="false"/>. Note that if <see langword="false"/> is returned, attempts to create/use objects of this class will throw exceptions.
/// </return>

///</para>
/// </summary>
[Experimental(DiagnosticIDs.ExperimentalDarkMode, UrlFormat = DiagnosticIDs.UrlFormat)]
public static bool IsDarkModeSupported
Copy link
Member

@lonitra lonitra Sep 4, 2024

Choose a reason for hiding this comment

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

At this point we cannot add new public API for net 9 RC2. Does this need to be public to fix rendering issues?

@lonitra lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Sep 4, 2024
Copy link
Member

@KlausLoeffelmann KlausLoeffelmann left a comment

Choose a reason for hiding this comment

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

Hey @memoarfaa,

I have a couple of additional issues which need to be addressed - most of them are just using consts for the theme names, so nothing big.

That said, while we would need to take a closer look at your proposed additional API for .NET 10, we cannot take that in RC2 for .NET 9 since we're way past code complete. Also, for new APIs, we need to bring that to the API review board, and it has to be officially discussed and then agreed to.

If you can do these changes on short notice, that would be nice, because the cut-of-date is approaching, and you bug fix would need to be tested before!

Thanks so much for doing this - we really appreciate!!

@@ -3320,6 +3320,7 @@ static System.Windows.Forms.VisualStyles.VisualStyleInformation.Url.get -> strin
static System.Windows.Forms.VisualStyles.VisualStyleInformation.Version.get -> string!
static System.Windows.Forms.VisualStyles.VisualStyleRenderer.IsElementDefined(System.Windows.Forms.VisualStyles.VisualStyleElement! element) -> bool
static System.Windows.Forms.VisualStyles.VisualStyleRenderer.IsSupported.get -> bool
static System.Windows.Forms.VisualStyles.VisualStyleRenderer.IsDarkModeSupported.get -> bool
Copy link
Member

Choose a reason for hiding this comment

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

We cannot take new APIs at this point. So, this needs to be reverted.

@@ -379,21 +377,9 @@ private static int GetSystemColorModeInternal()
return SystemDarkModeDisabled;
}

int systemColorMode = SystemDarkModeDisabled;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be as it is, please revert this.

{
}

return systemColorMode;
Copy link
Member

Choose a reason for hiding this comment

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

Same here - we need to keep this.

private static readonly VisualStyleElement s_comboBoxElement = VisualStyleElement.ComboBox.DropDownButton.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_comboBoxElement = Application.IsDarkModeEnabled
? VisualStyleElement.CreateElement($"{Control.DarkModeIdentifier}_{Control.ComboBoxButtonThemeIdentifier}::COMBOBOX", 1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Could you introduce a const for COMBOBOX as well?

@@ -17,7 +17,11 @@ public partial class DataGridViewButtonCell : DataGridViewCell
private static readonly int s_propButtonCellFlatStyle = PropertyStore.CreateKey();
private static readonly int s_propButtonCellState = PropertyStore.CreateKey();
private static readonly int s_propButtonCellUseColumnTextForButtonValue = PropertyStore.CreateKey();
private static readonly VisualStyleElement s_buttonElement = VisualStyleElement.Button.PushButton.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_darkButtonElement = VisualStyleElement.CreateElement("DarkMode_Explorer::BUTTON", 1, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I do not see that you changed anything.
Can you make the changes?


#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_comboBoxReadOnlyButton = Application.IsDarkModeEnabled
? VisualStyleElement.CreateElement($"{Control.DarkModeIdentifier}_{Control.ComboBoxButtonThemeIdentifier}::COMBOBOX", 5, 1)
Copy link
Member

Choose a reason for hiding this comment

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

And here! 😸

private static readonly VisualStyleElement s_headerElement = VisualStyleElement.Header.Item.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_headerElement = Application.IsDarkModeEnabled
? VisualStyleElement.CreateElement($"{Control.DarkModeIdentifier}_{Control.ItemsViewThemeIdentifier}::Header", 1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Same for the "Header".

private static readonly VisualStyleElement s_headerElement = VisualStyleElement.Header.Item.Normal;
#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
private static readonly VisualStyleElement s_headerElement = Application.IsDarkModeEnabled
? VisualStyleElement.CreateElement($"{Control.DarkModeIdentifier}_{Control.ItemsViewThemeIdentifier}::Header", 1, 1)
Copy link
Member

Choose a reason for hiding this comment

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

And here! 😺

#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
if (Application.IsDarkModeEnabled)
{
PInvoke.SendMessage(tooltipHwnd, PInvoke.TTM_SETWINDOWTHEME, default, "DarkMode_Explorer");
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the existing const here?

{
get
{
bool supported = AreClientAreaVisualStylesSupported;
Copy link
Member

Choose a reason for hiding this comment

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

This is too invasive a change this late in the game.
Can we revert this to the existing method, because that had been tested exhaustively?

#pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
if (Application.IsDarkModeEnabled && GetStyle(ControlStyles.ApplyThemingImplicitly))
{
_ = PInvoke.SetWindowTheme(
Copy link
Contributor

Choose a reason for hiding this comment

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

If not all tabs can be shown at once, the tab control will display an up-down control that will also need to be styled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draft draft PR 📭 waiting-author-feedback The team requires more information from the author
Projects
None yet
6 participants