Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Aug 15, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion macxconf.pri
Expand Up @@ -9,7 +9,7 @@ exists($$OUT_PWD/../conf.pri) {
include(conf.pri)
}

LIBS += -framework Carbon -framework IOKit
LIBS += -framework Carbon -framework IOKit -framework AppKit

QT_LANG_PATH = ../dist/qt-translations
DIST_PATH = ../dist/mac
Expand Down
3 changes: 2 additions & 1 deletion src/base/CMakeLists.txt
Expand Up @@ -141,5 +141,6 @@ endif ()
if (APPLE)
find_library(IOKit_LIBRARY IOKit)
find_library(Carbon_LIBRARY Carbon)
target_link_libraries(qbt_base PRIVATE ${Carbon_LIBRARY} ${IOKit_LIBRARY})
find_library(AppKit_LIBRARY AppKit)
target_link_libraries(qbt_base PRIVATE ${Carbon_LIBRARY} ${IOKit_LIBRARY} ${AppKit_LIBRARY})
endif (APPLE)
5 changes: 5 additions & 0 deletions src/gui/CMakeLists.txt
Expand Up @@ -118,6 +118,11 @@ transferlistwidget.cpp
updownratiodlg.cpp
)

if (APPLE)
list(APPEND QBT_GUI_HEADERS macutilities.h)
list(APPEND QBT_GUI_SOURCES macutilities.mm)
endif (APPLE)
Copy link
Contributor

@vit9696 vit9696 Aug 11, 2017

Choose a reason for hiding this comment

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

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 :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I forgot to add that.


if (WIN32 OR APPLE)
list(APPEND QBT_GUI_HEADERS programupdater.h)
list(APPEND QBT_GUI_SOURCES programupdater.cpp)
Expand Down
5 changes: 5 additions & 0 deletions src/gui/gui.pri
Expand Up @@ -121,6 +121,11 @@ win32|macx {
SOURCES += $$PWD/programupdater.cpp
}

macx {
HEADERS += $$PWD/macutilities.h
OBJECTIVE_SOURCES += $$PWD/macutilities.mm
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I forgot to add it.

}

FORMS += \
$$PWD/mainwindow.ui \
$$PWD/about.ui \
Expand Down
39 changes: 39 additions & 0 deletions src/gui/macutilities.h
@@ -0,0 +1,39 @@
/*
* Bittorrent Client using Qt and libtorrent.
* Copyright (C) 2017 Brian Kendall <brian@briankendall.net>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* In addition, as a special exception, the copyright holders give permission to
* link this program with the OpenSSL project's "OpenSSL" library (or with
* modified versions of it that use the same license as the "OpenSSL" library),
* and distribute the linked executables. You must obey the GNU General Public
* License in all respects for all of the code used other than "OpenSSL". If you
* modify file(s), you may extend this exception to your version of the file(s),
* but you are not obligated to do so. If you do not wish to do so, delete this
* exception statement from your version.
*/

#ifndef MACUTILITIES_H
Copy link
Member

Choose a reason for hiding this comment

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

You need to put standard project header in your files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

#define MACUTILITIES_H

#include <QPixmap>
#include <QSize>
#include <objc/objc.h>

QPixmap pixmapForExtension(const QString &ext, const QSize &size);
void overrideDockClickHandler(bool (*dockClickHandler)(id, SEL, ...));

#endif // MACUTILITIES_H
71 changes: 71 additions & 0 deletions src/gui/macutilities.mm
@@ -0,0 +1,71 @@
/*
* Bittorrent Client using Qt and libtorrent.
* Copyright (C) 2017 Brian Kendall <brian@briankendall.net>
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
* as published by the Free Software Foundation; either version 2
* of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* In addition, as a special exception, the copyright holders give permission to
* link this program with the OpenSSL project's "OpenSSL" library (or with
* modified versions of it that use the same license as the "OpenSSL" library),
* and distribute the linked executables. You must obey the GNU General Public
* License in all respects for all of the code used other than "OpenSSL". If you
* modify file(s), you may extend this exception to your version of the file(s),
* but you are not obligated to do so. If you do not wish to do so, delete this
* exception statement from your version.
*/

#include "macutilities.h"

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

QPixmap pixmapForExtension(const QString &ext, const QSize &size)
{
@autoreleasepool {
NSImage *image = [[NSWorkspace sharedWorkspace] iconForFileType:ext.toNSString()];
if (image) {
NSRect rect = NSMakeRect(0, 0, size.width(), size.height());
CGImageRef cgImage = [image CGImageForProposedRect:&rect context:nil hints:nil];
return QtMac::fromCGImageRef(cgImage);
}

return QPixmap();
}
}

void overrideDockClickHandler(bool (*dockClickHandler)(id, SEL, ...))
{
NSApplication *appInst = [NSApplication sharedApplication];

if (!appInst)
return;

Class delClass = [[appInst delegate] class];
SEL shouldHandle = sel_registerName("applicationShouldHandleReopen:hasVisibleWindows:");

if (class_getInstanceMethod(delClass, shouldHandle)) {
if (class_replaceMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:"))
qDebug("Registered dock click handler (replaced original method)");
else
qWarning("Failed to replace method for dock click handler");
}
else {
if (class_addMethod(delClass, shouldHandle, (IMP)dockClickHandler, "B@:"))
qDebug("Registered dock click handler");
else
qWarning("Failed to register dock click handler");
}
}
32 changes: 7 additions & 25 deletions src/gui/mainwindow.cpp
Expand Up @@ -31,8 +31,6 @@
#include "mainwindow.h"

#ifdef Q_OS_MAC
#include <objc/objc.h>
#include <objc/message.h>
#include <QtMacExtras>
#include <QtMac>
#endif
Expand Down Expand Up @@ -107,7 +105,11 @@
#include "hidabletabwidget.h"
#include "ui_mainwindow.h"

#ifdef Q_OS_MAC
#if defined (Q_OS_MAC)
Copy link
Member

Choose a reason for hiding this comment

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

pls add an empty line above this.

Copy link
Member

Choose a reason for hiding this comment

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

let's keep it the same in the same file

#ifdef Q_OS_MAC

Copy link
Member

Choose a reason for hiding this comment

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

Change it to #ifdef Q_OS_MAC as it was previously.

#include "macutilities.h"
#endif

#if defined (Q_OS_MAC)
Copy link
Member

Choose a reason for hiding this comment

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

same here

void qt_mac_set_dock_menu(QMenu *menu);
#endif

Expand Down Expand Up @@ -1294,29 +1296,9 @@ static bool dockClickHandler(id self, SEL cmd, ...)
}

void MainWindow::setupDockClickHandler()
{
Class cls = objc_getClass("NSApplication");
objc_object *appInst = objc_msgSend(reinterpret_cast<objc_object *>(cls), sel_registerName("sharedApplication"));

if (!appInst)
return;

{
dockMainWindowHandle = this;
objc_object* delegate = objc_msgSend(appInst, sel_registerName("delegate"));
Class delClass = reinterpret_cast<Class>(objc_msgSend(delegate, sel_registerName("class")));
SEL shouldHandle = sel_registerName("applicationShouldHandleReopen:hasVisibleWindows:");
if (class_getInstanceMethod(delClass, shouldHandle)) {
if (class_replaceMethod(delClass, shouldHandle, reinterpret_cast<IMP>(dockClickHandler), "B@:"))
qDebug("Registered dock click handler (replaced original method)");
else
qWarning("Failed to replace method for dock click handler");
}
else {
if (class_addMethod(delClass, shouldHandle, reinterpret_cast<IMP>(dockClickHandler), "B@:"))
qDebug("Registered dock click handler");
else
qWarning("Failed to register dock click handler");
}
overrideDockClickHandler(dockClickHandler);
}

#endif
Expand Down
43 changes: 14 additions & 29 deletions src/gui/torrentcontentmodel.cpp
Expand Up @@ -32,14 +32,12 @@
#include <QFileIconProvider>
#include <QFileInfo>
#include <QIcon>
#include <QMap>

#if defined(Q_OS_WIN)
#include <Windows.h>
#include <Shellapi.h>
#include <QtWin>
#elif defined(Q_OS_MAC)
#include <objc/objc.h>
#include <objc/message.h>
#else
#include <QMimeDatabase>
#include <QMimeType>
Expand All @@ -52,13 +50,8 @@
#include "torrentcontentmodelitem.h"
#include "torrentcontentmodelfolder.h"
#include "torrentcontentmodelfile.h"

#ifdef Q_OS_MAC
struct NSImage;
// This function is a private QtGui library export on macOS
// 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)
Copy link
Member

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

#include "macutilities.h"
#endif

namespace
Expand Down Expand Up @@ -113,31 +106,23 @@ namespace
{
const QString ext = info.suffix();
if (!ext.isEmpty()) {
const QPixmap pixmap = pixmapForExtension(ext, QSize(32, 32));
if (!pixmap.isNull())
return QIcon(pixmap);
auto cacheIter = m_iconCache.find(ext);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (cacheIter != m_iconCache.end())
return *cacheIter;

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

@Chocobo1 Chocobo1 Aug 11, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

return UnifiedFileIconProvider::icon(info);
}

private:
QPixmap pixmapForExtension(const QString &ext, const QSize &size) const
{
QMacAutoReleasePool pool;
objc_object *woskspaceCls = reinterpret_cast<objc_object *>(objc_getClass("NSWorkspace"));
SEL sharedWorkspaceSel = sel_registerName("sharedWorkspace");
SEL iconForFileTypeSel = sel_registerName("iconForFileType:");

objc_object *sharedWorkspace = objc_msgSend(woskspaceCls, sharedWorkspaceSel);
if (sharedWorkspace) {
objc_object *image = objc_msgSend(sharedWorkspace, iconForFileTypeSel, ext.toNSString());
if (image)
return qt_mac_toQPixmap(reinterpret_cast<NSImage *>(image), size);
}

return QPixmap();
}
mutable QMap<QString, QIcon> m_iconCache;
};
#else
/**
Expand Down