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
Remove false comment regarding ColorSettingsPage #9073
Remove false comment regarding ColorSettingsPage #9073
Conversation
update tests
@NikolayXHD Really sorry for the mistake... |
No problem, I myself lost code in .designer.cs multiple times. Me putting events in .designer.cs file was poor design encouraged by WinForms editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just nits.
public bool SettingsAreModified => | ||
_page.SelectedThemeId != ThemeModule.Settings.Theme.Id || | ||
_page.UseSystemVisualStyle != ThemeModule.Settings.UseSystemVisualStyle || | ||
!_page.SelectedThemeVariations.SequenceEqual(AppSettings.ThemeVariations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support left-reading, please:
public bool SettingsAreModified => | |
_page.SelectedThemeId != ThemeModule.Settings.Theme.Id || | |
_page.UseSystemVisualStyle != ThemeModule.Settings.UseSystemVisualStyle || | |
!_page.SelectedThemeVariations.SequenceEqual(AppSettings.ThemeVariations); | |
public bool SettingsAreModified => | |
_page.SelectedThemeId != ThemeModule.Settings.Theme.Id | |
|| _page.UseSystemVisualStyle != ThemeModule.Settings.UseSystemVisualStyle | |
|| !_page.SelectedThemeVariations.SequenceEqual(AppSettings.ThemeVariations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
GitUI/Theming/ThemeModule.cs
Outdated
@@ -11,6 +11,8 @@ namespace GitUI.Theming | |||
{ | |||
public static class ThemeModule | |||
{ | |||
private static bool _suppressWin32Hooks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add documention like /// To be activated by the TestAccessor only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed variable to indicate test-only scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
The changes in the actual code seem fine.
Have only briefly looked at the changed tests. Seem fine, no deeper investigation.
Have not run.
Some time ago the event handlers from ColorsSettingsPage.cs were lost by mistake.
Before knowing that, I misinterpreted what was going on, which lead me to writing this
Here I revert that unnecessary complication, as well as the factually incorrect comment I wrote to explain it.
Test
Tested manually and with unit tests
✒️ I contribute this code under The Developer Certificate of Origin.