Various macOS UI improvements #6952

vit9696 · 2017-06-12T04:06:07Z

Hello,

This pull-request consolidates and fixes most of the existing changes (e.g. #4757 and #2096) and brings us other improvements. Whereas this is not a complete fix to all design issues in qBittorrent in regards to macOS, this is certainly a good start.

Please let me know what you consider important to change (if anything), and let's get this merged :)

The changelist includes:

Screenshots

Regarding separator → spacer replacement: I am thinking of removing them entirely, since they do not really add much clarity, but to my eyes it looks fine either way. If you think it is better to remove them, let me know.

Regarding Dock restoration: it is a very annoying bug (or shall I say feature) in Qt, no event is delivered to the dispatcher when you click on the icon regardless of the docs. The only reason is to use native APIs, and although it is a little ugly, it works well.

Regarding the badges: the API Qt offers is very limited, and one cannot display more than a single line, and its length is very short. To avoid dotted shortening even the "D: " prefix was removed. Unfortunately it also does not allow colouring, and to avoid different sorts of confusion I decided not to add the uploading speed badge.

Even though I am open for a discussion on what is worth changing and may even make another pull-request fixing certain other areas, I will not add any new features to the current one.

Among the macOS-specific issues I am aware of are all the menu/app icons. They are supposed to be greyscale, not coloured. And I am actually wondering how would you prefer to change it?

vit9696 · 2017-06-12T04:55:40Z

I fixed the build issues except the one with qmake+gui. Your homebrew formula defines -skip qtmacextras, which is necessary to provide Dock icon badges and thus mandatory. Once you update your toolchain (just removing this line should be enough), your buildsystem will start working once again.

I could make it build even without the badges, but I see no reason to limit macOS functionality just because a certain toolchain is broken.

zeule · 2017-06-12T06:28:59Z

src/CMakeLists.txt

 endif (GUI)
 if (DBUS)
    list (APPEND QBT_QT_COMPONENTS DBus)
 endif (DBUS)
 find_package(Qt5 5.5.1 COMPONENTS ${QBT_QT_COMPONENTS} REQUIRED)

+if (GUI AND APPLE)
+    include_directories(${Qt5MacExtras_INCLUDE_DIRS})


This one and the next 2 lines shall not be needed, as target_link_libraries(... Qt5::MacExtras), initiated by list (APPEND QBT_QT_COMPONENTS MacExtras) call above should do the very same.

I thought this myself, but actually it does not work. If I remove these lines, compilation will stop working.
I could not explain the reason and simply hardcoded the variables. I think it is a bug in Qt, because I saw other people doing similar things on the net.

You are right, I forgot that when we migrated from Qt4, I replaced smart commands with direct component names. You have to mention Qt5::MacExtras in target_link_libraries() directly. For instance:

if (APPLE) target_link_libraries(qbt_gui Qt5::MacExtras) endif()

in src/gui/CMakeLists.txt

Indeed, thanks.

Chocobo1 · 2017-06-12T07:05:41Z

src/gui/mainwindow.cpp

@@ -1929,7 +1929,7 @@ QIcon MainWindow::getSystrayIcon() const
    }

    QIcon icon;
-#if (defined(Q_OS_UNIX)


why this fails the compilation?

Chocobo1 · 2017-06-12T07:28:15Z

src/app/main.cpp

@@ -252,6 +252,11 @@ int main(int argc, char *argv[])
        signal(SIGSEGV, sigAbnormalHandler);
 #endif

+#ifdef Q_OS_MAC
+        // On OS X the standard is to not show icons in the menus
+        app->setAttribute(Qt::AA_DontShowIconsInMenus);


there are already an #if defined(Q_OS_MAC) block at line 210
you should move it there.

Chocobo1 · 2017-06-12T07:32:23Z

src/base/preferences.h

@@ -115,16 +115,18 @@ class Preferences: public QObject
    void setHideZeroValues(bool b);
    int getHideZeroComboValues() const;
    void setHideZeroComboValues(int n);
+#ifndef Q_OS_MAC


I would take out all entries in this #ifndef block and move it down to where you put

#ifndef Q_OS_MAC TrayIcon::Style trayIconStyle() const;

Chocobo1 · 2017-06-12T07:33:59Z

src/gui/mainwindow.cpp

@@ -52,6 +52,11 @@
 #include <QCryptographicHash>
 #include <QProcess>

+#ifdef Q_OS_MAC


move this block under

#include "mainwindow.h"

and move

#include <QtMac> #include <QtMacExtras>

to here

Chocobo1 · 2017-06-12T07:35:38Z

src/gui/mainwindow.cpp

@@ -204,6 +211,9 @@ MainWindow::MainWindow(QWidget *parent)
    qDebug("create tabWidget");
    m_tabs = new HidableTabWidget(this);
    connect(m_tabs, SIGNAL(currentChanged(int)), this, SLOT(tabChanged(int)));
+#ifdef Q_OS_MAC


move this block 1 line up.

Chocobo1 · 2017-06-12T07:38:24Z

src/gui/mainwindow.cpp

+
+#ifdef Q_OS_MAC
+    foreach (QAction *action, m_ui->toolBar->actions()) {
+        if (action->isSeparator()) {


what its purpose?

This removes vertical separators from the toolbar menu, they are invalid in macOS design. As I said above, there is no such concept. One could have space separation or no separation.

I replaced the separators with some extra space, but I think they are fine even if just removed.

OK, after searching for some osx toolbar screenshot on google, I get what you mean.

Chocobo1 · 2017-06-12T07:46:06Z

src/gui/mainwindow.cpp

@@ -343,6 +367,7 @@ MainWindow::MainWindow(QWidget *parent)
    // Load Window state and sizes
    readSettings();

+#ifndef Q_OS_MAC


is this equivalent?

if (!m_systrayIcon) { // Make sure the Window is visible if we don't have a tray icon if (pref->startMinimized()) { showMinimized(); } else { show(); activateWindow(); raise(); } } #ifndef Q_OS_MAC else { if (!(pref->startMinimized() || m_uiLocked)) { show(); activateWindow(); raise(); } else if (pref->startMinimized()) { showMinimized(); if (pref->minimizeToTray()) hide(); } } #endif

m_systrayIcon is undefined on mac, so one cannot write it this way.

Chocobo1 · 2017-06-12T07:48:04Z

src/gui/mainwindow.cpp

@@ -1022,12 +1055,20 @@ void MainWindow::showEvent(QShowEvent *e)
 void MainWindow::closeEvent(QCloseEvent *e)
 {
    Preferences *const pref = Preferences::instance();
+#ifndef Q_OS_MAC


change it to #ifdef Q_OS_MAC and switch the code

Chocobo1 · 2017-06-12T07:50:49Z

src/gui/mainwindow.cpp

        m_systrayIcon->setToolTip(html); // tray icon
    }
+#else
+    if (status.payloadDownloadRate > 0)
+        QtMac::setBadgeLabelText(tr("%1/s").arg(Utils::Misc::friendlyUnit(status.payloadDownloadRate)));


change tr to QString

Are you sure this is a correct suggestion? E.g. in Russian I would want to see %1/с not s. Since seconds in Russian are секунды.

Are you sure this is a correct suggestion?

After reading your explanation, obviously not.
IMO, it would be better to add this:

QtMac::setBadgeLabelText(tr("%1/s", "s is shorthand for seconds") .arg(Utils::Misc::friendlyUnit(status.payloadDownloadRate)));

and split the line please.

Alright, done

Chocobo1 · 2017-06-12T07:55:03Z

src/gui/mainwindow.h

    void balloonClicked();
    void writeSettings();
    void readSettings();
+#ifndef Q_OS_MAC


move this block down to under void toolbarFollowSystem();

Chocobo1 · 2017-06-12T07:57:37Z

src/gui/optionsdlg.cpp

@@ -143,13 +143,17 @@ OptionsDialog::OptionsDialog(QWidget *parent)

    // Load options
    loadOptions();
+#ifndef Q_OS_MAC


write #ifdef Q_OS_MAC and switch code.

Chocobo1 · 2017-06-12T08:10:49Z

2 things I wanted to add:

create fixup commits for now, note that eventually you'll have to squash all commits into one.
please provide before & after screenshots if possible, majority (at least me) of the devs don't have easy access to a mac.

Chocobo1 · 2017-06-12T08:14:56Z

src/gui/mainwindow.cpp

+    Class cls = objc_getClass("NSApplication");
+    objc_object *appInst = objc_msgSend(reinterpret_cast<objc_object *>(cls), sel_registerName("sharedApplication"));
+
+    if (appInst != nullptr) {


if (!appInst) return; dockMainWindowHandle = this; // and so on...

Chocobo1 · 2017-06-12T08:16:02Z

src/gui/mainwindow.cpp

+
+static MainWindow *dockMainWindowHandle;
+
+static bool dockClickHandler(id self, SEL _cmd, ...) {


move { to a newline.

pls avoid using underscore in variable names.

pls avoid using underscore in variable names.

@vit9696
I think you missed this one.

Yep, actually did. Fixed it now.

Chocobo1 · 2017-06-12T08:16:18Z

src/gui/mainwindow.cpp

+    return false;
+}
+
+void MainWindow::setupDockClickHandler() {


move { to a newline.

thalieht · 2017-06-12T10:06:16Z

src/gui/categoryfilterwidget.cpp

@@ -73,6 +73,7 @@ CategoryFilterWidget::CategoryFilterWidget(QWidget *parent)
    setIconSize(Utils::Misc::smallIconSize());
 #if defined(Q_OS_MAC)
    setAttribute(Qt::WA_MacShowFocusRect, false);
+    setIndentation(0);


I don't know if it's any different in mac but in KDE if you do this and enable Use Subcategories in options, any subcategories vanish from the sidepanel and there doesn't seem to be a way to show them because the expand icon isn't visible.

I realize this is probably out of the scope of this PR, IMO this should be used on all platforms but only when Use Subcategories is disabled and maybe reduce the indentation of branches (and the root as much as possible).

@thalieht actually I faield to find this option yesterday, and thus assumed that subcategories simply do not work on macOS. Now I found it, thanks :)

I changed the indentation to not be disabled when they are off, but doing something like this for all the platforms is the job of a separate pull request indeed.

vit9696 · 2017-06-12T16:38:33Z

@Chocobo1 I fixed all the issues you asked me to change except the ones I consider invalid.
The screenshots are available in the first post if you did not notice ^^

thalieht · 2017-06-12T17:08:26Z

src/gui/categoryfilterwidget.cpp

@@ -73,6 +73,8 @@ CategoryFilterWidget::CategoryFilterWidget(QWidget *parent)
    setIconSize(Utils::Misc::smallIconSize());
 #if defined(Q_OS_MAC)
    setAttribute(Qt::WA_MacShowFocusRect, false);
+    if (!BitTorrent::Session::instance()->isSubcategoriesEnabled())


But if Use Subcategories is changed it would need a restart for the indentation to change. Is that ok?
If you can make it change without restart i would like to suggest to move this out of the #if so all platforms can have it.

Edit: On second thought moving it outside the #if wouldn't fit the PR name :)

Of course I could fix it, and I have just done it. Thanks for reminding. But you are right, if I change it outside the preprocessor macro, it wouldn't fit this PR.

I have no development tools installed for the other platforms, but I think it should work fine for them. You or anybody else could submit a new PR removing the ifdefs once this gets merged :)

Chocobo1 · 2017-06-12T17:53:39Z

src/gui/categoryfilterwidget.h

@@ -57,4 +57,8 @@ private slots:
    QSize minimumSizeHint() const override;
    void rowsInserted(const QModelIndex &parent, int start, int end) override;
    QString askCategoryName();
+
+#ifdef Q_OS_MAC
+    int defaultIndentation;


private variables should prepended with m_,
thus m_defaultIndentation

vit9696 · 2017-06-12T19:50:55Z

Alright, I squashed all the changes into a single commit and even fixed a couple of other issues I spotted. Would you please merge this?

Chocobo1 · 2017-06-13T01:32:29Z

src/app/main.cpp

+            QByteArray path = "/usr/local/bin:";
+            path += qgetenv("PATH");
+            qputenv("PATH", path.constData());
+        }


please do us a favor if you don't mind: remove the curly braces here, it isn't really needed.

Chocobo1 · 2017-06-13T01:36:18Z

Would you please merge this?

I would like to if I can verify this PR on a mac, otherwise, this PR will be handled by the project owner @sledgehammer999.

vit9696 · 2017-06-13T01:40:08Z

@Chocobo1 well, I build this on a vm (too much of a dislike for Qt to let it arrive on my real machines), and can give you vnc or teamviewer without any issues. Maybe you could spot something yourself…
Ping me on irc or write an email to vit9696 at avp dot su.

vit9696 · 2017-06-13T08:54:27Z

@LordNyriox actually it is a very small change, so I added it right here.
It is especially bad because of the main menus, where the entry already says "Preferences…", and because of Dock that has Options.

To be honest, after this I felt like making all the icons greyscale on macOS and be done with it. I am not sure how to do it properly. On macOS one pretty much must use greyscale icons on toolbar, statusbar, in fact everywhere. And what you have at the moment looks terrible.

Shall I open an issue for a discussion? You have some theming pull requests here awaiting the merge, but this is not really about theming but about following the mandatory UI guidelines.

vit9696 · 2017-06-13T09:11:49Z

And just another thing, while I was changing the Preferences title I thought that something is wrong with the window. Now I see it. You use a qBittorrent icon everywhere in the windows.

On macOS we do not have a window icon unless this icon represents a file type, which is quite convenient. Also fixed everywhere.

vit9696 · 2017-06-13T09:38:32Z

In addition to the screens in the first post, here are more:

Screenshots

sledgehammer999 · 2017-07-15T17:51:37Z

Thx for the time you took to find all the little details and fix them.

Chocobo1 · 2017-07-16T03:44:19Z

Getting some errors on qbt launch:

$ src/qbittorrent 
QObject::connect: No such slot MainWindow::toggleVisibility()
QObject::connect:  (sender name:   'actionToggleVisibility')
QObject::connect:  (receiver name: 'MainWindow')
QObject::connect: No such slot MainWindow::updateTrayIconMenu()
QObject::connect:  (receiver name: 'MainWindow')
QObject::connect: No such slot MainWindow::toggleVisibility(QSystemTrayIcon::ActivationReason)
QObject::connect:  (receiver name: 'MainWindow')

vit9696 · 2017-07-16T09:50:27Z

@Chocobo1 I definitely don't have this. The only issues I see are:

$ /Applications/qbittorrent.app/Contents/MacOS/qbittorrent ; exit; 
2017-07-16 12:46:45.118 qbittorrent[34665:31468133] Path  given to -[NSWorkspace iconForFile:] is not a full path.
2017-07-16 12:46:45.118 qbittorrent[34665:31468133] Path  given to -[NSWorkspace iconForFile:] is not a full path.

And they are most certainly due to #6156.
Are you using cmake or qmake builds? I use cmake, and now I do wonder whether qmake also needs Q_OS_MAC definition for MOC. Like this one.

Chocobo1 · 2017-07-16T09:54:31Z

@vit9696
No worries, #7106 has just been merged.

Are you using cmake or qmake builds?

qmake

vit9696 · 2017-07-16T09:57:10Z

Oh, I see, thanks a lot :)

zeule · 2017-07-29T15:01:32Z

src/CMakeLists.txt

+if (GUI AND APPLE)
+    # Fix MOC inability to detect macOS. This seems to only affect cmake.
+    # Relevant issue: https://bugreports.qt.io/browse/QTBUG-58325
+    set(CMAKE_AUTOMOC_MOC_OPTIONS ${CMAKE_AUTOMOC_MOC_OPTIONS} -DQ_OS_MAC)


@vit9696, could you test CMake 3.9 and without this define, please? As of version 3.9 CMake generates moc_predefs.h as qmake does.

Tested, exactly the same error. Find reveals no such file moc_predefs.h.

Thank you. The results is quite unexpected.

D3V4N5H · 2018-07-26T17:15:02Z

Hello. I'm relatively new. I love qBittorrent. I'd like to see ETA (expected time to finish download) on the Badge instead of Global Download Speed, as it makes sense to me, to not to check now and then from the window and then hide it again.

I know C++ but not Qt.
How can I contribute?
Do I need any special development environment in my MacBook Air?

zeule reviewed Jun 12, 2017

View reviewed changes

Chocobo1 reviewed Jun 12, 2017

View reviewed changes

src/gui/mainwindow.cpp Outdated

return false;

}

void MainWindow::setupDockClickHandler() {

Copy link

Member

Chocobo1 Jun 12, 2017

Choose a reason for hiding this comment

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

move { to a newline.

thalieht reviewed Jun 12, 2017

View reviewed changes

Chocobo1 reviewed Jun 12, 2017

View reviewed changes

vit9696 closed this Jun 12, 2017

vit9696 reopened this Jun 12, 2017

Chocobo1 reviewed Jun 13, 2017

View reviewed changes

Various macOS UI improvements

d25467d

sledgehammer999 merged commit c964f0c into qbittorrent:master Jul 15, 2017

LordNyriox mentioned this pull request Jul 15, 2017

Color and font themes #6698

Closed

12 tasks

LordNyriox mentioned this pull request Jul 16, 2017

Use system file type specific icons in contents tab #6156

Merged

vit9696 mentioned this pull request Jul 19, 2017

Fix main menu item location on macOS #7122

Merged

zeule reviewed Jul 29, 2017

View reviewed changes

This was referenced Aug 10, 2017

Badge Notification on Mac #2096

Closed

OS X improvements #4757

Closed

This was referenced Aug 14, 2017

Fix macOS window restoration after using hide icon #7272

Merged

Fix notification display on macOS #7281

Merged

This was referenced Jul 22, 2018

OSX: Right click menu has giant icons #3030

Closed

Context Menu Icons are too Large #3617

Closed

Artoria2e5 mentioned this pull request Feb 7, 2023

macOS speed badge display is "wrong" by mac UI conventions #18527

Closed


		static MainWindow *dockMainWindowHandle;

		static bool dockClickHandler(id self, SEL _cmd, ...) {

Various macOS UI improvements #6952

Various macOS UI improvements #6952

Conversation

vit9696 commented Jun 12, 2017 • edited Loading

vit9696 commented Jun 12, 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

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

Chocobo1 Jun 12, 2017 • edited Loading

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 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Chocobo1 Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Chocobo1 commented Jun 12, 2017

Chocobo1 Jun 12, 2017 • edited Loading

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

Choose a reason for hiding this comment

vit9696 commented Jun 12, 2017

thalieht Jun 12, 2017 • edited Loading

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 Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Jun 13, 2017 • edited Loading

vit9696 commented Jun 13, 2017 • edited Loading

vit9696 commented Jun 13, 2017

vit9696 commented Jun 13, 2017 • edited Loading

vit9696 commented Jun 13, 2017

sledgehammer999 commented Jul 15, 2017

Chocobo1 commented Jul 16, 2017

vit9696 commented Jul 16, 2017 • edited Loading

Chocobo1 commented Jul 16, 2017

vit9696 commented Jul 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D3V4N5H commented Jul 26, 2018

vit9696 commented Jun 12, 2017 •

edited

Loading

Chocobo1 Jun 12, 2017 •

edited

Loading

vit9696 Jun 12, 2017 •

edited

Loading

Chocobo1 Jun 12, 2017 •

edited

Loading

Chocobo1 Jun 12, 2017 •

edited

Loading

Chocobo1 Jun 12, 2017 •

edited

Loading

thalieht Jun 12, 2017 •

edited

Loading

Chocobo1 commented Jun 13, 2017 •

edited

Loading

vit9696 commented Jun 13, 2017 •

edited

Loading

vit9696 commented Jun 13, 2017 •

edited

Loading

vit9696 commented Jul 16, 2017 •

edited

Loading