D17154: Go back to SCSS

cgiboudeaux added a comment. In D17154#392806 , @davidedmundson wrote: > > 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 n

D17154: Go back to SCSS

davidedmundson added a comment. > 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. REPOSITORY R98 Breeze

D17154: Go back to SCSS

lbeltrame added a comment. In D17154#391812 , @gepardo wrote: > 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

D17154: Go back to SCSS

gepardo added a comment. > 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 c

D17154: Go back to SCSS

davidedmundson added inline comments. INLINE COMMENTS > build_theme.sh:89-98 > +COLOR_SCHEME="/usr/share/color-schemes/Breeze.colors" > + fi > +else > + THEME_NAME="${COLOR_SCHEME}" > + if [ -f "/usr/share/color-schemes/${COLOR_SCHEME}.colors" ]; then > +COLOR_SCHEME="/usr/share/color-

D17154: Go back to SCSS

lbeltrame added a comment. 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). REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phab

D17154: Go back to SCSS

lbeltrame added a comment. In D17154#391718 , @lbeltrame wrote: > > - 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://

D17154: Go back to SCSS

lbeltrame added a comment. In D17154#391718 , @lbeltrame wrote: > > - There are decoding errors in the Python script because it doesn't take into account translations Fixed in https://commits.kde.org/breeze-gtk/cf5f675799a356

D17154: Go back to SCSS

lbeltrame added a comment. 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

D17154: Go back to SCSS

davidedmundson closed this revision. davidedmundson added a comment. Merged. This didn't close because we had to disable the commit hooks. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd, d

D17154: Go back to SCSS

ngraham added a comment. Awesome, thanks a lot David! REPOSITORY R98 Breeze for Gtk BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd, davidedmundson Cc: bshah, cgiboudeaux, dav

D17154: Go back to SCSS

davidedmundson added a comment. 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 p

D17154: Go back to SCSS

ngraham added a comment. Hmm, so what's our path forward here? What's the best way to merge this while preserving some semblance of history and state? REPOSITORY R98 Breeze for Gtk BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL https://phabricator.kde.org/D17154 To: ge

D17154: Go back to SCSS

davidedmundson accepted this revision. davidedmundson added a comment. 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. REPOSITORY

D17154: Go back to SCSS

ngraham accepted this revision. ngraham added a comment. This revision is now accepted and ready to land. 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 sh

D17154: Go back to SCSS

ngraham added a comment. Thanks, much better now! Will resume testing the actual user interface now. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd Cc: bshah, cgiboudeaux, davidedmundson, bcook

D17154: Go back to SCSS

gepardo updated this revision to Diff 48474. gepardo added a comment. Prevent the theme from rebuilding each time REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48462&id=48474 BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL

D17154: Go back to SCSS

bshah edited subscribers, added: bshah; removed: Sysadmin. bshah added a comment. (removing sysadmin from subscribers) REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd Cc: bshah, cgiboudeaux, dav

D17154: Go back to SCSS

ngraham added a comment. No worries! CMake can be a bit challenging. :) REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd Cc: cgiboudeaux, davidedmundson, bcooksley, #sysadmin, ngraham, jackg, pl

D17154: Go back to SCSS

gepardo added a comment. In D17154#384519 , @ngraham wrote: > 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

D17154: Go back to SCSS

ngraham added a comment. 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`. REPOSITORY R98 Breeze for Gtk

D17154: Go back to SCSS

gepardo updated this revision to Diff 48462. gepardo added a comment. CMake fixes: - Compatibility with older versions - Add docs and copyright into FindSass.cmake REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48224&id=48462 BRANCH

D17154: Go back to SCSS

gepardo added inline comments. INLINE COMMENTS > cgiboudeaux wrote in FindSass.cmake:1 > Missing doc & license Which one should I choose? Is BSD 3-clause, that is used by //Extra CMake modules//, OK? REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gep

D17154: Go back to SCSS

cgiboudeaux added inline comments. INLINE COMMENTS > CMakeLists.txt:14 > > -include(KDEInstallDirs) > +if(${CMAKE_VERSION} GREATER_EQUAL 3.12.0) > +find_package(Python3 COMPONENTS Interpreter REQUIRED) This doesn't exist in CMake 2.8.12. Use if(NOT CMAKE_VERSION VERSION_LESS 3.12.0) or

D17154: Go back to SCSS

ngraham added a comment. FWIW I've applied this and have been using it in my day-to-day, and so far I haven't found any problems. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham, dirrukd Cc: davidedmund

D17154: Go back to SCSS

davidedmundson added a comment. > 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.

D17154: Go back to SCSS

ngraham added a comment. It's on my to-do list, but please have some patience. :) Most people are probably still out for their winter holiday vacations. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham C

D17154: Go back to SCSS

gepardo added a comment. Hello, is there any progress on the review? REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham Cc: bcooksley, #sysadmin, ngraham, jackg, plasma-devel, GB_2, ragreen, Pitel, ZrenBot

D17154: Go back to SCSS

ngraham added a comment. There we go, much better! Thanks for all the work on this. Now we can move on to the next part of the review. :) REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham Cc: bcooksley, #

D17154: Go back to SCSS

gepardo updated this revision to Diff 48224. gepardo added a comment. Remove compiled themes from the sources; now the building process doesn't modify the sources at all REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48223&id=48224 BRANCH

D17154: Go back to SCSS

gepardo added a comment. > 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 fi

D17154: Go back to SCSS

ngraham added a comment. 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

D17154: Go back to SCSS

gepardo updated this revision to Diff 48223. gepardo added a comment. 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 - Re

D17154: Go back to SCSS

gepardo added a comment. In D17154#382450 , @ngraham wrote: > 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 Phabr

D17154: Go back to SCSS

ngraham added a comment. Also, overwriting sources is a no-no. :/ If this is a pre-existing bug, perhaps we should fix it in another patch though. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze, #vdg, ngraham Cc

D17154: Go back to SCSS

gepardo added a comment. 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. REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To

D17154: Go back to SCSS

ngraham added a comment. 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: c

D17154: Go back to SCSS

bcooksley added a comment. Thanks @ngraham. I've now scheduled the addition of `scss` into the images with 63d16727883e9c9f9d09bdd2bc7163fb386c0a57 REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricat

D17154: Go back to SCSS

gepardo updated this revision to Diff 48156. gepardo added a comment. Add gtk-dark.css support for Breeze REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48153&id=48156 BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL https:

D17154: Go back to SCSS

gepardo updated this revision to Diff 48153. gepardo added a comment. Fix GTK+ 3.18 theme installation REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48148&id=48153 BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL https://p

D17154: Go back to SCSS

gepardo added a comment. 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 `

D17154: Go back to SCSS

ngraham added a subscriber: Sysadmin. ngraham added a comment. 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,

D17154: Go back to SCSS

gepardo updated this revision to Diff 48148. gepardo marked an inline comment as done. gepardo added a comment. Fix finding Python package for CMake <3.12 REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48145&id=48148 BRANCH breeze-gtk-sass

D17154: Go back to SCSS

gepardo marked 2 inline comments as done. gepardo added inline comments. INLINE COMMENTS > ngraham wrote in CMakeLists.txt: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 NO

D17154: Go back to SCSS

gepardo updated this revision to Diff 48145. gepardo added a comment. Make Sass a required dependency REPOSITORY R98 Breeze for Gtk CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D17154?vs=48031&id=48145 BRANCH breeze-gtk-sass (branched from master) REVISION DETAIL https://ph

D17154: Go back to SCSS

ngraham added inline comments. INLINE COMMENTS > CMakeLists.txt:12 > find_package(GTKEngine) > +find_package(Sass) > +find_package(Python3 REQUIRED) Shouldn't this be required since you can't build the theme without it? > CMakeLists.txt:13 > +find_package(Sass) > +find_package(Python3 REQUIRED

D17154: Go back to SCSS

gepardo updated this revision to Diff 48031. gepardo added a comment. More imrovements: - 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 REPOSITORY R98 Breeze for Gt

D17154: Go back to SCSS

gepardo updated this revision to Diff 48030. gepardo added a comment. 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 &

D17154: Go back to SCSS

gepardo added a comment. In D17154#380574 , @ngraham wrote: > 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 t

D17154: Go back to SCSS

ngraham added a reviewer: VDG. ngraham requested changes to this revision. ngraham added a comment. This revision now requires changes to proceed. @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

D17154: Go back to SCSS

gepardo added a comment. This patch is not reviewed for two weeks. How soon can I get some feedback on it? REPOSITORY R98 Breeze for Gtk REVISION DETAIL https://phabricator.kde.org/D17154 To: gepardo, #plasma, jackg, #breeze Cc: ngraham, jackg, plasma-devel, ragreen, Pitel, ZrenBot, les

D17154: Go back to SCSS

ngraham added subscribers: jackg, ngraham. ngraham added reviewers: Plasma, jackg, Breeze. ngraham added a comment. @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'

D17154: Go back to SCSS

gepardo created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. gepardo requested review of this revision. REVISION SUMMARY As it was explained in https://phabricator.kde.org/D16365, > Further potential steps in the same direction of saving code would