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

A little bit ot KF5 integration #2717

Closed
wants to merge 16 commits into from
Closed

A little bit ot KF5 integration #2717

wants to merge 16 commits into from

Conversation

zeule
Copy link
Contributor

@zeule zeule commented Mar 12, 2015

Using qBittorrent in Plasma 5 brought two displeasures for me. With dark colour scheme rows with special status in the torrents list are almost unreadable (downloading, green, is a good example of this). qBittorrent shows desktop notifications, but they are not as nice as Plasma ones.

Since KDE Frameworks 5 (KF5) supply QMake modules, it is easy to use them from qBittorrent. Here I've tried to address these problems by reading KDE colour scheme (and using its colours for the torrent list) and by using KF5 notifications component.

As a result this greatly improved the look of the main window, made lines with any status readable (depends on the colour scheme, of course). Notifications in Plasma became more functional: important ones stay visible until a user close them, "download finished" notification contains an action to open the torrent contents, and details of the notification can be tuned via standard KDE settings. Notification action is possible with pure DBus, of course, but KF5 interface is much simpler to work with.

The code shall, in principle, work with KDE4 too (maybe with minor changes), but I do not know how to hook KDE4 into the build system.

I've not implemented configure checks for KF5 (do not know how to do that), but QMake fails if KF5 modules are not found, and I hope this is enough.

@sledgehammer999
Copy link
Member

I don't want to integrate with any specific DE.
Does knotify follow the "desktop notifications spec"? If it doesn't, does it have its own DBUS spec? I prefer using it via dbus dynamically, than depending on KDE code.

About the colors. I am going to change them for v3.2.0. For light backgrounds the color scheme will be the ones shown here: http://qbforums.shiki.hu/index.php/topic,2509.msg14715.html#msg14715
I haven't investigated for a dark background yet, but I would be interested if you posted a screenshot with your colors since you already have it patched the way you like it.

@zeule
Copy link
Contributor Author

zeule commented Mar 20, 2015

May I ask why do you object to use KF5 code? Because this is, nominally, is not KDE, and all the used libraries are loaded anyway if an Qt5 app is ran under Plasma environment. You do not want to have dependencies in the source code?

The colour scheme I'm using is the following:
qbt-tango, which is slightly modified Tango KDE colour scheme. But I'm absolutely not sure that its colours will be pleasurable for anyone with different dark theme, but, of course, on dark backgrounds they are more readable than the default ones anyway. If you are interested, I will provide the colour values.

KNotify, as far as I know, follows FDO specification. However, Qt-based code to handle events via DBus will not differ significantly from that in KNotifications framework, that is why I decided to use it.

To summarise, the changes being proposed do not add any runtime overhead, they just make dependencies explicit. Overhead in the the source code is also not big, and the palette code can be made nicer, without many #ifdefs.

@sledgehammer999
Copy link
Member

May I ask why do you object to use KF5 code? Because this is, nominally, is not KDE, and all the used libraries are loaded anyway if an Qt5 app is ran under Plasma environment.

Many users use qbt under XFCE, GNOME and LXDE.

The colour scheme I'm using is the following:

The color scheme is very similar to new color scheme I pushed. What are your thoughts on that?

KNotify, as far as I know, follows FDO specification. However, Qt-based code to handle events via DBus will not differ significantly from that in KNotifications framework, that is why I decided to use it.

Can you point me to the FDO specification?
How about the DBUS specification?
The difference between dbus and KNotifications(library) usage is that qbt doesn't need KNotifications to build and run. It checks in runtime if the dbus api exists and if not falls back to other methods already used.

@pmzqla
Copy link
Contributor

pmzqla commented Apr 1, 2015

@sledgehammer999
In case you missed it, the code is ifdefed, KF5 is completely optional.

@sledgehammer999
Copy link
Member

In case you missed it, the code is ifdefed, KF5 is completely optional.

True. But I have a hard time believing that distro packagers will provide multiple packages. They will either enable KF5 support altogether or not enable it at all.

@pmzqla
Copy link
Contributor

pmzqla commented Apr 1, 2015

It's still nice for those who build qBittorrent on their own.
It's still possible that some packager might create different packages. For example Debian provides two versions of transmission, one built with Qt, the other with GTK.

EDIT: OK, maybe this case is a bit different, but what's important is that qBittorrent does not depend on the KF5.

@sledgehammer999
Copy link
Member

IMO, since we can use d-bus there is no need for KF5 libs.

@zeule
Copy link
Contributor Author

zeule commented Apr 1, 2015

Here is a draft of the spec: https://developer.gnome.org/notification-spec/
I would like to mention, that the .notifyrc file allows KDE users to configure notification behaviour (like, log to file, sound, whatever)

@sledgehammer999
Copy link
Member

We already support that spec in the current code. Maybe knotify doesn't support it?

@zeule
Copy link
Contributor Author

zeule commented Apr 1, 2015

Yes, but partially. You do not support actions, for example.

@sledgehammer999
Copy link
Member

Yes, but partially. You do not support actions, for example.

I am ok with extending it then.

@zeule
Copy link
Contributor Author

zeule commented Jul 10, 2015

While rebasing on top of the current qBittorrent master branch I encountered problems with the new TorrentHandle class. Excuse me if this is not right place to ask questions, but could, please, someone explain to me what is the purpose of the following:

class TorrentHandle : public QObject, public TorrentHandlePrivate
{
Q_DISABLE_COPY(TorrentHandle)

In particular:

  1. Why a handle can not be copied?
  2. Why it inherits QObject?
  3. What does mean "public TorrentHandlePrivate" , i.e. public inheritance from a class called XXXPrivate.
  4. How to create invalid handle?

Thank you in advance

@pmzqla
Copy link
Contributor

pmzqla commented Jul 25, 2015

I think @glassez is the right person to answer those questions.

@glassez
Copy link
Member

glassez commented Jul 25, 2015

Why a handle can not be copied?

I used this model (unlike libtorrent). Each torrent will only match a single instance of the class TorrentHandle. Therefore, the objects themselves cannot be copied. Session manages these objects, and we can only get pointers to them.

Why it inherits QObject?

It uses QObject parent-child relationship.

What does mean "public TorrentHandlePrivate" , i.e. public inheritance from a class called XXXPrivate.

TorrentHandlePrivate is an interface between TorrentHandle and Session (to avoid c++ friendship). Session object need to use some methods of TorrentHandle class, which in this case should be kept private to the outside world. Therefore, it accesses this methods through TorrentHandlePrivate interface. This may not be a good idea, but I have not found another yet.

How to create invalid handle?

Why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants