Bug 445489

Summary: Light theme icons randomly get installed in breeze-dark theme
Product: Breeze Reporter: Andreas Ferber <af+kde>
Component: IconsAssignee: Rodney Dawes <dobey.pwns>
Severity: normal CC: dobey.pwns, kainz.a, KDE, med.medin.2014, mk47blast, mo78, nate, RaitaroHikami
Priority: NOR Keywords: regression
Version: master   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
See Also: https://bugs.kde.org/show_bug.cgi?id=444095
Latest Commit: Version Fixed In: 5.89

Description Andreas Ferber 2021-11-14 14:51:27 UTC

After installation the `breeze-dark` theme randomly contains icons from the `breeze` (light) theme instead of the actual dark mode icons.

The problem was introduced by the following two commits:

* https://invent.kde.org/frameworks/breeze-icons/-/commit/d9a5af79a559a2ecff118e580bd560f2d50353c4
* https://invent.kde.org/frameworks/breeze-icons/-/commit/dda2316fc37156366def936d04029463087a153c

The intention seems to be to install icons from the light theme into the dark theme that are missing from the dark theme (eg. `actions/16/color-management.svg` exists in `icons` but not in `icons-dark`).  This was done by installing the light icons into the destination first probably with the expectation that any icons that actually exist in the dark theme will then get overwritten with the dark version.

However, cmake when installing files only compares timestamps when deciding whether a file is already up-to-date in the destination, and skips copying the file if the timestamps match (unless the `CMAKE_INSTALL_ALWAYS` environment variable is set). It *does not* compare file size or contents! The result is that whenever the light theme icon file and the corresponding file in the dark theme happen to have the same timestamp (as is often the case as git doesn't preserve timestamps when checking out files, so if you say do a fresh clone on a fast disk where the checkout runs in under a second all files have the same timestamp) then the "preseeded" version from the light theme doesn't get overwritten with the dark theme version, so you end up with the light version of the icon in the installed theme.


Run the following commands in a shell:

git clone https://invent.kde.org/frameworks/breeze-icons.git demo-breeze-icons-bug
cd demo-breeze-icons-bug
# ensure two files have same timestamp
touch -r icons/actions/12/object-fill.svg icons-dark/actions/12/object-fill.svg
mkdir build inst
cd build
cmake ..
make  install DESTDIR=../inst
cd ../inst
diff -u usr/share/icons/breeze{,-dark}/actions/12/object-fill.svg


The diff at the end of the commands above doesn't output anything, ie. the files are identical.


The diff should show the same differences as if you made a diff between `icons/actions/12/object-fill.svg` and `icons-dark/actions/12/object-fill.svg` in the source tree:

--- icons/actions/12/object-fill.svg    2021-11-14 15:35:50.214936474 +0100
+++ icons-dark/actions/12/object-fill.svg       2021-11-14 15:35:50.214936474 +0100
@@ -1,11 +1,11 @@
 <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="12" height="12" id="svg3049">
     <defs id="defs3051">
         <linearGradient id="linearGradient4234">
-            <stop style="stop-color:#232629;stop-opacity:1" offset="0" id="stop4236"/>
+            <stop style="stop-color:#eff0f1;stop-opacity:1" offset="0" id="stop4236"/>


Digging through cmake it seems that the environment variable mentioned above (CMAKE_INSTALL_ALWAYS) is the only way that "optimization" in cmake can be disabled, and I don't think there's any way to set it from within CMakeLists.txt in a way that it affects the currently running cmake . So I think unless you want to tell people that they must set this variable before installing breeze icons reverting the commits listed above is the only way to fix this.
Comment 1 Andreas Ferber 2021-11-15 01:01:22 UTC
It took me a while to realize what the mentioned commits were actually trying to achieve.

The underlying issue of bug
Comment 2 Andreas Ferber 2021-11-15 01:09:31 UTC
It took me a while to realize what the mentioned commits actually were trying to achieve.

The underlying issue of bug 444095 is that according to https://specifications.freedesktop.org/icon-theme-spec/latest/ar01s05.html if an icon doesn't exist in the requested size in a theme then falling back to a different size from the same theme takes precedence over falling back to a parent theme.

According to the KDE HIG for the mimetypes and places categories monochrome icons should be used for sizes 16 and 22 while colorful icons are used from 32 upwards. The breeze-dark theme only contains the monochrome sizes of those icons, and because of the lookup rules those were then used for all sizes. So what Rodney Dawes was trying to do is installing those "colorful" sizes from the breeze theme into the breeze-dark theme as well, but his solution copies a lot more than just that.
Comment 3 Andreas Ferber 2021-11-15 02:28:51 UTC
After further investigation I think https://invent.kde.org/frameworks/breeze-icons/-/commit/1b92cfc450f6ab6b72ed9ef69c052e4624e5a040 needs to be reverted too, as that appears to be the root cause of bug 444095 and the ill-fated attempt to fix it. Because of the icon lookup rules mentioned in my previous comment the way duplicates between icons and icons-dark were removed actually broke *all* icons where only certain sizes have tweaked dark-mode versions.

I thiink the only clean way to achieve the goal of not having duplicate files between the two themes is to first rename the `icons` and `icons-dark` folders in the source tree to `breeze` and `breeze-dark` respectively in order to be able to make symlinks between the two themes that don't break when getting installed (I don't think cmake can adjust symlink targets during install), and then replacing duplicate files with symlinks. Only in cases where *all* sizes are identical between breeze and breeze-dark can the icon safely be removed from breeze-dark completely.

The alternatives would be to either make a separate list of all breeze icons that need to be installed into breeze-dark in order to make all sizes work properly, or make a complex heuristic that determines which icons need to be copied at install time. Both are IMO inferior to the symlink approach outlined above. A list would have to be maintained and is easy to miss when adding new tweaked icons to the dark theme. A heuristic OTOH would most likely be fragile and prone to future breakage. And both approaches would only remove the duplicates in the source tree, the installed versions of the themes would still have duplicated files while the symlink approach would remove duplicates there as well.
Comment 4 Bug Janitor Service 2021-11-15 15:50:28 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/breeze-icons/-/merge_requests/171
Comment 5 Rodney Dawes 2021-11-15 15:53:42 UTC
A quick fix for this issue is to use the `CODE` option of CMake's `install` command, so that we instead do the copy ourselves, and ignore the timestamps when copying.

I've opened a PR which does this.

Renaming the directories in the repository and creating a symlink farm would be better for the installed themes, as it would take up much less disk space, but maintaining it in repo would be nearly as daunting as having to ensure duplicate icons are kept updated in the repo as well.
Comment 6 Antonio Rojas 2021-11-29 19:22:34 UTC
*** Bug 446253 has been marked as a duplicate of this bug. ***
Comment 7 Nate Graham 2021-12-01 04:28:06 UTC
*** Bug 446293 has been marked as a duplicate of this bug. ***
Comment 8 Nate Graham 2021-12-02 17:53:46 UTC
*** Bug 446383 has been marked as a duplicate of this bug. ***
Comment 9 Rodney Dawes 2021-12-03 18:00:36 UTC
Git commit 250a7d0bbe6dd8e7d2e7cba4cc82a2d8cce37cf1 by Rodney Dawes.
Committed on 03/12/2021 at 17:49.
Pushed by ngraham into branch 'master'.

Install dark icons via execute_process to use copy command

In order to ensure the dark icons are copied over the light icons which
are installed into the dark theme for BUG:444095, we need to execute the
copy command ourselves rather than using CMake's internal copy mechanism
used by the normal install command, so we use CODE mode of install to
execute_process and print an appropriate status message.

M  +28   -2    icons-dark/CMakeLists.txt