D27557: Auto-generate 24px monochrome icons

davidhurka added a comment. In D27557#616831 , @sitter wrote: > > - if your script is doing a lot of stuff you may want to revise how the script works. specifically for this diff sed needs to iter all the content of all the SVGs

D27557: Auto-generate 24px monochrome icons

sitter added a comment. It's doable in cmake but it'd be awkward and needlessly complex I think, you are much better served putting the actual processing into a script. ki18n's cmake/ macros have function with very similar tech. In fact, KI18N_INSTALL probably is super close in the

D27557: Auto-generate 24px monochrome icons

ngraham added a comment. Sorry, I was busy at a hackathon this weekend and am only just now catching up on KDE stuff. Thanks for the hotfix. To echo what Noah said, we'll be more careful in the future. This script probably needs to be rewritten in Python and the loop moved into CMake,

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. Sorry for all the trouble. Unfortunately, neither Nate or I have much experience with CMake or the gotchas of cross platform shellscript and Unix tool support. We'll have to be a lot more careful next time and stick to something with fewer gotchas like Python.

D27557: Auto-generate 24px monochrome icons

kossebau added a comment. Pushed 004cf12a2fdb09ab8b62e3f8d84e1584fe08ebab as intermediate hotfix to help CI and other parallel builds. The whole logic needs a clean rework though, e.g. to make sure the generated

D27557: Auto-generate 24px monochrome icons

kossebau added a comment. There is also the issue that the `breeze-generate-24px-versions` target is not set as dependency for the `breeze-validate-svg` target. As a result both targets will be processed in parallel if multiple jobs are possible. And on KDE CI, where the "build/" directory

D27557: Auto-generate 24px monochrome icons

usta added a comment. Freebsd side broken again : 00:31:45 ./build/icons-dark/generated/devices/24/network-wired-activated.svg:14: parser error : Opening and ending tag mismatch: g line 8 and svg 00:31:45 00:31:45^ 00:31:45

D27557: Auto-generate 24px monochrome icons

ngraham added a comment. Thanks so much everyone! @sitter I'll read up on CMake a bit and try to implement your suggestions. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: davidre, bcooksley, kossebau,

D27557: Auto-generate 24px monochrome icons

sitter added a comment. This is now working again. The symlink tests fail though. That still needs fixing by someone who isn't me. Here's my feedback: For bash or sh scripts: - make sure a sh script is actually POSIX compliant by running it through a POSIX sh implementation

D27557: Auto-generate 24px monochrome icons

davidre added a comment. I disagree but it's not my call REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: davidre, bcooksley, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

D27557: Auto-generate 24px monochrome icons

sitter added a comment. @davidre we provide a theme, if someone explicitly looks for a specific file within that theme that'd be squarely outside the supported realm of a theme. it'd prevent improving the theme which may mean changing scaling rules and removing the then useless sizes.

D27557: Auto-generate 24px monochrome icons

davidre added a comment. Also we need to have this on Windows, too. Breeze-icons is part of Frameworks and these were exported/installed files. A user could easily have a reference to one of the files somewhere. REPOSITORY R266 Breeze Icons REVISION DETAIL

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. Should we just revert for now? REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: bcooksley, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27557: Auto-generate 24px monochrome icons

sitter added a comment. Bhushan already has D27614 for the installation problem. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: bcooksley, kossebau,

D27557: Auto-generate 24px monochrome icons

sitter added a comment. Oh, oh boy, there's many more things wrong with this than just the bashism :| @ndavis @ngraham why don't you generate the 24x version inside the source? `COMMAND ${BASH_EXE} ${CMAKE_SOURCE_DIR}/generate-24px-versions.sh ${BREEZE_INSTALL_DIR}` this is

D27557: Auto-generate 24px monochrome icons

sitter added a comment. `{}` expansions are bashism. Never ever test files meant for sh with bash and be mindful that many distributions will symlink /bin/bash to /bin/sh. There is a huge amount of not so subtle syntax differences between POSIX sh and bash that an sh-only implementation

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. looks like it's not posix sh, but bash code REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: bcooksley, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

D27557: Auto-generate 24px monochrome icons

bcooksley added a comment. Please note that the FreeBSD breakage has now started to cause Dependency Builds on the CI system to fail. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: bcooksley, kossebau,

D27557: Auto-generate 24px monochrome icons

kossebau added a comment. Seems FreeBSD has an issue with the generation code. Please check https://build.kde.org/view/Failing/job/Frameworks/job/breeze-icons/job/kf5-qt5%20FreeBSDQt5.13/135/console REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To:

D27557: Auto-generate 24px monochrome icons

This revision was automatically updated to reflect the committed changes. Closed by commit R266:d0582a4f16bd: Auto-generate 24px monochrome icons (authored by ngraham). REPOSITORY R266 Breeze Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27557?vs=76228=76229 REVISION DETAIL

D27557: Auto-generate 24px monochrome icons

ngraham updated this revision to Diff 76228. ngraham added a comment. Rebase (thanks for the icon fixes, @ndavis) REPOSITORY R266 Breeze Icons CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D27557?vs=76129=76228 BRANCH autogenerate-24px-icons (branched from master) REVISION

D27557: Auto-generate 24px monochrome icons

ndavis accepted this revision. ndavis added a comment. This revision is now accepted and ready to land. After you rebase this, I'd say this patch is ready to go. REPOSITORY R266 Breeze Icons BRANCH autogenerate-24px-icons (branched from master) REVISION DETAIL

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. I've added the icons that were only in actions/24 to actions/22. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. I've fixed the 22px files that had height/width and viewbox set. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham,

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. The following 24px icon symlinks are linked to 22px icons. Maybe it's material for another patch, but I figured I'd bring it up. configure-shortcuts.svg -> ../../devices/22/input-keyboard.svg dblatex.svg -> ../../places/22/network-server-database.svg

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. `draw-highlight` got a bit mangled: F8124601: Screenshot_20200222_043230.PNG I think it's because both the viewbox and the height/width were set, which made the scaling at 22px equal to 0.26459. This looks fine when we're

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. Out of the ones listed above, these are symlinks: 24/font.svg 24/gnumeric-format-border-all.svg 24/gnumeric-format-border-none.svg 24/gnumeric-format-border-outside.svg 24/gnumeric-object-label.svg 24/gnumeric-object-line.svg

D27557: Auto-generate 24px monochrome icons

ndavis added a comment. Looking pretty good so far. The only icons that have 24px versions but no 22px versions are the following from the actions category: 24/align-horizontal-node.svg 24/align-vertical-node.svg 24/audio-volume-high.svg 24/audio-volume-low.svg

D27557: Auto-generate 24px monochrome icons

ngraham edited the summary of this revision. REPOSITORY R266 Breeze Icons REVISION DETAIL https://phabricator.kde.org/D27557 To: ngraham, #vdg, ndavis, #frameworks, sitter Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27557: Auto-generate 24px monochrome icons

ngraham added inline comments. INLINE COMMENTS > generate-24px-versions.sh:45 > +fi > +done comment to make this file visible for reviewers > CMakeLists.txt:26 > +endif() > + > gtk_update_icon_cache(${BREEZE_INSTALL_DIR}) comment to make the file visible for reviewers REPOSITORY R266

D27557: Auto-generate 24px monochrome icons

ngraham created this revision. ngraham added reviewers: VDG, ndavis, Frameworks, sitter. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. ngraham requested review of this revision. REVISION SUMMARY Right now we have 24px monochrome icons for compatibility