share common values for both Breeze and Breeze-dark GTK themes
ClosedPublic

Authored by ohelin on Oct 21 2018, 11:48 PM.

Details

Summary

BUG: 396091

As described in bug #396091, the Breeze-dark theme is often neglected in updates. This patch fixes #396091 and some additional, related inconsistencies, enables sharing the basic stuff like shape, size, style, etc. of components in a common single css file while keeping the colour definitions separated from that in different files for both the light and dark theme.
This allows easier changes, easier maintenance because of less LOC, less duplicate code as well as easier extensibility for potential additional colour schemes like light or high-contrast.

I only did this for GTK 3.20 for now. If you like the effort, it could (and maybe should) get extended to other versions.

Further potential steps in the same direction of saving code would be going back to SASS, which is used by many other popular GTK themes and was used by Breeze-gtk as well in the past. The only downside of that is that the SASS source files would have to be "compiled" to CSS prior to packaging.

Test Plan

I don't know of any automated test suites comparing UI components. I have already posted screenshots in the original bug report and can post additional ones of the new state for comparison. Apart from that, my manual sparse testing makes me feel the dark GTK theme looks and feels much more consistent to Breeze now.

Diff Detail

Repository
R98 Breeze for Gtk
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ohelin created this revision.Oct 21 2018, 11:48 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 21 2018, 11:48 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
ohelin added a comment.EditedOct 22 2018, 12:38 AM

Ok, I just dissected the thing. First, I made the updated diff file against the newest master: diff_updated.diff.
Then I saved these files from Phabricator:

  • Breeze-dark-gtk_gtk-3.20_gtk.css: saved the left raw diff file
  • Breeze-gtk_gtk-3.20_common.css: saved the new raw diff file
  • Breeze-gtk_gtk-3.20_gtk.css: saved the left raw diff file

I then made the following diff files:

diff Breeze-gtk_gtk-3.20_gtk.css Breeze-gtk_gtk-3.20_common.css > diff_gtk_vs_common.diff
diff -t -y --suppress-common-lines Breeze-dark-gtk_gtk-3.20_gtk.css Breeze-gtk_gtk-3.20_common.css > diff_gtk_dark_vs_common.diff

The contents of diff_gtk_vs_common.diff are trivial:

1,3d0
< @define-color headerbar_disabled_highlight_top @disabled_highlight_top;
< @define-color headerbar_disabled_highlight_bottom @disabled_highlight_bottom;
< @define-color headerbar_action_disabled_backdrop @action_disabled_backdrop;

Therefore, for the light theme, I say the patch is reviewed. The only new line in the light CSS file imports the new common file, so basically the contents have just been moved.

For the dark theme, we review the file diff_gtk_dark_vs_common.diff. Almost all the file is just the obvious, i.e. replacing color literals with the new variables. The real differences start from line 595: border-width has been modified. The differences continue until line 648. In the original files these lines correspond to the following:
Breeze-dark-gtk_gtk-3.20_gtk.css: 3069-3160
Breeze-gtk_gtk-3.20_common.css: 3066-3142

I stripped those lines as files Breeze-dark-gtk_gtk-3.20_gtk.css_diff_part and Breeze-gtk_gtk-3.20_common.css_diff_part. I then created the special diff which is the beef of this review:

diff Breeze-dark-gtk_gtk-3.20_gtk.css_diff_part Breeze-gtk_gtk-3.20_common.css_diff_part > special_diff.diff

So, the only thing left to review is the contents of special_diff.diff, and obviously that everything looks about correct visually in practice (i.e. the colors are fine). I'll continue to check out that special diff later when I have time, or if someone else would like to check it out, please go ahead.









Sort of an unorthodox way to do this, but... it works I guess! Thanks, the patch now applies, and rudimentary testing with some GTK apps doesn't show anything hideously wrong. I assume it works well enough for you too?

JFYI, if we land this patch instead of the original, we'll credit the original author. But do feel free to continue along this vein and maybe do the same for the other GTK version CSS files in another patch afterwards if you'd like!

I checked the both the diff_part files next to each other. All the other differences were quite easy to see, but this one bugs me a little:

scrollbar.vertical button.down {
  -gtk-icon-source: -gtk-icontheme("pan-down-symbolic"); }
scrollbar.vertical button.up {
  -gtk-icon-source: -gtk-icontheme("pan-up-symbolic"); }
scrollbar.horizontal button.down {
  -gtk-icon-source: -gtk-icontheme("pan-end-symbolic"); }
scrollbar.horizontal button.up {
  -gtk-icon-source: -gtk-icontheme("pan-start-symbolic"); }

So that was in the old gtk-dark theme, but not in the new one. However, it's probably just some obsolete stuff anyway. From the few differences between the theme files, I think most are just leftovers from the past and simply forgotten to update. That alone speaks for the usefulness of this patch, as probably there shouldn't be any differences in margins for example between a light and a dark version of a theme.

So, for the code part I'd say it's fine, unless someone knows better. I don't have any visual experience using Breeze, so judging from the quick look I guess it's fine, too.

Thanks, it looks good to me too in my testing. Any objections to landing this?

ngraham retitled this revision from Update diff against newest master to share common values for both Breeze and Breeze-dark GTK themes.
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)

Nada from here, I'd say go ahead.

ngraham accepted this revision.Thu, Oct 25, 2:27 AM
ngraham added a subscriber: grmat.

I haven't noticed any problems running with this over the past few days. It's unfortunate that neither the maintainer (@jackg) nor anyone else familiar with this code has showed up to review it. But it would be a shame to let something this extensive rot away, and since you and I haven't found any problems with it, my inclination is to land it.

If nobody else shows up soon to offer a review, I'll land this on Halloween (October 31).

Note for @grmat: I will land this with your authorship and then close out D15786.

This revision is now accepted and ready to land.Thu, Oct 25, 2:27 AM
grmat added a comment.Thu, Oct 25, 1:30 PM

Thanks for the comments, review and for splitting up the diff. I also know it was daunting to review the patch in its original form and I could've done that better.

Nah, you did great! The only thing you could have done to make this easier would have been to submit two patches: one to templatize the code, and a second one to make functional changes. But Phabricator makes this workflow somewhat awkward and more difficult than it needs to be compared to Github for example, so I can understand why you didn't. Thanks again for your valuable contribution, and I think this will probably make it into Plasma 5.15!

@grmat, could you provide your full name and email address so I can add your authorship information? Thanks!

Closed by commit R98:6ec73bf06938: share common values for both Breeze and Breeze-dark GTK themes (authored by Matthias Groß <a.grmat@sub.red>, committed by ngraham). · Explain WhyWed, Oct 31, 6:47 PM
This revision was automatically updated to reflect the committed changes.