Fixed mac specific compiler errors and cleaned up Objective C code. Closes #7252 #7253

briankendall · 2017-08-11T06:18:51Z

Implements changes suggested in #7252

I removed the use of a private Qt function that was causing link errors, added a new file src/gui/macutilities.mm so that there's a place to write Objective-C functions that can be used from C++, and converted the two places where the Objective-C runtime was being used in C++ into real Objective C and moved it into this new file.

sledgehammer999 · 2017-08-11T08:27:27Z

@vit9696 you might want to see this.

@briankendall I see that the new code is "different" syntactically. If yours objective-c++ what was the previous? It seemed more C/C++.

glassez · 2017-08-11T08:52:39Z

src/gui/macutilities.h

@@ -0,0 +1,13 @@
+#ifndef MACUTILITIES_H


You need to put standard project header in your files.

Made this change. Please let me know if I got the header correct.

You should specify the email after your name. You don't want to do it because of some reasons?

I noticed Mike used his github name and went with that but I can change it to my email.

glassez · 2017-08-11T08:54:10Z

src/gui/macutilities.h

+#ifndef MACUTILITIES_H
+#define MACUTILITIES_H
+
+#ifdef Q_OS_MAC


You should use this ifdef in that place where you include this header, not here.

glassez · 2017-08-11T08:55:23Z

src/gui/macutilities.h

+void overrideDockClickHandler(bool (*dockClickHandler)(id, SEL, ...));
+#endif
+
+#endif // MACUTILITIES_H


It seems that something wrong is here... Do you have linebreak at the end of file?

glassez · 2017-08-11T08:56:40Z

src/gui/macutilities.mm

+#import <Cocoa/Cocoa.h>
+#include <QtMac>
+#include <objc/message.h>
+#include "macutilities.h"


Please fix includes order. See Coding Guidelines.

vit9696 · 2017-08-11T09:51:37Z

Hello,

I will reply here on both the issue (#7252) and the pull request.

Qt 5.7.1 and even earlier still have this function, here it is in 5.7 branch, for example. However, starting with 5.8 they moved it to QtGui, so it might indeed be possible that Qt linking options are not so smart to link you automatically.

Be that microoptimisation or not, this code is being invoked pretty frequently, and from what I remember from the logs, not just once per appearance, but way more often for some reason. On dual-gpu macs or macs with just discrete gpus I may well imagine slowdowns due to constant ram <--> vram uploading.

From my own side, I do not see a reason to use 5.7 or even 5.8 on 10.12.x. They are not even supportedby Qt itself, and if one wants 10.9.x support, there is 5.8, which has the function in the correct place. (By the way, @sledgehammer999, I am starting to wonder, whether it could be a good idea to provide the official binaries linked to 5.8, so that they work on 10.9, or even 5.7 to get 10.8 support back?)

As for toNSString, I am really not sure what's up. It was there since 5.2 according to Qt docs. I guess it could be some sort of header inclusion stuff, since you used it in this pull request.
As for objc stuff, I did not want to add extra files to the build system, since I only used cmake, and had no desire to mess with qmake. The code felt too small to bother using objc syntax. I am personally fine with this getting a new file, but it currently breaks cmake compilation for an obvious reason. Please fix.

In any case, I am not too against this code since it already is written and this way is easier to maintain. But one should at least visually check the performance on torrents with thousands of files. I guess Qt does not do things rightly in most of the places anyway, so it may well be "ok".

briankendall · 2017-08-11T16:44:39Z

@vit9696

Be that microoptimisation or not, this code is being invoked pretty frequently, and from what I remember from the logs, not just once per appearance, but way more often for some reason. On dual-gpu macs or macs with just discrete gpus I may well imagine slowdowns due to constant ram <--> vram uploading.

You're right, it is called a lot. I think the appropriate thing to do if we're being performance minded is to cache the icons.

Anyone have any objections to changing MacFileIconProvider to something like this:

    class MacFileIconProvider: public UnifiedFileIconProvider
    {
    public:
        using QFileIconProvider::icon;

        QIcon icon(const QFileInfo &info) const override
        {
            const QString ext = info.suffix();
            if (!ext.isEmpty()) {
                if (iconCache.contains(ext))
                    return iconCache[ext];
                
                MacFileIconProvider *mutableThis = const_cast<MacFileIconProvider *>(this);
                mutableThis->iconCache[ext] = QIcon(pixmapForExtension(ext, QSize(32, 32)));
                if (!mutableThis->iconCache[ext].isNull())
                    return mutableThis->iconCache[ext];
            }

            return UnifiedFileIconProvider::icon(info);
        }
        
    private:
        QMap<QString, QIcon> iconCache;
    };

It does involve some tomfoolery with modifying a member variable in a const method, but that can't really be helped since it's originally defined in QFileIconProvider. And this does seem to work.

As for toNSString, I am really not sure what's up. It was there since 5.2 according to Qt docs. I guess it could be some sort of header inclusion stuff, since you used it in this pull request.

QString::toNSString is only supposed to be available if you're compiling an Objective-C or Objective-C++ source file. Honestly I'm not sure why the file compiled in the first place! ~~Perhaps there's some difference between qmake vs. cmake compiling?~~ edit: No, it's not that -- I get the same error when building using cmake.

zeule · 2017-08-11T16:54:38Z

Anyone have any objections to changing MacFileIconProvider to something like this:

The Windows one seems to be suffering from the same problem.

It does involve some tomfoolery with modifying a member variable in a const method,

private:
    mutable QMap<QString, QIcon> iconCache;

vit9696 · 2017-08-11T17:12:04Z

@briankendall, actually given the small size of the icons caching is likely the right way. Other than mutable I would suggest you avoid double search in all the places:

contains and square brace operator for a successful lookup → use QMap::find
square brace for insertion → you are not overwriting but inserting → use QMap::insert

The above suggestions are minor code cleaning stuff, but there is an actual bug.
You are checking for null after icon insertion, which means you could cache a null value and then it will be found and returned by the provider. Null check should be performed on a temp variable before the insertion.

briankendall · 2017-08-11T17:19:52Z

@evsh

mutable QMap<QString, QIcon> iconCache;

I learned something new about C++11 today. 😝

@vit9696 Good points

I just updated the PR with the changes @glassez requested, fixes for building with cmake, and the caching feature as discussed.

zeule · 2017-08-11T17:26:15Z

src/gui/torrentcontentmodel.cpp

+                    return cacheEntry.value();
+
+                QIcon icon = QIcon(pixmapForExtension(ext, QSize(32, 32)));
+                if (!icon.isNull())


Or return *m_iconCache.insert(ext, icon);

Braces is correct. The coding style I'm used to has braces being used on every if statement even when they're one line... mainly so I don't have to worry about making silly mistakes like I just did.

vit9696 · 2017-08-11T17:30:28Z

QString::toNSString is only supposed to be available if you're compiling an Objective-C or Objective-C++ source file. Honestly I'm not sure why the file compiled in the first place! Perhaps there's some difference between qmake vs. cmake compiling? edit: No, it's not that -- I get the same error when building using cmake.

This looks like a change introduced in 5.8, surprisingly nothing in the docs says about it. In 5.8 and above NSString is defined as class in c++ code and as as an objective C class in objc/objc++ code. Thus QString header has no ifdefs against defining toNSString, and buildbots eat this code without issues.

#if defined(Q_OS_DARWIN) || defined(Q_QDOC)
    static QString fromCFString(CFStringRef string);
    CFStringRef toCFString() const Q_DECL_CF_RETURNS_RETAINED;
    static QString fromNSString(const NSString *string);
    NSString *toNSString() const Q_DECL_NS_RETURNS_AUTORELEASED;
#endif

Yet in 5.7 there indeed was a check for objc, which is rather unfortunate.

As for pull-request changes I marked one in code, the rest looks more or less good, except maybe some style issues qBittorrent devs find more often than anything :D.

Chocobo1 · 2017-08-11T17:43:19Z

src/gui/mainwindow.cpp

@@ -106,6 +104,9 @@
 #include "executionlog.h"
 #include "hidabletabwidget.h"
 #include "ui_mainwindow.h"
+#if defined (Q_OS_MAC)


pls add an empty line above this.

Chocobo1 · 2017-08-11T17:50:09Z

src/gui/torrentcontentmodel.cpp

+                auto cacheEntry = m_iconCache.find(ext);
+
+                if (cacheEntry != m_iconCache.end())
+                    return cacheEntry.value();


personal opinion:

I would name it cacheIter or something like that. cacheEntry sounds like its type is QIcon.

return *cacheIter;, I'm so used to using operator* to dereference an iterator.

glassez · 2017-08-11T17:58:05Z

src/gui/torrentcontentmodel.cpp

            }

            return UnifiedFileIconProvider::icon(info);
        }
-
+        


You've inserted some redundant spaces in this line. Remove it, please.

briankendall · 2017-08-11T17:59:42Z

@glassez @Chocobo1 Made the requested changes.

Chocobo1 · 2017-08-11T18:00:53Z

src/gui/torrentcontentmodel.cpp

+                QIcon icon = QIcon(pixmapForExtension(ext, QSize(32, 32)));
+                if (!icon.isNull()) {
+                    m_iconCache.insert(ext, icon);
+                    return icon;


OK, one last thing, move return outside/below the if(){} so that the function will always return something.

I think we want to avoid returning a null icon. If it's null, it'll exit the if block and hit return UnifiedFileIconProvider::icon(info);. So that way it falls back on the behavior of UnifiedFileIconProvider.

damn, I missed the return below, so nevermind this.

vit9696 · 2017-08-11T17:26:12Z

src/gui/CMakeLists.txt

@@ -118,6 +118,10 @@ transferlistwidget.cpp
 updownratiodlg.cpp
 )

+if (APPLE)
+    list(APPEND QBT_GUI_SOURCES macutilities.mm)
+endif (APPLE)


I think it is better to add macutilities.h to QBT_GUI_HEADERS as well, since the code below does it at least.

Ugh, don't tell me github did not send it right away :/

Right, I forgot to add that.

Chocobo1

LGTM

glassez · 2017-08-11T18:27:26Z

src/gui/gui.pri

@@ -121,6 +121,10 @@ win32|macx {
    SOURCES += $$PWD/programupdater.cpp
 }

+macx {
+    OBJECTIVE_SOURCES += $$PWD/macutilities.mm


Shouldn't "macutilities.h" be in project too?

Yup, I forgot to add it.

sledgehammer999 · 2017-08-11T19:02:25Z

About icon-caching: Minor side-effect is that we won't pickup any changes made in the system after first time querying each file extension. eg new icon registered.
There is QPixmapCache that might be useful.

About the NSImage to QPixmap conversion: Should we just copy the implementation of qt_mac_toQPixmap() instead and leave a comment about it? The licenses are compatible...

I am starting to wonder, whether it could be a good idea to provide the official binaries linked to 5.8, so that they work on 10.9

Hmm, maybe that is a good idea. Both 10.9 and 10.12 are supported this way.
I just hope that the homebrew formula for 5.8 has the patch for macdeployqt recorded in it.

sledgehammer999 · 2017-08-11T19:03:49Z

src/gui/torrentcontentmodel.cpp

-                const QPixmap pixmap = pixmapForExtension(ext, QSize(32, 32));
-                if (!pixmap.isNull())
-                    return QIcon(pixmap);
+                auto cacheIter = m_iconCache.find(ext);


It should come in a separate commit.
Or since it is out-of-scope for this PR, you can leave it unimplemented.

I'm not in a position to build or test in Windows right now so I'd prefer to leave it for a separate PR.

briankendall · 2017-08-11T19:31:06Z

@sledgehammer999 I'm not too worried about using out-of-date icons, because a) they rarely change for a particular file type, and b) the cache only lasts as long as the torrent that's being displayed in the GUI since it's a member of MacFileIconProvider. If that's a concern you want addressed, though, I could modify the PR to attach a timestamp to each entry in the cache so that after enough time is elapsed the icon is reacquired.

Re: QPixmapCache: do you want me to change the PR to use it? I wasn't aware of that class when I wrote this. It may not be necessary since we're not dealing with large images or a large number of images but it would help not duplicate icons between instances of MacFileIconProvider. I wouldn't be able to implement the above timestamping if I used it, though.

Re: qt_mac_toQPixmap : I looked into copying it into qBittorrent and found it to be a bit onerous. It depends on a lot of other private helper functions and classes so disentangling that would be tricky. I think it's better to use public Qt functions like this PR has now, especially if we're just caching the results. Using a more direct method of converting to a QPixmap would only save us a negligible amount of processing time.

sledgehammer999 · 2017-08-11T20:00:11Z

If that's a concern you want addressed, though, I could modify the PR to attach a timestamp to each entry in the cache so that after enough time is elapsed the icon is reacquired.

As I said it is a minor issue. I don't think it warrants the coplexity of timestamping.

QPixmapCache: do you want me to change the PR to use it?

As you said performance wise it probably won't be measurable, but it is sweet that the cache is application wide and different torrents can share the icons for the same file extension.
Personally I would change it. Wait for others to reply on this, if you don't feel like changing it now.

qt_mac_toQPixmap: I looked into copying it into qBittorrent and found it to be a bit onerous.

You're right, the cache pretty much makes this moot.

sledgehammer999 · 2017-08-11T20:07:44Z

src/gui/macutilities.mm

+    @autoreleasepool {
+        NSImage *image = [[NSWorkspace sharedWorkspace] iconForFileType:ext.toNSString()];
+        if (image) {
+            CGImageRef cgImage = [image CGImageForProposedRect:nil context:nil hints:nil];


Would it be better to do this instead?

NSRect rect = NSMakeRect(0, 0, size.width(), size.height()); CGImageRef cgImage = [image CGImageForProposedRect:&rect context:nil hints:nil];

And not scale it below?

That does make sense. I had misunderstood what the purpose of the first parameter of CGImageForProposedRect was for.

sledgehammer999 · 2017-08-11T20:09:46Z

src/gui/mainwindow.cpp

@@ -107,6 +105,10 @@
 #include "hidabletabwidget.h"
 #include "ui_mainwindow.h"

+#if defined (Q_OS_MAC)


let's keep it the same in the same file

#ifdef Q_OS_MAC

sledgehammer999 · 2017-08-11T20:10:44Z

src/gui/torrentcontentmodel.cpp

-// See src/gui/painting/qcoregraphics_p.h for more details
-// QtMac::fromCGImageRef takes a CGImageRef and thus requires a double conversion
-QPixmap qt_mac_toQPixmap(const NSImage *image, const QSizeF &size);
+#if defined(Q_OS_MAC) 


I'm confused. In torrentcontentmodel.cpp it's consistently written as #if defined(...) in each instance, including ones that were already there. Do you want me to change it to #ifdef?

I would prefer #ifdef Q_OS_MAC as it was like that before your edit.
The rest of the file is a bit inconsistent.

vit9696 · 2017-08-11T20:19:05Z

In my opinion this PR should not use QPixmapCache, although it might be an idea for the future development. The main reason against is what one should cache, what limits should there be, how much of QPixmapCachw should file icons use. This is more an application-wide decision than a local platform-specific change.

There already are tons of issues with icons, especially on mac. And I personally hope #6698 gets merged soon, even if without many changes, so that some sort of icon categorisation (or better to say tagging) could later appear with the fixes to ugly icon highlightion that should not happen on mac. And based on that some icon categories could be cacheable. Therefore it is better not to make way too much mess and redo things many times.

sledgehammer999 · 2017-08-11T20:26:17Z

OK, unless someone of the devs agrees with me leave the cache as you have implemented it.
PS: I have left minor inline comments.

Created new file src/gui/macutilities.mm, moved code from mainwindow.cpp and torrentcontentmodel.cpp that used the Objective C runtime into it and converted it to actual Objective C. Rewrote pixmapForExtension() so that it doesn't call into private Qt functions.

sledgehammer999 · 2017-08-13T19:03:15Z

src/gui/mainwindow.cpp

@@ -107,7 +105,11 @@
 #include "hidabletabwidget.h"
 #include "ui_mainwindow.h"

-#ifdef Q_OS_MAC
+#if defined (Q_OS_MAC)


Change it to #ifdef Q_OS_MAC as it was previously.

sledgehammer999 · 2017-08-13T19:03:22Z

src/gui/mainwindow.cpp

+#include "macutilities.h"
+#endif
+
+#if defined (Q_OS_MAC)


briankendall · 2017-08-14T02:09:37Z

Unfortunately I’m out of town until the 23rd so I won’t be able to make these changes until then.

vit9696 · 2017-08-14T14:31:06Z

@sledgehammer999 maybe merge this as is, and then push my commit 56366b0 on top of it? Other than those ifdefs, which I fixed myself, there is nothing wrong here, and without macutilities.mm merged it is pretty problematic to work on anything else.

The reason is that you might want to release 3.4.0 reasonably soon, and currently notifications are entirely broken in macOS (have never known that they worked before, so could not have checked). I suppose it is a pretty critical regression, and one must fix it before 3.4.0 lands.

sledgehammer999 · 2017-08-14T16:26:42Z

@vit9696 I was planning to do something similar. But where the hell is that commit you are referencing? On which branch? It points to a commit from this repo but I can't find it locally or which PR introduced it.

vit9696 · 2017-08-14T16:28:44Z

@sledgehammer999 I am afraid something is up to shithub to mess it up :/
Here is the branch: https://github.com/vit9696/qBittorrent/tree/notifications

glassez · 2017-08-14T19:03:23Z

@sledgehammer999, you can fetch this branch, squash its commits and add some fixups yourself and then force push it here (to this PR author repo).

sledgehammer999 · 2017-08-15T18:06:02Z

@sledgehammer999, you can fetch this branch, squash its commits and add some fixups yourself and then force push it here (to this PR author repo).

It worked!!!
I think your blocking is fixed now, so I am merging this.

@briankendall thx.

sledgehammer999 · 2017-08-15T18:10:31Z

@vit9696 can you open a PR for your notifications fix?

vit9696 · 2017-08-15T18:19:13Z

Yes, done #7281.

briankendall changed the title ~~Fixed mac specific compiler errors and cleaned up Objective C code~~ Fixed mac specific compiler errors and cleaned up Objective C code. Closes #7252 Aug 11, 2017

sledgehammer999 added the OS: macOS Issues specific to macOS label Aug 11, 2017

sledgehammer999 added this to the 3.4.0 milestone Aug 11, 2017

glassez requested changes Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from 677b718 to 040909d Compare August 11, 2017 17:17

zeule reviewed Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from 040909d to 810fe91 Compare August 11, 2017 17:27

Chocobo1 reviewed Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from 810fe91 to f5152d7 Compare August 11, 2017 17:58

glassez requested changes Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from f5152d7 to da285a7 Compare August 11, 2017 17:59

Chocobo1 reviewed Aug 11, 2017

View reviewed changes

vit9696 reviewed Aug 11, 2017

View reviewed changes

Chocobo1 previously approved these changes Aug 11, 2017

View reviewed changes

briankendall dismissed Chocobo1’s stale review via 2bd9051 August 11, 2017 18:06

briankendall force-pushed the fix_objc_errors branch 2 times, most recently from 2bd9051 to b13a355 Compare August 11, 2017 18:09

glassez reviewed Aug 11, 2017

View reviewed changes

sledgehammer999 reviewed Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from b13a355 to cf95a95 Compare August 11, 2017 19:34

sledgehammer999 reviewed Aug 11, 2017

View reviewed changes

briankendall force-pushed the fix_objc_errors branch from cf95a95 to 62b9569 Compare August 12, 2017 00:51

zeule mentioned this pull request Aug 12, 2017

[3.4.0] A lot of files slow down the content tab #7264

Closed

sledgehammer999 mentioned this pull request Aug 13, 2017

Fix file icon loading on Windows. #7265

Closed

sledgehammer999 reviewed Aug 13, 2017

View reviewed changes

src/gui/mainwindow.cpp Outdated

#include "macutilities.h"

#endif

#if defined (Q_OS_MAC)

Copy link

Member

sledgehammer999 Aug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

vit9696 mentioned this pull request Aug 14, 2017

Fix macOS window restoration after using hide icon #7272

Merged

Use simpler ifdef style.

c87c1d2

sledgehammer999 merged commit e5ea3bd into qbittorrent:master Aug 15, 2017

Fixed mac specific compiler errors and cleaned up Objective C code. Closes #7252 #7253

Fixed mac specific compiler errors and cleaned up Objective C code. Closes #7252 #7253

Conversation

briankendall commented Aug 11, 2017

sledgehammer999 commented Aug 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vit9696 commented Aug 11, 2017

briankendall commented Aug 11, 2017 • edited

zeule commented Aug 11, 2017

vit9696 commented Aug 11, 2017

briankendall commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vit9696 commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankendall commented Aug 11, 2017

Chocobo1 Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vit9696 Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sledgehammer999 commented Aug 11, 2017

Choose a reason for hiding this comment

briankendall Aug 11, 2017 • edited

Choose a reason for hiding this comment

briankendall commented Aug 11, 2017

sledgehammer999 commented Aug 11, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vit9696 commented Aug 11, 2017

sledgehammer999 commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankendall commented Aug 14, 2017

vit9696 commented Aug 14, 2017

sledgehammer999 commented Aug 14, 2017

vit9696 commented Aug 14, 2017

glassez commented Aug 14, 2017

sledgehammer999 commented Aug 15, 2017

sledgehammer999 commented Aug 15, 2017

vit9696 commented Aug 15, 2017

briankendall commented Aug 11, 2017 •

edited

Chocobo1 Aug 11, 2017 •

edited

vit9696 Aug 11, 2017 •

edited

briankendall Aug 11, 2017 •

edited