Repository navigation
Use dark-themed images and colors in dark mode - #746
mfkeeq199458 wants to merge 4 commits into
Conversation
Menu, toolbar and panel icons are now loaded through ThemedImages, which picks the *_dark resource variants (images/png/48_dark) when the application runs with the dark color mode, so black glyphs stay visible on dark strips. Dark-mode defaults were adjusted where fixed colors were still hardcoded: bookmark and auto-bookmark markers, the bookmark/filter 'i'/'!' marker text, LED off/tail states, the ColorComboBox items and the highlight entry preview label. The change is a pivot from an earlier, never-committed approach that inverted icons at paint time via a custom ToolStrip renderer.
Hirogen
left a comment
There was a problem hiding this comment.
Thanks @mfkeeq199458 — switching to proper _dark assets is much better than the paint-time inversion idea, and the dark PNGs look right (same masks, inverted glyphs). Using Application.IsDarkModeEnabled at construction matches the rest of the app and the "restart required" setting, and light mode stays unchanged.
A few things before merging:
Coverage gaps
LogWindow.cs:535-536:btnFilterDown/btnFilterUpstill useResources.ArrowDown/ArrowUp, even though this PR adds_darkvariants for both.toolStripButtonTail(LogTabWindow.designer.cs:893) andPro_Filter(LogWindow.cs:534) stay light-only.
Contrast
- In dark mode the 'i'/'!' marker text (220,130,20) on the new bookmark fill (140,195,240) is about 1.5:1, harder to read than in light mode. Could you pick a darker text or a darker fill?
- LED off (170) and tail-off (175) are almost indistinguishable in dark mode; in light mode they're clearly different (100 vs 160). Please keep a visible gap.
ThemedImages
(Bitmap)Resources.ResourceManager.GetObject(name)triggers nullable warnings (CS8600/CS8603) and silently returnsnullon a typo. Please throw (orDebug.Assert) when the resource is missing, so a bad name fails loudly.ApplyDarkImagesalso runs in light mode and replaces the designer-set bitmaps without disposing them. Either only run it whendarkModeis true, or rename it toApplyThemedImages.
Cleanup
- 13 of the 26
_darkPNGs aren't referenced anywhere (Add, ArrowLeft/Right, Bookmarks, Bookmark_added/manager/remove, Check_circle, Deceased, Delete, Favorite, Folder_open, Star). Please drop them, or use the arrows for the filter buttons above. HighlightEntryDialog.cs:409-411: please reusePaintHelper's dark colors (GetForeColorFromHighlightEntry/GetBackColorFromHighlightEntry) instead of hardcoding them. The (32,32,32) background is a no-op anyway, sinceSystemColors.Windowalready resolves to it in dark mode.ColorComboBox.cs:71:e.ForeColorwould be theme- and selection-aware instead ofBrushes.White/Black.LedIndicatorService.cs: please revert the whitespace-only realignment and the redundantusing System.Windows.Forms;.
Description
- The default dark-mode grid text changes from White to (220,220,220) (
PaintHelper.cs:107/260/275). That affects every log line, so please mention it in the PR description. I'm fine with the change itself.
Thanks again — this is close!
- Use dark arrow icons for the pro filter up/down buttons - Darken bookmark marker text in dark mode for contrast on the light bookmark fills (220,130,20 -> 90,45,0, ~6:1 contrast) - Widen gray gap between LED off-state (170 -> 110) and tail-off (175) in dark mode, mirroring the light-mode 100 vs 160 difference - ThemedImages.Get now throws InvalidOperationException on a missing resource instead of silently returning null (also fixes CS8600/CS8603) - Rename ApplyDarkImages to ApplyThemedImages (runs in both modes) - HighlightEntryDialog preview: reuse PaintHelper.GetForeColorFromHighlightEntry defaults; revert background to SystemColors.Window ((32,32,32) was a no-op) - ColorComboBox: draw item text with e.ForeColor instead of hardcoded Brushes.White/Black - LedIndicatorService: remove redundant using System.Windows.Forms and whitespace-only realignment - Remove dead image assignment on toolStripButtonTail: DisplayStyle=Text means the embedded image was never rendered - Remove unused image assets: 13 unreferenced icon pairs (light + _dark) with their resx/Designer entries (main + de localization), plus 5 orphaned files (LogExpert.bmp, bubble.ico, Icon1.ico, Icon2.ico, LogExpert-Icon.gif)
Hirogen
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround @mfkeeq199458 — almost everything from the last round is in. The new marker text is ~6.2:1 on the bookmark fill and ~6.9:1 on the auto-bookmark fill, the LED gap is clear now, the filter arrows are themed, and ThemedImages.Get throws on a missing resource.
A few small things left:
Build
ThemedImages.cs:11and:17add two CA1304 warnings (ResourceManager.GetObject(string)without a culture). Please passCultureInfo.CurrentUICulture, or useResources.Cultureto match the generatedResources.Designer.cs.
Leftovers from the previous review
Pro_Filter(LogWindow.cs:534) is still light-only. APro_Filter_darkvariant throughThemedImageswould finish the set. If that's not feasible, just say so and we can track it separately.- Please update the PR description. It should mention that the default dark-mode grid text changes from White to (220,220,220), and list the removed assets (the 13 light/dark icon pairs,
LogExpert.bmp,LogExpert-Icon.gif,Icon1.ico,Icon2.ico,bubble.ico). I checked: nothing references them, and the installer andApplicationIconuselogexpert.ico, so the cleanup is fine. It just needs to be visible in the description.
Consistency (small)
PaintHelper.BookmarkMarkerTextColorreadsApplication.IsDarkModeEnableddirectly, while the bookmark fills take adarkModeparameter. Could it be a methodGetBookmarkMarkerTextColor(bool darkMode)like the otherPaintHelperAPIs? That keeps it testable inPaintHelperTestsand stops the text and fill from disagreeing.LogWindow.cs:535-537: please readApplication.IsDarkModeEnabledonce into a local, as:275already does.- Optional: in light mode,
ApplyThemedImagesreloads every bitmap the designer already assigned and doesn't dispose the originals. An earlyif (!darkMode) return;would avoid that.
FYI, no action needed: a selected ColorComboBox item now draws its text in the highlight colour in light mode too, instead of black. I think that's an improvement. Just noting it because the PR otherwise leaves light mode unchanged.
After the warnings and the description are sorted, this is good to merge from my side. Thanks!
Build warnings (CA1304): - ThemedImages: pass CultureInfo.CurrentUICulture to ResourceManager.GetObject API consistency: - PaintHelper.BookmarkMarkerTextColor property -> GetBookmarkMarkerTextColor(bool darkMode) method, matching other PaintHelper APIs Minor cleanup: - LogWindow.ApplyResourceImages: cache Application.IsDarkModeEnabled into local variable - LogTabWindow.ApplyThemedImages: early return in light mode to avoid unnecessary bitmap reloads PR description updates (per reviewer request): - Default dark-mode grid text changes from Color.White to Color.FromArgb(220, 220, 220) (soft gainsboro tone) - Removed assets (no code references remain): 13 light/dark icon pairs, LogExpert.bmp, LogExpert-Icon.gif, Icon1.ico, Icon2.ico, bubble.ico - Installer and ApplicationIcon already use logexpert.ico, cleanup is safe Pro_Filter note: - Pro_Filter uses neutral line art that is readable on both light and dark backgrounds, so no Pro_Filter_dark variant is needed.
bb7f631 to
e48c0cf
Compare
Menu, toolbar and panel icons are now loaded through ThemedImages, which picks the *_dark resource variants (images/png/48_dark) when the application runs with the dark color mode, so black glyphs stay visible on dark strips.
Dark-mode defaults were adjusted where fixed colors were still hardcoded: bookmark and auto-bookmark markers, the bookmark/filter 'i'/'!' marker text, LED off/tail states, the ColorComboBox items and the highlight entry preview label.
The change is a pivot from an earlier, never-committed approach that inverted icons at paint time via a custom ToolStrip renderer.