Thanks for the review, Yuri!
Thanks for the review and testing, Martin!
Nicely fixed, thanks Nikita!
Thanks. Nice fixes.
This is one of the steps towards the release.
Updates to ChangeLog, NEWS and features.docbook describing changes from v2.6 to v2.7 will go to a separate review.
Sun, Apr 22
Didn't close automatically, probably because it was reopened earlier.
Toni, please accept this revision if you think it's good to merge with master.
Sat, Apr 21
Thanks Toni and Martin for extensive testing!
To reproduce this issue: set a dark colours theme and oxygen icons, close all tabs in one panel and observe disabled icon in Window menu's Close Current Tab action. Try this with master
branch and with fix-missing-icons branch.
Fri, Apr 20
Same here, replicated with attached krusaderrc. And it no longer happens with this patch. Thanks, Nikita! :)
Sorry for my late response. Thanks Nikita for fixing all the issues and Toni for useful testing screenshots.
Using Ubuntu 18.04 daily build: I could reproduce the problem, and it was solved by the patch made by Nikita. Thanks, Nikita!
Thu, Apr 19
Thanks for testing, Toni!
Martin, please let me know if you spot any problem.
Hello! I can't reproduce the "fixed invisible column" issue, so I attach some screenshots and "krusaderrc" files, if it may help:
With the latest improvements made by Nikita, In an Ubuntu 18.04 daily build virtual machine, the icons were shown as it was expected:
In case you are eager to repro the problem I faced, here is excerpt from my config:
Wed, Apr 18
Resolved all outstanding issues, please check on your end.
Also updated 'icon-missing' icon — please let me know if you like it better or not. If you have a better icon, please attach a particular svg/svgz file you're interested in.
- Fixed missing icons in the file list (KrView)
- Updated icon-missing icon to be similar in style to the Breeze icon theme
- Added dynamic selection of light or dark Breeze fallback theme
- Replaced KIconLoader::loadMimeTypeIcon with Icon
- Added workaround for Breeze variant selection based on theme lightness
- Icon: improved a method name and a comment
Mon, Apr 16
It works if user specifies breeze-dark as the fallback theme in settings. I agree it would be nice to do this automatically. Your patch should work but also we need to make default of the fallback theme in settings empty, because the user's fallback theme takes precedence. Would you like to push it under your name to the remote branch?
Sun, Apr 15
Regarding the greyed out icons in Krusader: I believe this is because "breeze" is set
as system theme (not "breeze-dark") and fallback is also set as "breeze", so
there is no "breeze-dark" to fallback to.
Thanks Nikita for your big work! :) It works nicely. I have 2 observations:
- choosing dark theme not working in dark environment - I'm proposing a simple solution - please see my code comment
- icon "application-x-cmakecache" (e.g. for file CMakeCache.txt inside krusader/build folder) is now rendered as a fallback icon (even with breeze) although previously it was rendered as "unknown"
- I'm not sure if this is even solvable, or if it is important; I was currently unable to come up with a solution
Fixing revision diff.
Hi Toni, Thanks a lot for testing.
Sat, Apr 14
Then the appearance of Krusader is improved when using Ubuntu 18.04 daily build (without having Breeze nor Oxygen installed):
Note: That "icon-missing.svgz" file can be downloaded from https://phabricator.kde.org/file/download/ieoxfxmcslybdhrcwkhf/PHID-FILE-zues6xeejyrafgjglyel/icon-missing.svgz
Thanks, Nikita! A lot of work! I also wanted to say that, using Ubuntu 18.04 beta, I saw that error message when building Krusader (the "icon-missing.svgz" file was not found):
[ 84%] Built target krusader_autogen make: *** No hay ninguna regla para construir el objetivo '/home/user/krusader/krusader/icons/icon-missing.svgz', necesario para 'krusader/qrc_resources.cpp'. Alto. CMakeFiles/Makefile2:222: recipe for target 'krusader/CMakeFiles/krusader.dir/all' failed make: *** [krusader/CMakeFiles/krusader.dir/all] Error 2 Makefile:140: recipe for target 'all' failed make: *** [all] Error 2
Alright, this is it. Please help to test.
- Fixed names of variables, functions, members, methods related to icons
- Icon: Added overlay support and replaced KDE::icon in the project
Fri, Apr 13
I implemented the rest and fixed all the issues I was aware of. As you may notice from the commit message headers, Icon class now unifies loading with QIcon, KIconLoader (aka krLoader that is gone now), ICON macro, FL_LOADICON, SmallIcon etc. Unfortunately, a small number of places were not converted due to the use of overlays. I plan to work on this a little later — I wanted to update the CR early so you can provide feedback.
- Replaced QIcon::fromTheme with Icon instances
- Added loading fallback icon from resource file
- Fixed icons in User Action Examples
- Implemented icon cache and system theme change handling
- + Konfigurator option to specify a fallback icon theme
- Switched app icon to Icon
- Replaced QIcon::hasThemeIcon calls with Icon::exists; refactored icon search
- Fixed icons on Synchronizer buttons
- Switched krLoader->loadIcon to Icon in case it's used as QIcon
- Switched krLoader->loadIcon to Icon in case it's used as QPixmap
- Switched from KIconLoader to Icon in ActionMan
- Fixed misuse of FL_LOADICON
- Replaced FL_LOADICON with FileListIcon::pixmap
- Replaced SmallIcon with Icon
- Used KIconLoader in icon search to improve mime icons
- Icon: improved comments and messages
- Removed KrGlobal::iconLoader and cleaned up KIconLoader includes
- Icon: don't apply fallback in case of empty icon name
- Fixed icon for dragging multiple items between panels
Tue, Apr 10
Mon, Apr 9
I'm fine with "emblem-unreadable" although my preference would rather be "image-missing" :).
Sun, Apr 8
Maybe we need to use something neutral like circle or diamond... With these missing/unreadable/broken icons the app looks like a broken one while it's not...
Also, I guess we better save a particular image we pick into the resource file, so we can load it for sure. Are you fine with it?
Same goes for me, sorry for not replying. The patch works as expected :).