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

Read and use KDE 4/5 colours for torrents list, torrent progress bars, and log window #3936

Closed
wants to merge 1 commit into from

Conversation

zeule
Copy link
Contributor

@zeule zeule commented Oct 13, 2015

Resurrection of pull request #3819. But now with an option to enable/disable (disabled by default) usage of the system colour theme, and of course without build or run-time dependencies on KF5/KDE4.

The used KDE colours are strongly connected with the torrent states (neutral/negative/positive), shows KDE colour effects (disabled and inactive) and therefore, I believe, are understandable and pleasant for KDE users. However, to implement the effects, colorutils .h + .cpp files from KF5 are directly included. But they are small files.

If XDG_CURRENT_DESKTOP environment variable suggests KDE session, read kdeglobals config file and use defined there colours for torrent list. Otherwise hard-coded default values are used.

@@ -0,0 +1,325 @@
#include "./torrentmodel_p.h"
Copy link
Member

Choose a reason for hiding this comment

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

#include "torrentmodel_p.h"

Copy link
Member

Choose a reason for hiding this comment

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

Move this include at the rest of project includes (after utils/colorutils.h).
Don't mix system/Qt/project includes.

@glassez
Copy link
Member

glassez commented Oct 14, 2015

For a start, fix coding style.

@sledgehammer999
Copy link
Member

As I said, on the other PRs, I am not sure I want to enable custom color values even if they're read from the theme.
Just so you know, I'll consider this PR almost last for v3.3.0 and there is a possibility of rejection.

Also I think you could get the same colors in a DE-agnostic way. Get the application's QPalette and use QPalette::color(<group>, QPalette::Text) and change the group values.

@zeule
Copy link
Contributor Author

zeule commented Oct 14, 2015

@sledgehammer999 , could you explain me the difference between icons and colours, please? qBt uses system icons and benefits from it. Why can't it use colours then?

QPalette was considered, but it does not have required colours.

@zeule
Copy link
Contributor Author

zeule commented Oct 14, 2015

I was looking for a good bittorrent client for Plasma desktop. Since KTorrent is stagnating, I was happy to find that qBt support Qt5. The backend is powerful. Its UI is nicely balanced and fits into Plasma desktop quite well. The only two big problems for me were desktop notifications and colours. I think qBt has good chances to be popular among Plasma users if it becomes a bit more friendly to them.

@zeule
Copy link
Contributor Author

zeule commented Mar 15, 2016

@sledgehammer999, if you are not going to accept these changes, I will abandon them and instead implement a proper KF5/Plasma integration for myself. Could you let me know this, please, in a foreseeable future?

By 'KF5/Plasma integration' I mean the following:

  1. Usage of the active colour theme in the torrents list and the torrents progress bars.
  2. DBus interface to qBt
  3. KNotify config files to set up the notification preferences.
  4. Probably a plasmoid to show status and basic controls.

@sledgehammer999
Copy link
Member

@evsh I am not rejecting this outright. I haven't even looked at it. I hope get the time to do it soon. But as you can see I am little bit swamped in PRs :P

@sledgehammer999
Copy link
Member

By 'KF5/Plasma integration' I mean the following:

Without looking too much into it: If you are going to somehow maintain "KDE intergration" in the foreseeable future AND that integration code isn't touching too much of qbt code AND it is optional for the user I'll consider it seriously.

When I say isn't touching too much qbt code I mean that it is its own set of files/folders, it doesn't introduce big changes and I can easily rip it out if it gets outdated and nobody wants to maintain it.

@zeule
Copy link
Contributor Author

zeule commented Mar 17, 2016

Thank you for the quick and informative answer! I have no doubts that I will use qBt and KDE in the foreseeable future and as such will be able to maintain the code. However, I doubt that I can handle it without polluting files with #ifdefs. Take the colour changes: I have to either 1) modify qBt code or 2) provide fully customizable colour schemes for qBt and implement KDE colour scheme there. The option 2 gives complete independence but is not lightweight. Would you accept such a change? BTW, colour blind persons might also benefit from it. The other additions (DBus interface, KNotify and a plasmoid) have to be implemented in their own files and interfere with the main codebase only in the make system. And what do you mean by "optional for the user"? Compile or run time optional? Or both? Dynamically linked?

@glassez
Copy link
Member

glassez commented Mar 17, 2016

And what do you mean by "optional for the user"? Compile or run time optional? Or both? Dynamically linked?

I think It should be implemented as compile time option, e.g.:

configure --enable-kde-integration

@glassez
Copy link
Member

glassez commented Mar 17, 2016

However, I doubt that I can handle it without polluting files with #ifdef s.

Indeed, you cannot do without #ifdefs. But to pollute the code is not necessary, if you concentrate it in one or two files, using the appropriate design pattern (e.g. Abstract Factory).

@zeule
Copy link
Contributor Author

zeule commented Mar 17, 2016

I think It should be implemented as compile time option

@glassez , it was already implemented like that in one of the previous versions of this PR, and the maintainer did not like it. That is why I asked.

@glassez
Copy link
Member

glassez commented Mar 17, 2016

@evsh, I looked at the current implementation. Overall, it looks good. It is rather structured and it can be easily changed to make KDE support as compile time option.
Of course, I have some comments. But, apparently, it is pointless to talk about it, if it will be severely altered on.

@zeule
Copy link
Contributor Author

zeule commented Mar 17, 2016

@glassez, this code was written when @sledgehammer999 refused to accept a more direct approach. Initially there was a straightforward code which called KF5 to get the colour corrected for the colour effects. @sledgehammer999 said that he does not want build time dependency on KF5 libraries, because package maintainers will likely to turn it on always. However, if there will be an option in the build system, I would use the direct approach and remove colorutils.[h,cpp] files, parsing of the KDE config file, etc. When an Qt5 app is executed in Plasma environment, all these KF5 libraries are get loaded anyway by the Qt platform integration layer.

@glassez
Copy link
Member

glassez commented Mar 17, 2016

Wait for @sledgehammer999. He's the last word. It will depend on what form he allows to have it here.

However, if there will be an option in the build system, I would use the direct approach and remove colorutils.[h,cpp] files, parsing of the KDE config file, etc.

Could you describe this solution in General for me. How would this be implemented. I'm more interested in right now, what dependencies it would add and what those dependencies would be. How will this affect the current code.

@zeule
Copy link
Contributor Author

zeule commented Mar 17, 2016

OK, with pleasure. Here we go:
0. General: options in the build system to compile with KF5 support.

  1. Colours: A class to hold colour theme for the app (colours for the torrent list, torrent progress bars, maybe for something else too). A property page in options to customize these colours, which should be comfortable for colour blind persons. At the same page a check box ("use system colours", perhaps if XDG_CURRENT_DESKTOP points to a KDE, "Use Plasma colours") and a reset option. Naturally, there will be code to populate this palette with either qBt default colours, Plasma colours, or saved custom user colours.
  2. DBus: In a separate files create ancestor of the QDBusAbstractAdaptor class and call appropriate functions using the application singletons. I so far can think of actions like start/stop, getting torrent list, getting torrent info. Signals about torrent completion and maybe errors are also can be routed to DBus. All these items can be used in scripting and from the plasmoid.
  3. KNotify: the task is to allow user to configure such notifications properties as notify sounds, taskbar highlighting, priorities, etc. For that a description file has to be provided and installed. In this file types of notifications have to be listed and default properties set. Then, signalling a notifications, we supply the same ID hint and KNotify may read user-customized settings for this notification kind.
  4. Plasmoid: well, just a plasmoid; code in a separate dir. Talks to the app via DBus. Shows status and basic actions. The code most likely will not require compilation, just Qt Quick + JS and SVG files.

@sledgehammer999
Copy link
Member

I forgot to mention that I don't want dependencies on (any) DE libs.
Can you query the colors via DBUS on KDE or do you have to use a KDE lib for that?

A little bit of #ifdef isn't a problem.

And what do you mean by "optional for the user"?

As a runtime option.

@zeule
Copy link
Contributor Author

zeule commented Mar 18, 2016

@sledgehammer999, no I don't know any remote API to get colours. Approach from this PR is the best I can imagine for KDE.

As a runtime option.

So, it means a plugin then?

@glassez
Copy link
Member

glassez commented Mar 18, 2016

@evsh, IMO, approach from this PR is runtime option because it determines whether to use KDE colors or not in runtime.

@glassez
Copy link
Member

glassez commented Mar 18, 2016

If it suits @sledgehammer999, I start to comment on code. Waiting...
@evsh, by the way, you can fix the coding style now.


#include "torrentmodel_p.h"

#if (defined(Q_OS_UNIX) && !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.

just Q_OS_LINUX ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about BSD systems?

Copy link
Member

Choose a reason for hiding this comment

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

you're right. I haven't consider that before. so non-intuitive...

@Chocobo1
Copy link
Member

OK, with pleasure. Here we go:

I'm not sure how much other DE will benefit from this, probably a minor thing but I would like KF5 specific code can be placed into a distinct folder in the source tree.
I guess that applies to point 3 & 4, not sure about point 2.

@sledgehammer999
Copy link
Member

Can you query the colors via DBUS on KDE or do you have to use a KDE lib for that?

@evsh ?

@zeule
Copy link
Contributor Author

zeule commented Mar 18, 2016

@sledgehammer999 , you must have overlooked my earlier answer. Here it follows with yet another question from myself:

@sledgehammer999, no I don't know any remote API to get colours. Approach from this PR is the best I can imagine for KDE.

As a runtime option.

So, it means a plugin then?

@zeule
Copy link
Contributor Author

zeule commented Mar 18, 2016

@Chocobo1

I'm not sure how much other DE will benefit from this, ...

I think scripting using DBus might be useful. One can do things (and I plan to to them) like "wait two hours, if download progress does not change, suspend the system" or "Once a given file gets ready, remove all other downloads with it or downlods with specific hashes". I think GUI for such tasks in qBt is an overkill. Speaking of colours, I can imagine the following way to do this for other DEs. We use python already, then why don't we supply a python programs which get DE palette and print it out. pygobject can be used for GTK3, for example. Then we can detect DE (via $XDG_CURRENT_DESKTOP), launch appropriate program if user want the system colour theme.

@Chocobo1
Copy link
Member

I think scripting using DBus might be useful. .... I think GUI for such tasks in qBt is an overkill.

True, an external scripting interface (that's how I interpret) can be very useful, in the sense of devs can create more user-friendly programs.

...launch appropriate program if user want the system colour theme.

pardon me, I don't follow.
A python GUI program when DE is using gtk?

So, it means a plugin then?

sounds cool (for me).

@zeule
Copy link
Contributor Author

zeule commented Mar 18, 2016

A python GUI program when DE is using gtk?

Sorry for being not clear enough. I mean that we can launch a helper python script to get DE colour palette.

@zeule
Copy link
Contributor Author

zeule commented Mar 18, 2016

@evsh, by the way, you can fix the coding style now.

@glassez, uncrustfied

@zeule zeule changed the title Read and use KDE 4/5 colours for torrents list Read and use KDE 4/5 colours for torrents list, torrent progress bars, and log window Mar 30, 2016
@zeule
Copy link
Contributor Author

zeule commented Mar 30, 2016

@sledgehammer999 , FYI log window is hardly readable with a dark colour theme; it needs the same colour adjustments as you did for the torrent list (isDarkTheme() and the rest)

If XDG_CURRENT_DESKTOP environment variable suggests KDE session,
read kdeglobals config file and use defined there colours for
torrent list, torrent progress bars, and log window.
Otherwise hardcoded default values are used.
@zeule zeule mentioned this pull request Apr 26, 2017
12 tasks
@zeule
Copy link
Contributor Author

zeule commented May 8, 2017

Superseded by #6698.

@zeule zeule closed this May 8, 2017
@zeule zeule deleted the kf5-colors branch May 8, 2017 09:54
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