Use system file type specific icons in contents tab #6156

zeule · 2016-12-30T00:20:12Z

Again a bit of eye-candy. Torrent contents tab shows icons for items, but icons of two types only: directory and text file. But we can get correct icon from the system mime database. The icon is determined via QFileIconProvider using filename extension only.

Perhaps default icon without mime database should be changed to "application/octet-stream" too.

okeatime · 2016-12-31T05:40:52Z

Sample screen shot please?

zeule · 2016-12-31T15:32:42Z

Please:

zeule · 2017-01-03T20:32:36Z

PR Updated: replace QMimeDatabase with QFileIconProvider that results in Qt 4 and all OSes support.

okeatime · 2017-01-04T06:18:04Z

👍 good job

sledgehammer999 · 2017-01-18T20:51:47Z

I was so pumped but unfortunately this doesn't work on Windows with Qt 5.7.1(didn't test qt4).
It seems to return an empty image.

sledgehammer999 · 2017-01-18T20:58:40Z

Possible explanation here. It seems QFileIconProvider works if the file actually exists on disk.

sledgehammer999 · 2017-01-18T21:01:31Z

And a stale Qt bug report: https://bugreports.qt.io/browse/QTBUG-25319

zeule · 2017-01-18T23:25:50Z

This is pity, of course. The feature is really nice and useful.... Except of patched Qt for the Windows builds or custom implementation of the same function I have no other ideas so far.

sledgehammer999 · 2017-01-18T23:36:35Z

The only choice here it to do this only for the platforms that support it and fallback to our current code for Windows. I'm in no mood to implement it in WINAPI.

Supposedly macOS should work too.

zeule · 2017-01-19T00:06:37Z

@okeatime, can you tell us, please, does it work in OSX?

thalieht · 2017-04-08T21:00:00Z

src/gui/torrentcontentmodel.h

+    virtual QVariant headerData(int section, Qt::Orientation orientation, int role) const;
+    virtual QModelIndex index(int row, int column, const QModelIndex &parent = QModelIndex()) const;
+    virtual QModelIndex parent(const QModelIndex &index) const;
+    virtual int rowCount(const QModelIndex &parent = QModelIndex()) const;


override's instead of virtual? No class seems to inherit from this one.

I just reformatted the code, that was written before C++11.

Yea, was just wondering if you want to change it.

Nope, sorry. Not in this PR.

zeule · 2017-04-20T16:28:22Z

PR Updated: add a naïve WIN32 API based implementation for Windows.

zeule · 2017-04-20T16:40:54Z

@Chocobo1, @glassez: could you help me to fix qmake build on Windows, please?

zeule · 2017-04-20T17:15:08Z

Will this feature work for 64-bit qBittorrent on Windows?

Yes, it does.

zeule · 2017-04-20T17:17:55Z

@Chocobo1, @glassez: could you help me to fix qmake build on Windows, please?

Apparently, Qt build from cache in AppVeyor does not contain qwinextras module (why qmake is silent on this during its run?). If I use pre-installed Qt (see the last commit) it compiles, but fails during linking because QtSingleApplication was linked with static runtime (BTW, why?) while the pre-installled Qt is linked with dynamic one. I give up.

zeule · 2017-04-20T17:23:51Z

I can do kind of a hack: declare

Q_GUI_EXPORT QPixmap qt_pixmapFromWinHICON(HICON icon);

(which is exported from QtGui library) and use it without QWinExtras module. What do you think?

zeule · 2017-04-20T19:27:00Z

The hack is in its own commit now.

zeule · 2017-04-20T19:37:03Z

@sledgehammer999, I think we need your, as Windows packages, opinion here regarding this hack: would you prefer to use the additional Qt module WinExtras (albeit very small one) and update AppVeyor cached Qt package to include it, or use this hack? Actually, the hack is identical to the internals of WinExtras module.

zywo · 2017-04-20T20:37:13Z

Just tested on Ubuntu 17.04 and I don't know why it's not working.

zeule · 2017-04-20T20:42:10Z

Thanks, will check one more time a bit later.

Chocobo1 · 2017-04-21T04:33:40Z

would you prefer to use the additional Qt module WinExtras (albeit very small one) and update AppVeyor cached Qt package to include it

I prefer this option.

zeule · 2017-04-21T08:08:36Z

Just tested on Ubuntu 17.04 and I don't know why it's not working.

Because I forgot that for a long time I'm applying this patch locally. Obviously, current implementation will not work outside of Plasma even if that patch get accepted. Probably, I better return to QMimeDatabase-based implementation for Linux.

zeule · 2017-04-21T11:25:08Z

@zywo, tell, please, in what environment did you test?

zywo · 2017-04-21T14:39:04Z

@evsh
Tested under Unity7 and Gnome DEs.

sledgehammer999 · 2017-07-02T22:01:08Z

…if we remove the option, the bug will be gone too.

+1

Just wait a bit, I'm in the process of building+uploading new package for appveyor with WinExtras. Then you can also remove the winextras check too.

zeule · 2017-07-02T22:02:37Z

Super! Could you ping me, please, when the new image is ready?

sledgehammer999 · 2017-07-02T23:33:37Z

@evsh new files are uploaded. I also updated the appveyor config you use msvc2017, so be sure to rebase on top of master.

zeule · 2017-07-03T09:57:12Z

@sledgehammer999: thanks!

PR updated: QWinExtras module is used unconditionally, icons are shown unconditionally.

sledgehammer999

Some minor things due to the rebase.
Otherwise, it is good for merging.

sledgehammer999 · 2017-07-03T12:39:47Z

src/gui/advancedsettings.cpp

@@ -37,6 +37,7 @@
 #include "base/bittorrent/session.h"
 #include "base/preferences.h"
 #include "gui/mainwindow.h"
+#include "gui/torrentcontentmodel.h"


Is this a leftover?

sledgehammer999 · 2017-07-03T12:40:45Z

src/gui/torrentcontentmodel.cpp

@@ -41,27 +50,60 @@

 namespace
 {
+    const char showSystemFileIconsSettingKey[] = "Preferences/Advanced/showSystemFileIcons";


I think this is a leftover too.

sledgehammer999 · 2017-07-03T12:52:56Z

src/gui/torrentcontentmodel.h

@@ -37,8 +37,10 @@
 #include <QVariant>

 #include "base/bittorrent/torrentinfo.h"
+#include "base/settingvalue.h"


sledgehammer999 · 2017-07-03T12:54:49Z

src/gui/torrentcontentmodel.cpp

@@ -74,6 +77,7 @@ namespace
    class WinShellFileIconProvider: public UnifiedFileIconProvider
    {
    public:
+        using QFileIconProvider::icon;


maybe this belongs to the previous commit?

sledgehammer999 · 2017-07-03T13:04:12Z

src/gui/torrentcontentmodel.cpp

+     * @brief Tests whether QFileIconProvider actually works
+     *
+     * Some QPA plugins do not implement QPlatformTheme::fileIcon(), and
+     * QFileIconProvider::icon() returns empty icons as the result. Here we asks it for


Typo: "ask"

zeule · 2017-07-03T13:28:36Z

@sledgehammer999: done. Sorry for the carelessness.

sledgehammer999 · 2017-07-03T13:34:27Z

src/gui/torrentcontentmodel.cpp

 #include "guiiconprovider.h"
+#include "base/settingsstorage.h"


I think both of us missed this.

The icon is determined via QFileIconProvider using filename extension only.

If built-in QFileIconProvider does not seem to work, use custom implementation which queries mime database.

sledgehammer999 · 2017-07-03T15:06:17Z

Thx

zeule · 2017-07-03T16:20:31Z

Thanks everyone!

zeule · 2017-07-10T08:43:18Z

Fix for QTBUG-25319 will be in Qt 5.9.2. I'll submit a corresponding PR.

sledgehammer999 · 2017-07-10T11:38:56Z

Fix for QTBUG-25319 will be in Qt 5.9.2. I'll submit a corresponding PR.

Finally. Although that Qt shortcoming was mentioned/known a long while ago. At least the fixed it.

vit9696 · 2017-07-15T19:34:54Z

@evsh I have just realised that this always returns blank icons on macOS for anything but folders.

zeule · 2017-07-16T16:44:51Z

@vit9696 thanks for pointing this out.

Unfortunately, I can't build qBt in MacOS so can not look into it right now. Maybe next week...

vit9696 · 2017-07-16T17:04:31Z

@evsh yes, it just gives iconForFile: empty path here. And that's even when the files are definitely available on disk. Cannot promise to look any soon on this either though.

vit9696 · 2017-07-19T07:53:07Z

@evsh, ok, I tracked the issue down, and the merged code simply makes no sense to me from now on.

This line building QFileInfo from the item does nothing but appends the current working directory to a relative name. To elaborate, imagine name() return debian-9.0.0-amd64-netinst.iso, in this case given working dir set to /Users/MyUser/ the newly created QFileInfo will be built with /Users/MyUser/debian-9.0.0-amd64-netinst.iso path. And the actual file location could be /Users/MyUser/Downloads/debian-9.0.0-amd64-netinst.iso.

Obviously icon lookup will always fail when the downloaded file resides in a folder different from the working directory. That's because iconForFile: needs an actual file, and you give it a completely invalid path. How does it even work on the other platforms? I do not understand why QFileInfo is even used here, especially since the detection could only happen by an extension, and since QFileInfo is believed to be slow.

What needs to be done here (especially given that one should be able to see the icons before downloading anything) is just a plain extension-based lookup. So I enforced MimeFileIconProvider, simplified it a little to avoid QFileInfo, and added some prints of the icon names and mimes:

"debian-9.0.0-amd64-netinst.iso"  has  "application/x-cd-image" mime
"application/x-cd-image"  has  "application-x-cd-image"  icon
"application/x-cd-image"  has  "application-x-generic"  genicon

So far so good, but QIcon::fromTheme from both of those returns a null icon. The docs say that this function appears to be a Linux hack compatible with some kind of Linux-only spec with an ability of gluing to a local icon database @_@. Sounds mad and of course does not work, on macOS you have iconForFileType:, which should be used in a similar way to iconForFile: :/.

Given the overall code craziness, could you please rework this? If you want to use file-based icon matching, please provide a valid file path. Otherwise this should be eliminated for good, along with the ugly test attempting to achieve nothing.

Here is a basic patch required to get things work on macOS. I could submit a pull-request with just this to make it in time for 3.4.0. Or it could be used as a reference for the refactoring of this code by somebody else.

CC @Chocobo1, @glassez, @LordNyriox, @sledgehammer999, @thalieht.

zeule · 2017-07-19T08:05:17Z

@vit9696, thanks for the investigation!

This line building QFileInfo from the item does nothing but appends the current working directory to a relative name.

This makes no sense to me. Are you saying that the QFileInfo objects contain actual full path (with the current directory for he moment of its creation)?

Obviously icon lookup will always fail when the downloaded file resides in a folder different from the working directory.

Lookup by file extension has to work with non-existing files.

That's because iconForFile: needs an actual file, and you give it a completely invalid path.

This means QTBUG-25319 affects MacOS too.

I do not understand why QFileInfo is even used here, especially since the detection could only happen by an extension, and since QFileInfo is believed to be slow.

Because it's a part of QFileIconProvider interface.

vit9696 · 2017-07-19T08:15:54Z

This makes no sense for me. Are you saying that the QFileInfo objects contain actual full path (with the current directory for he moment of its creation)?

I am only talking about absoluteFilePath() used by Qt internally and by the Windows icon lookup here. From what I can tell QFileInfo changes nothing itself and just does what you ask it. Want absolute path → working dir gets appended unless the original path has already been absolute. In this case all the files are just filenames without even a torrent subdir prefix (see the prints above).

Lookup by file extension has to work with non-existing files.

Yes, but what is the point of doing this when you already have mime-based search? This is not going to do any better unless you actually have the file at least partially downloaded, pass the valid path, have the data-based detection enabled. Otherwise it is just unnecessary code duplication.

Because it's a part of QFileIconProvider interface.

QFileIconProvider is just a spike that helps nothing when you provide invalid paths that will never become valid to it.

zeule · 2017-07-19T10:11:06Z

I am only talking about absoluteFilePath() used by Qt internally and by the Windows icon lookup here…

Thanks, understood.

Yes, but what is the point of doing this when you already have mime-based search?

Mime database does not exist on Windows, while QFileIconProvider give a uniform interface on all platforms.

QFileIconProvider is just a spike that helps nothing when you provide invalid paths that will never become valid to it.

Uniform interface.

So, QFileIconProvider::icon() does not work on MacOS for non-existing files. You probably should submit a bug report to Qt and submit a PR here basing on your investigation. Also, I would recommend you to subclass QFileIconProvider instead of hooking up with #ifdef into the MimeFileIconProvider. That would keep code cleaner and allow our descendants to easily remove the workaround when Qt fixes the problem.

vit9696 · 2017-07-19T10:34:28Z

Uniform interface.

That does not work on anything but a couple of Linux distros :D
Anyway, no reason for any further discussion. Created #7118.
As for bugreports there are way too many terrible and obvious Qt bugs on macOS for me to bother reporting them.

zeule · 2017-07-19T13:14:58Z

That does not work on anything but a couple of Linux distros

It works on Windows too (Qt 5.9.2 and later).

sledgehammer999 added the Look and feel Affect UI "Look and feel" only without changing the logic label Jan 18, 2017

zeule mentioned this pull request Feb 8, 2017

Configurable UI appearance and corresponding options page #6363

Closed

thalieht reviewed Apr 8, 2017

View reviewed changes

zeule requested review from glassez and Chocobo1 April 20, 2017 16:36

zeule self-assigned this Apr 21, 2017

zeule mentioned this pull request Apr 26, 2017

Color and font themes #6698

Closed

12 tasks

Follow project coding style. Issue #2192.

8318f72

zeule dismissed stale reviews from glassez and Chocobo1 via b834ac1 July 3, 2017 09:54

zeule changed the title ~~Optionally use system file type specific icons in contents tab~~ Use system file type specific icons in contents tab Jul 3, 2017

sledgehammer999 reviewed Jul 3, 2017

View reviewed changes

zeule added 2 commits July 3, 2017 16:49

Use system file type specific icons in contents tab

30bf6e9

The icon is determined via QFileIconProvider using filename extension only.

Add QFileIconProvider implementation using mime database

57493c5

If built-in QFileIconProvider does not seem to work, use custom implementation which queries mime database.

sledgehammer999 merged commit 8e6df57 into qbittorrent:master Jul 3, 2017

zeule deleted the file-icons branch July 3, 2017 16:20

vit9696 mentioned this pull request Jul 16, 2017

Various macOS UI improvements #6952

Merged

13 tasks

vit9696 mentioned this pull request Jul 19, 2017

Fixed file type icons not displaying on macOS #7118

Merged

		#include "guiiconprovider.h"
		#include "base/settingsstorage.h"

Use system file type specific icons in contents tab #6156

Use system file type specific icons in contents tab #6156

Conversation

zeule commented Dec 30, 2016 • edited Loading

okeatime commented Dec 31, 2016

zeule commented Dec 31, 2016 • edited Loading

zeule commented Jan 3, 2017

okeatime commented Jan 4, 2017

sledgehammer999 commented Jan 18, 2017

sledgehammer999 commented Jan 18, 2017

sledgehammer999 commented Jan 18, 2017

zeule commented Jan 18, 2017 • edited Loading

sledgehammer999 commented Jan 18, 2017

zeule commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Apr 20, 2017 • edited Loading

zeule commented Apr 20, 2017 • edited Loading

zeule commented Apr 20, 2017

zeule commented Apr 20, 2017

zeule commented Apr 20, 2017 • edited Loading

zeule commented Apr 20, 2017

zeule commented Apr 20, 2017

zywo commented Apr 20, 2017

zeule commented Apr 20, 2017

Chocobo1 commented Apr 21, 2017

zeule commented Apr 21, 2017

zeule commented Apr 21, 2017

zywo commented Apr 21, 2017

sledgehammer999 commented Jul 2, 2017

zeule commented Jul 2, 2017

sledgehammer999 commented Jul 2, 2017

zeule commented Jul 3, 2017

sledgehammer999 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledgehammer999 commented Jul 3, 2017

zeule commented Jul 3, 2017

zeule commented Jul 10, 2017

sledgehammer999 commented Jul 10, 2017

vit9696 commented Jul 15, 2017

zeule commented Jul 16, 2017

vit9696 commented Jul 16, 2017 • edited Loading

vit9696 commented Jul 19, 2017

zeule commented Jul 19, 2017 • edited Loading

vit9696 commented Jul 19, 2017

zeule commented Jul 19, 2017

vit9696 commented Jul 19, 2017

zeule commented Jul 19, 2017

zeule commented Dec 30, 2016 •

edited

Loading

zeule commented Dec 31, 2016 •

edited

Loading

zeule commented Jan 18, 2017 •

edited

Loading

zeule commented Apr 20, 2017 •

edited

Loading

zeule commented Apr 20, 2017 •

edited

Loading

zeule commented Apr 20, 2017 •

edited

Loading

vit9696 commented Jul 16, 2017 •

edited

Loading

zeule commented Jul 19, 2017 •

edited

Loading