Go back to SCSS
ClosedPublic

Authored by gepardo on Nov 25 2018, 12:38 PM.

Details

Summary

As it was explained in https://phabricator.kde.org/D16365,

Further potential steps in the same direction of saving code would be going back to SASS

This patch tries to go back to SCSS sources with backporting all the fixes made in Breeze-gtk recently. As a base, I used this repository: https://github.com/dirruk1/gnome-breeze. But it's outdated, so I walked through the commit history of breeze-gtk and ported all the changes here. The script for building the theme was ported to Python3, and shell scripts now use POSIX sh instead of bash. Also I fixed some inconsistences between Qt and GTK Breeze themes. For example, Breeze-Qt colorscheme was updated a little (in Plasma 5.12, as far as I remember), but the GTK theme was updated only partially; now the colors are synchronized. Both GTK-3.18 and GTK-3.20 versions work fine for me; many things were also fixed for GTK 3.18.

For building the theme, ruby-sass (or more lightweght sassc) is required. Also the theme can be patched to allow changing the colorscheme in System Settings (though, rebuilding the theme with sassc is required for this). The original repository (https://github.com/dirruk1/gnome-breeze) had also named-colors branch that allowed using named colors and changing the colorscheme without rebuilding; but I couldn't get this working.

Because both Breeze-gtk and Breese-dark-gtk are now built from sources, there will be no more inconsistences between them. Rebuilding these themes can be done using rebuild-theme.sh.

Test Plan

I do not know how to test this automatically. But I am using this version of Breeze-GTK for several months (I am not using many GTK+ applications, though). Also checked it on gtk3-widget-factory; it seems to work fine.

GTK2 version was tested on Gimp, but it is mostly unchanged.

For differences between https://github.com/dirruk1/gnome-breeze and this version, you can see my GitHub repo where I worked on this patch: https://github.com/alex65536/gnome-breeze/tree/breeze-gtk-merge.

Diff Detail

Repository
R98 Breeze for Gtk
Branch
breeze-gtk-sass (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6343
Build 6361: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: plasma-devel. · View Herald TranscriptNov 25 2018, 12:38 PM
gepardo requested review of this revision.Nov 25 2018, 12:38 PM
ngraham added subscribers: jackg, ngraham.

@jackg, if you're around, I'd really appreciate your perspective on this patch. I'm not sure about the history regarding why we moved away from this approach, whether or not it's proved to be a mistake, and if this is the right way to move back.

More generally, if anyone else knows the history here, please speak up.

This patch is not reviewed for two weeks. How soon can I get some feedback on it?

ngraham requested changes to this revision.Dec 22 2018, 4:47 AM
ngraham added a reviewer: VDG.

@gepardo I'm very sorry that you haven't gotten any real feedback on this yet. :( The maintainer seems to have vanished. I will CC come more people and have a go at reviewing it myself.

One thing that's definitely going to need to change is that if since you've added build dependencies, this needs to be reflected in CMakeLists.txt, because right now, running your rebuild_theme.sh simply doesn't work without the new dependency:

$  (arcpatch-D17154) ./rebuild-theme.sh
./build_theme.sh: 18: ./build_theme.sh: sass: not found
./build_theme.sh: 18: ./build_theme.sh: sass: not found
./build_theme.sh: 18: ./build_theme.sh: sass: not found
./build_theme.sh: 18: ./build_theme.sh: sass: not found

On that subject, rebuilding the theme should be done as a part of the default build target when running make rather than requiring the use of a script. It's a requirement that all the work gets done simply by running make. This will require a few CMake adjustment too.

Can you make these changes?

This revision now requires changes to proceed.Dec 22 2018, 4:47 AM

On that subject, rebuilding the theme should be done as a part of the default build target when running make rather than requiring the use of a script. It's a requirement that all the work gets done simply by running make. This will require a few CMake adjustment too.

I had an even better idea: to run these lines directly from CMakeLists.txt, so it will be integrated with building process as good as possible.

gepardo updated this revision to Diff 48030.Dec 22 2018, 8:42 PM

Use CMake for building themes

The following changes are made:

  • cmake/FindSass.cmake is now used to detect SASS compiler presence
  • build scripts are invoked from cmake, so everythings builds using simple cmake && make
gepardo updated this revision to Diff 48031.EditedDec 22 2018, 8:51 PM

More improvements:

  • remove gtk-3.18 as it's not rebuilt. The theme is still usable under GTK+ 3.18 (files are actually located in gtk-3.0)
  • add python as a dependency in CMakeLists.txt
ngraham added inline comments.Dec 24 2018, 4:06 PM
CMakeLists.txt
12

Shouldn't this be required since you can't build the theme without it?

13

This doesn't seem like the correct way to require Python 3:

dev@dev-pc:~/repos/breeze-gtk$  (arcpatch-D17154) cmake -DCMAKE_INSTALL_PREFIX=/usr
-- Could NOT find Sass (missing: Sass_EXECUTABLE) 
CMake Error at CMakeLists.txt:13 (find_package):
  By not providing "FindPython3.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Python3", but
  CMake did not find one.

  Could not find a package configuration file provided by "Python3" with any
  of the following names:

    Python3Config.cmake
    python3-config.cmake

  Add the installation prefix of "Python3" to CMAKE_PREFIX_PATH or set
  "Python3_DIR" to a directory containing one of the above files.  If
  "Python3" provides a separate development package or SDK, be sure it has
  been installed.


-- Configuring incomplete, errors occurred!
See also "/home/dev/repos/breeze-gtk/CMakeFiles/CMakeOutput.log".
gepardo updated this revision to Diff 48145.Dec 24 2018, 7:01 PM

Make Sass a required dependency

gepardo marked 2 inline comments as done.Dec 24 2018, 7:30 PM
gepardo added inline comments.
CMakeLists.txt
13

Using find_package(Python3) worked for me. But I understood the reason why it may fail.

It seems that find_package(Python3) appeared only in CMake 3.12, while find_package(PythonInterp) was used earlier (now it's deprecated). So, I'll add a conditional for older and current CMake.

gepardo updated this revision to Diff 48148.Dec 24 2018, 7:32 PM
gepardo marked an inline comment as done.

Fix finding Python package for CMake <3.12

Thanks much better. Now it can find Python 3 with CMake version < 3.12, and Sass is a required dependency. I believe Sysadmin has asked to be notified of all new required dependendies, so adding I'm subscribing them.

Now we're close, but the install target doesn't complete successfully:

dev@dev-pc:~/repos/breeze-gtk$  (arcpatch-D17154) sudo make install
[100%] Built target gtkbreeze5.5
/usr/lib/ruby/vendor_ruby/sass/util.rb:1109: warning: constant ::Fixnum is deprecated

[...]

-- Installing: /usr/share/themes/Breeze-Dark/assets/check-selectionmode-checked-backdrop-insensitive@2.png
-- Installing: /usr/share/themes/Breeze-Dark/assets/check-mixed-hover@2.png
-- Installing: /usr/share/themes/Breeze-Dark/assets/togglebutton-hover.png
CMake Error at Breeze-dark-gtk/cmake_install.cmake:73 (file):
  file INSTALL cannot find
  "/home/dev/repos/breeze-gtk/Breeze-dark-gtk/gtk-3.18".
Call Stack (most recent call first):
  cmake_install.cmake:42 (include)


Makefile:73: recipe for target 'install' failed
make: *** [install] Error 1

Here's the contents of that folder:

dev@dev-pc:~/repos/breeze-gtk$  (arcpatch-D17154) ls /home/dev/repos/breeze-gtk/Breeze-dark-gtk/
assets      cmake_install.cmake  gtk-2.0  gtk-3.20
CMakeFiles  CMakeLists.txt       gtk-3.0  Makefile

Looks like GTK 3.18 didn't get built?

Oops, sorry, I forgot to check the installation.

GTK+ 3.18 did built, but it was built into gtk-3.0 directory (the build scripts in the old repo seem to do so). But CMakeLists.txt remained unchanged.

I noticed that recent versions install gtk-3.18 and gtk-3.20 dirs, so I'll change the build scripts to have gtk-3.18 as a target directory.

gepardo updated this revision to Diff 48153.Dec 24 2018, 8:49 PM

Fix GTK+ 3.18 theme installation

gepardo updated this revision to Diff 48156.Dec 24 2018, 9:05 PM

Add gtk-dark.css support for Breeze

Thanks @ngraham. I've now scheduled the addition of scss into the images with 63d16727883e9c9f9d09bdd2bc7163fb386c0a57

Hmm, now make fails:

~/repos/breeze-gtk$  (arcpatch-D17154) make -j2
Scanning dependencies of target gtkbreeze5.5
[ 50%] Building CXX object kconf_update/CMakeFiles/gtkbreeze5.5.dir/main.cpp.o
/usr/lib/ruby/vendor_ruby/sass/util.rb:1109: warning: constant ::Fixnum is deprecated
[100%] Linking CXX executable gtkbreeze5.5
[100%] Built target gtkbreeze5.5
/usr/lib/ruby/vendor_ruby/sass/util.rb:1109: warning: constant ::Fixnum is deprecated
/usr/lib/ruby/vendor_ruby/sass/util.rb:1109: warning: constant ::Fixnum is deprecated
[100%] Built target Breeze-gtk
/usr/lib/ruby/vendor_ruby/sass/util.rb:1109: warning: constant ::Fixnum is deprecated
mv: cannot stat 'assets': No such file or directory
src/CMakeFiles/Breeze-dark-gtk.dir/build.make:57: recipe for target 'src/CMakeFiles/Breeze-dark-gtk' failed
make[2]: *** [src/CMakeFiles/Breeze-dark-gtk] Error 1
CMakeFiles/Makefile2:215: recipe for target 'src/CMakeFiles/Breeze-dark-gtk.dir/all' failed
make[1]: *** [src/CMakeFiles/Breeze-dark-gtk.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

It seems to be the issue with parallel Make. The original scripts often overwrite sources, so they can't be built in parallel. I'll try to fix it to resolve the race conditions.

Also, overwriting sources is a no-no. :/

If this is a pre-existing bug, perhaps we should fix it in another patch though.

Also, overwriting sources is a no-no. :/

If this is a pre-existing bug, perhaps we should fix it in another patch though.

What does it mean? Should I create a new Phabricator revision or so?

This bug did exist in the repo I took as a base (https://github.com/dirruk1/gnome-breeze). The scripts from there overwrote the sources.

Now I am almost ready with fixing this issue. This involves changing Python script a liitle to make it write gtk2rc and global.scss into custom locations.

Also, overwriting sources is a no-no. :/

So, I think, it's even better to remove Breeze-gtk and Breeze-dark-gtk directories from the sources (because they are regenerated each time). Is it OK?

gepardo updated this revision to Diff 48223.Dec 26 2018, 6:53 PM

Summary of changes done:

  • Fix race condition during the build
  • Prevent the build process from writing into the source directory
  • Style fixes in build_theme.sh
  • Improve parameter parsing in build_theme.sh
  • Remove gtkrc, at it is autogenerated anyway
  • Pycodestyle fixes

Thanks, I can verify that the race condition is fixed so parallel building now works! But files in the source repo are still overwritten during the build process if you do an in-source build. If this is not easily resolvable, you could just disallow in-source builds in the CMake file (see Kirigami repo for an example of how).

But files in the source repo are still overwritten during the build process if you do an in-source build.

Breeze-gtk and Breeze-dark-gtk are ovewritten regardless of whether an in-source build in used or not. I'm going to fix it now by removing compiled files from the repo (located in Breeze-gtk and Breeze-dark-gtk)

gepardo updated this revision to Diff 48224.Dec 26 2018, 8:40 PM

Remove compiled themes from the sources; now the building process doesn't modify the sources at all

There we go, much better! Thanks for all the work on this. Now we can move on to the next part of the review. :)

Hello, is there any progress on the review?

It's on my to-do list, but please have some patience. :) Most people are probably still out for their winter holiday vacations.

As a base, I used this repository: https://github.com/dirruk1/gnome-breeze

Dirruk is also the author who moved this repo from SCSS to having all the assets, especially annoyingly that commit has no description on why. Would be good if we hear from him. Generated CSS certainly seems sensible.

Hello, is there any progress on the review?

There's a lot to review, practically every commit since June 2016 :/
I went through 6 at random and they all seemed correct which is very encouraging.

FWIW I've applied this and have been using it in my day-to-day, and so far I haven't found any problems.

krop added a subscriber: krop.Dec 31 2018, 12:58 PM
krop added inline comments.
CMakeLists.txt
20

This doesn't exist in CMake 2.8.12. Use

if(NOT CMAKE_VERSION VERSION_LESS 3.12.0)

or invert the if/else to avoid the 'NOT'

cmake/FindSass.cmake
2

Missing doc & license

gepardo added inline comments.Dec 31 2018, 7:01 PM
cmake/FindSass.cmake
2

Which one should I choose? Is BSD 3-clause, that is used by Extra CMake modules, OK?

gepardo updated this revision to Diff 48462.Dec 31 2018, 7:18 PM

CMake fixes:

  • Compatibility with older versions
  • Add docs and copyright into FindSass.cmake
gepardo marked 3 inline comments as done.Dec 31 2018, 7:18 PM

The install target is installing to ./Breeze and ./Breeze-Dark in addition to the CMAKE_INSTALL_PREFIX location, which messes up the permissions in your source checkout if you do an in-source build and run sudo make install.

The install target is installing to ./Breeze and ./Breeze-Dark in addition to the CMAKE_INSTALL_PREFIX location, which messes up the permissions in your source checkout if you do an in-source build and run sudo make install.

Got it. The issue is more compilcated, though. Breeze and Breeze-Dark directories are created during the build. They are not real targets currently, so are build each time (even during the install). So, when installed as root, they are modified. I will think of how to fix this issue.

Sorry for these issues with the building process, I'm not familar with it much. But at least I hope there're no such issues with the theme itself.

No worries! CMake can be a bit challenging. :)

bshah edited subscribers, added: bshah; removed: Sysadmin.Jan 1 2019, 6:19 AM

(removing sysadmin from subscribers)

gepardo updated this revision to Diff 48474.Jan 1 2019, 1:26 PM

Prevent the theme from rebuilding each time

Thanks, much better now! Will resume testing the actual user interface now.

ngraham accepted this revision.Jan 8 2019, 5:44 PM

This has been working fine in my testing. I haven't noticed any overt regressions. Technically it looks fine to me too now. It would be nice to get some more testing from others before shipping it though--especially people who use a lot of GTK apps.

This revision is now accepted and ready to land.Jan 8 2019, 5:44 PM
davidedmundson accepted this revision.Jan 8 2019, 6:09 PM

We should try to see how we can merge this properly

Effectively it's a revert - and there's relevant tracking in the github history here. I'd rather not just have a massive patch that changes everything.

Hmm, so what's our path forward here? What's the best way to merge this while preserving some semblance of history and state?

I've git filter-branch'd Alex's github branch so the root commit's now match ours.

Then merged that into master.
History graph still looks like quite a mess, but then that's a reflection of reality :)


I'll wait a day for some other git people to comment, then will push it.

Awesome, thanks a lot David!

Merged.

This didn't close because we had to disable the commit hooks.

There are several issues with this patch, which cause build failures:

  • It requires PyCairo (python3-cairo in openSUSE) but the dependency isn't checked anywhere
  • It requires the Breeze style to be installed to load the color schemes but the dependency isn't checked anywhere
  • There are decoding errors in the Python script because it doesn't take into account translations

As shown by the falures in the CI.

  • There are decoding errors in the Python script because it doesn't take into account translations

Fixed in https://commits.kde.org/breeze-gtk/cf5f675799a35687dd1fd4a13a4b80a860b71935.

  • It requires the Breeze style to be installed to load the color schemes but the dependency isn't checked anywhere

This should be hopefully fixed by https://commits.kde.org/breeze-gtk/92978bb2a243fdca5f58fa231b425bbe1802b231

https://invent.kde.org/sysadmin/ci-tooling/commit/d3443ccf5b0233f4fd0b19c93d8332046b616f58 adds the PyCairo dependency to the CI, although the build system must still check for it (as it doesn't now).

davidedmundson added inline comments.Jan 12 2019, 8:19 AM
src/build_theme.sh
90–99

We can't assume '/usr/share'/
Nor can we use the install prefix for where this is installed as that breaks distro packaging.

Breeze needs to export this location in it's installed .cmake file, then we should reference that.

I would also scrap all the stuff about ~/.local.

I would also scrap all the stuff about ~/.local

Are there any strong reasons?

Reasons for not removing ~/.local are the following. KDE still doesn't have custom color schemes support for Breeze-GTK. But build-theme.sh allows to rebuild the theme with custom color scheme and by default install it into ~/.local/share/themes. As you can see, it can read the colors from kdeglobals and color schemes (local and system-wide). (BTW that's how I'm using the theme: I just run build_theme.sh when changing color scheme)

I do agree that's a temporary hack there should be a user-friendly way to change the colors. But I think there no reason to remove this stuff now.

Maybe this way to rebuild the theme can be mentioned in README also.

Are there any strong reasons?

Not everyone has their XDG paths set in ~/.local, even more so when doing development.

In addition, I had a look at the script. While I do stuff in Python for a living (and for many years) I could not figure out what was the purpose of the classes, the functions, and the last part of the generation of the theme is a bunch of calls without much context. In short, this needs to be documented, and clearly, otherwise it'll be treated as a magic black box and the moment something breaks, no one will know how to fix it.

Yes, I'm to blame because I didn't look at it sooner.

Are there any strong reasons?

It's not installed, so a user can't use it.

That means it's only a build tool. As it's a build tool it's very important to not get stuff from the host system as it makes things less reproducible.

krop added a comment.Jan 14 2019, 11:45 AM

Are there any strong reasons?

It's not installed, so a user can't use it.

That means it's only a build tool. As it's a build tool it's very important to not get stuff from the host system as it makes things less reproducible.

at the very least, it should use XDG_DATA_HOME by default and ${HOME}/.local/share as a fallback (https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)