Ensure DevDiagnostics' owned windows honor theme changes #3790
Conversation
andreww-msft
requested review from
timkur,
jaholme,
xhan65,
aeloros,
zadesai and
dabhattimsft
tools/DevDiagnostics/DevHome.DevDiagnostics/Controls/ThemeAwareWindow.cs
Show resolved
Hide resolved
| private void ThemeAwareWindow_Closed(object sender, WindowEventArgs args) | ||
| { | ||
| var barWindow = Application.Current.GetService<PrimaryWindow>().DBarWindow; | ||
| barWindow?.RemoveRelatedWindow(this); | ||
| } |
There was a problem hiding this comment.
Why can't this be handled in the base class?
There was a problem hiding this comment.
Because BarWindow is also derived from the same base class, and it probably isn't right for the base class to have knowledge of its derived classes.
| * 3. Add the tool window to the list of related windows for the BarWindow by calling AddRelatedWindow. | ||
| * 4. Remove the tool window from the list of related windows for the BarWindow when the tool window is closed. |
There was a problem hiding this comment.
Both of these seem like candidates for code reusability. Maybe the base class should do this in some manner.
There was a problem hiding this comment.
The issue here is that both the main BarWindow and the tool windows derive from the same base class. You could argue that I should separate the common theme behavior from the related-window behavior - and I wouldn't disagree, but we can't derive a window class from multiple classes.
There was a problem hiding this comment.
You could hardcode the base class to recognize BarWindow, since it is only "one" whereas the others are dynamically "many". Also, you could look into Generics and include the BarWindow class as a type argument to do something similar but less hardcoded.
There was a problem hiding this comment.
Yes, I see this PR as temporary. It fixes the issues, but I will update it, incorporating this feedback, once we move to WAS 1.6, per new issue: #3803
| <RowDefinition Height="*"/> | ||
| </Grid.RowDefinitions> | ||
| <StackPanel Orientation="Horizontal" Height="31"> | ||
| <Image Source="/Images/dd.ico" Height="16" Width="16" Margin="8,2,0,0" VerticalAlignment="Center"/> |
There was a problem hiding this comment.
What are these changes for? Why are we adding an icon to this clipboard window? I thought our text was already getting crunchy.... does this make it worse?
There was a problem hiding this comment.
In order to leverage the same theme-awareness, I made this view the same as BarWindow in respect of ExtendsContentIntoTitleBar, hence had to add back the icon/title we already had. I will clean this all up once we move to WAS 1.6 later (#3803 tracking)- there's also some overlap with tool-window consistency (#3805 tracking), and the two should probably be done toghether.
Summary of the pull request
Prior to this, all the theme-related work was done in BarWindow, but this didn't accommodate any other windows. We want internal tool windows to follow the theme of the main BarWindow, regardless of whether the change is from the app's own Settings or from system-wide personalization changes. To achieve this, I created a new ThemeAwareWindow class (derived from WindowEx) to abstract all the theme-related work, including for custom chrome. We also want general consistency between the main BarWindow and any other internal windows, and we can use this abstracted class to achieve this. BarWindow now derives from this, as does the first internal tool window, ClipboardMonitoringWindow. Theme changes will now propagate from the BarWindow to internal tool windows.
Any future tool windows should follow this same pattern:
Closes #3718