-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Optimise status colours and icons. Closes #6174. #6186
Conversation
@magao |
Urgh! That requires getting qBittorrent compiling on Windows ... definitely not the trivial task it is on Linux and something I was hoping to avoid. On the plus side, my Windows machine should be much faster at compiling than my qBittorrent Linux VM. Any chance someone who already has a Windows compile setup or dark theme could fork and pull this changeset and post a screenshot? If not, I'll see if I can set it up on the weekend. |
Thanks! Would you be able to change it to use maroon for the dark theme as well so we can compare? I suspect maroon will be too dark. |
You could use something like http://contrastchecker.com/ to test colors if you want. |
OK - so I think the colours in the pull request work as they are, so it's just a question of whether the change is desirable or not. |
Rebased on top of #6212 and uncrustified. |
The new icon scheme #4253 is based on the existing colours - stalled and paused look a little weird in my build. If this PR gets merged then might want to adjust the icon colours to match. |
I am pretty sure it's impossible to find a scheme that will fit every distribution's default colors. Maybe allow color customization via the config file, so the distribution packagers can set it up correctly? Then again, that's just another added load of bloat. |
Rebased on top of master. I'll see if I can work out how to change the colours of the icons to match to include in this PR, but I'm not a graphical person ... |
@magao The easiest method is using Hue/Saturation in Paint.net/GIMP. (But yeah, see my previous comment, if you match one theme/platform, it will look weird on the other one. Maybe just a new ini for the colors?) |
I've gone with a different approach - when loading the icons, set the icon colour to match the state colour. This will handle both light and dark themes. Doesn't require any changes to the icon files - just creates a new solid-colour pixmap masked by the transparent pixels. I've also got an alternative that I prefer that I've included in the PR for discussion (72399d3) - inverting the icons. This also doesn't require any changes to the icon files (just inverts the mask). If you like 1ea2539 and aren't sure about the others yet (or don't want them) I can split 1ea2539 out into its own PR. @Balls0fSteel Whilst I think allowing customisation of the colours might be a nice feature, I think it's outside the scope of this PR (but 1ea2539 would be a step towards it as then the icons will follow the customised colours). |
Interesting - I just realised that my change doesn't invert the status icons in the sidebar (I don't normally display them). I'll work out how to fix that and re-push. Although if this is desirable then the icon files themselves should be changed IMO - this would only be a stopgap. |
I've got a new series of commits (not pushed to github yet) which I could either leave as a single PR, or split them up. This PR should not be merged in its current state. In the options below, if the final one is where we want to end up then I think it would be best to just have everything in this single PR. For the transfer filter list, I've special-cased The following are all on a light theme - the screenshots in earlier comments show what would happen for a dark theme (in particular, Original state colours, current icons Modified state colours, current icons Original state colours, inverted icons Modified state colours, inverted icons (this is what I'll be using no matter the decision) |
@magao Very nice job. 👍 |
Note that this doesn't change anything for the web UI - the icon files will need to be changed for that (or an equivalent to what I've done be implemented in javascript). |
While joining the others who like the icons look, I would like to raise a couple of concerns regarding the code in this PR.
class Skin {
public:
QIcon icon(SkinIconEnum icon); // or maybe QString &iconName
QIcon stateIcon(TorrentState state);
QColor stateColor(TorrentState state)
}; might be a much cleaner solution. Perhaps you wanted to change the current code as little as possible, but your changes make the code even uglier. Plus, your code can not react to colour theme changes in principle. While now icons are readable when switching from light to dark colour theme (and vise-versa), with these changes reading them would become problematic.
My suggestion, therefore, is to implement the aforementioned |
I've worked out how to change the original SVG files (externally) to invert them (using masks - thank you text-based graphics format). So I'll do that (and modify the colours to match my new light theme) and then use the instructions to regenerate the PNG files. @evsh Could you post a screenshot of both the icons on a dark theme, and with anti-aliasing artifacts? I may be able to find a method of doing the fill without the artifacts. I think making more drastic changes such as how the caching works (i.e. allowing swapping between light and dark themes dynamically) needs to be a separate issue. For example, the transfer filter list is only built once - we would need to detect the theme change and then rebuild the transfer filter list. If the code is to be refactored, it should probably be done by someone who has a better grasp of the whole picture. For example, I have no idea how the web UI fits in other than the image files being used. That's the main reason I want to have as light a touch as possible. |
Here is the screenshot, please: P.S. Most of my torrents are in stalled state. Therefore, I would prefer a neutral colour for them, but now it is too reddish. |
@evsh I do feel that the Salmon is too light. It used to be used for paused as you can see in my earlier comments, and changing that was the whole point of this PR. I think we need some red in it, and I like the Maroon for the light theme, but we'd need something better for the dark theme then (as can also be seen in earlier comments, Maroon is too dark). You're probably best placed to come up with a colour you like on a dark theme. The good thing is that with the PR in its current state you can experiment just by modifying I'm glad the antialiasing isn't a problem - I was afraid that This looks relevant (though still talking about text): I see that your stalled torrents are uploading. That's not really an undesirable condition (unlike stalled downloading) - just means no one is leeching. So maybe we should give stalled uploading the same colour as uploading? |
@LordNyriox I'll compare your SVG files with mine, and take the simplest (e.g. if yours aren't done with masking, that's probably simpler for someone else to modify in the future). Just took a look ... Inkscape added a lot of gunk to the files ... maybe I can combine the 2 sets by just taking the paths from yours ... |
My point is: the most common status (stalled) should be mapped to a neutral colour (a shade of grey). I use my own modification (#3936) for the colour theme cause I don't think a single set of colours can satisfy everyone. That is why my only suggestion is to be conservative: as little different colours as possible and prefer low saturated ones, especially for elements that fill a significant part of a window. Update: and please bear in mind that on a dark background colours look more saturated than on a light one. |
@evsh Your most common status (stalled) is different to my most common status (paused). In my case, I have a lot of RSS filtering, and I have all torrents paused by default so I can pick and choose (there's often several options available). As you said, no theme is going to satisfy everyone. Stalled uploading is generally an inconsequential status, and so IMO should be either a neutral colour or the same as active uploading (royal blue on light theme, steel blue on dark theme). I'm going to implement having both uploading states be the same in my next version of the PR. Edit: tried this, disconnected my VPN, started seeding but for some reason it stayed in Seeding instead of Stalled Uploading - with zero traffic. Stalled downloading is almost always undesirable e.g. if all my torrents are stalled downloading for a while it might indicate that my VPN connection has gone down. I think it definitely deserves a warning colour (i.e. reddish). Note that there are cases that stalled uploading could indicate a problem (e.g. a networking issue). |
@sledgehammer999, would you accept a PR which implements skin colours selection by a user adding a new options page "Appearance" (possibly merging with #6196)? |
src/base/iconprovider.cpp
Outdated
@@ -35,7 +36,9 @@ IconProvider::IconProvider(QObject *parent) | |||
{ | |||
} | |||
|
|||
IconProvider::~IconProvider() {} | |||
IconProvider::~IconProvider() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we put = default
here and in GuiProvider
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand. This was just a reformatting via uncrustify. Same for other places you've mentioned this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean though - c++11 = default
. Still catching up on c++11 - much of this stuff wasn't around last time I used c++ (~10 years ago). And it's been over 20 years since I hung around on comp.std.c++ :)
src/gui/guiiconprovider.h
Outdated
static void initInstance(); | ||
static GuiIconProvider *instance(); | ||
|
||
int getThemeFlags(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QFlags<ThemeFlags>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, didn't know what to look for. Thanks.
src/gui/guiiconprovider.h
Outdated
enum ThemeFlags | ||
{ | ||
LightTheme = 1, | ||
DarkTheme = 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be only one from each pair (Dark or Light, Large or Small), because one can not have both set or cleared at a time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Not exactly. I needed to be able to distinguish between not yet knowing the theme or icon size. But if I can hook into the events (configure does it for the icon size) then that's not an issue.
src/gui/guiiconprovider.cpp
Outdated
{ | ||
static int cachedIconFlags = 0; | ||
static QHash<QPair<QString, BitTorrent::TorrentState>, QIcon> cache; | ||
int theme = getThemeFlags(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why on every icon request? Why don't you handle QEvent::PaletteChange
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that's how the dark theme was detected previously, and I simply extended it. Didn't know about QEvent::PaletteChange
- I tried to find some event that would indicate a theme change but didn't know what to look for.
Now I'll see about hooking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to hook it. GuiIconProvider
doesn't have a parent, so it doesn't seem to receive events.
I'm wondering if maybe mainwindow
should notice QEvent::ApplicationPaletteChange
and then forward it as a Preferences::changed()
signal - or maybe a new Preferences::themeChanged
signal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would catch that event in the application object. Like the Preferences::themeChanged
signal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My current prototype does that, except it's caught in MainWindow::event()
which already existed.
src/gui/guiiconprovider.cpp
Outdated
return icon; | ||
|
||
QPixmap pixmap(path); | ||
QPixmap &iconPixmap(pixmap); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reference to pixmap
which may be reassigned to colored
below, rather than unnecessarily copying objects. But I think I messed it up as colored
goes out of scope ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I introduced a bug when I fixed up the #ifdef
stuff so it didn't resize ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which may be reassigned to colored below, rather than unnecessarily copying objects.
This is wrong in C++. One needs a pointer for this kind of things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - as I updated my comment, I messed this up. A reference is actually usable for it - the problem was referencing (or pointing to) an object that went out of scope ... I've fixed this in my not-yet-pushed version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In C++ a reference can not be reassigned, that is what I wanted to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - now I think about it further, which I think was happening is that operator=()
was being called (hence safe, but no performance advantage).
So many potential gotchas in C++ - it's no wonder I prefer python these days :)
src/base/iconprovider.h
Outdated
@@ -43,7 +44,7 @@ class IconProvider : public QObject | |||
static void freeInstance(); | |||
static IconProvider *instance(); | |||
|
|||
virtual QString getIconPath(const QString &iconId); | |||
virtual QString getIconPath(const QString &iconId, const QString &overrideTheme = QString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't like this API. And don't like the fact that our icons are split onto two parts while they naturally form the single theme. UNIX users would benefit from merging them together and distributing them as individual files but not resources. Perhaps we better merge these icons into the main theme right away (for instance into a subdir "status", as per FDO specification)? Because the proposed API looks like it allows to mix icons from different themes, which shall not happen and certainly may not be encouraged by an API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't intending to change the file layout at all, but there is already a src/icons/flags/
directory, so there's precedence for doing this.
@sledgehammer999 Do you want me to reorganise so there's a src/icons/status/
directory?
|
||
Preferences::Preferences() {} | ||
Preferences::Preferences() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= default
too?
src/gui/guiiconprovider.cpp
Outdated
#endif | ||
|
||
QString GuiIconProvider::getIconPath(const QString &iconId) | ||
QString GuiIconProvider::getIconPath(const QString &iconId, const QString &overrideTheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes seem to need a rebasing on top of the current master (the signature of this function is old).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was after rebasing, and I didn't see any changes. Will become moot if @sledgehammer999 wants me to reorganise the icon hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, this is another function.
src/gui/torrentmodel.h
Outdated
@@ -33,15 +34,20 @@ | |||
#define TORRENTMODEL_H | |||
|
|||
#include <QAbstractListModel> | |||
#include <QColor> | |||
#include <QIcon> | |||
#include <QPixmap> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetical order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - <QList>
is out of order - is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
src/gui/torrentmodel.h
Outdated
#include <QList> | ||
|
||
#include "base/bittorrent/torrenthandle.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are adding a bunch of libtorrent headers for the sake of what? Those static functions below do not need access to the private members of the class. Then why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that BitTorrent::TorrentState
is a complete type. But I could get around that by the parameter instead being const BitTorrent::TorrentState &
- then a forward declaration would be enough. Because TorrentState
isn't actually an enum as I just realised ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering are these function need to be in the class interface. BTW, you may make TorrentState
an enum class
with explicit underlying type and then forward declare it, eliminating the include.
@LordNyriox My current prototype removes the direct manipulation of mainwindow and replaces it with capturing the palette change and forwarding a signal. The widgets then connect to that signal and update their icons. |
Eager to see it :) |
--HG-- branch : magao-dev
789e295
to
417ec61
Compare
@sledgehammer999 @evsh @LordNyriox @Chocobo1 Rebased on top of master and hopefully addressed all comments (either in discussion or code). I've included commit 417ec61 which moves the status icons out to their own directory - let me know if you don't want it @sledgehammer999 and I'll remove it from the PR. I've simplified the API somewhat, and in particular Theme detection now takes place in BTW I've kept the "large" status icon terminology as the current votes are currently 2 in favour (@magao, @LordNyriox) to 1 against (@evsh). I've also used At one point this was a simple 6-line PR ... |
src/gui/guiiconprovider.cpp
Outdated
#include <QApplication> | ||
#include <QBitmap> | ||
#include <QDebug> | ||
#include <QEvent> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had not found where this include is used in the code. Is it needed at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from my prototype.
src/gui/guiiconprovider.h
Outdated
#include "base/iconprovider.h" | ||
|
||
class QEvent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from my prototype.
src/gui/guiiconprovider.h
Outdated
class QIcon; | ||
template <class T, class U> class QPair; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template <class T, class U> struct QPair;
@@ -213,6 +213,16 @@ | |||
</widget> | |||
</item> | |||
<item> | |||
<widget class="QCheckBox" name="checkLargeStatusIcons"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this little tune really belong to the main options page? I would put it into the advanced page (where we have some icons-related settings already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, but the transfer list UI options seemed like an obvious place for it. It's about as significant as hiding zero and infinity values IMO.
src/gui/torrentmodel.h
Outdated
@@ -33,15 +34,19 @@ | |||
#define TORRENTMODEL_H | |||
|
|||
#include <QAbstractListModel> | |||
#include <QColor> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these new includes are unneeded. You can forward declare these classes and move the includes into the .cpp file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that because they were being returned from functions they needed to be complete types. But you can declare functions which accept or return incomplete types. Rusty C++.
https://stackoverflow.com/questions/553682/when-can-i-use-a-forward-declaration
src/gui/transferlistfilterswidget.h
Outdated
@@ -88,6 +90,8 @@ private slots: | |||
virtual void applyFilter(int row); | |||
virtual void handleNewTorrent(BitTorrent::TorrentHandle *const); | |||
virtual void torrentAboutToBeDeleted(BitTorrent::TorrentHandle *const); | |||
|
|||
int m_currentThemeFlags; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops - forgot to remove this.
BTW, there is an unanswered question from that times: this PR floods the main window with a lot of bright colours, effectively colourising all the strings in the torrent list, not only making them hard to read, but also making the main window look tasteless. I again suggest to colourise only the torrents that are in an active state which implies quick changes, i.e. those under checksum computations and maybe actively downloaded and the ones with errors. For the rest of them leave a neutral text and colourise icons only. |
In my main window, it massively reduces the amount of colour as all the paused torrents go from salmon -> black. I changed StalledUploading to be the same colour as Uploading - did that not fix your problem? If not, what state are the torrents in that you're complaining about? I would suggest that once this is eventually merged you create a PR with your suggested changes. It would need a separate Or thinking about this further - maybe have all the text neutral, and just rely on the icon colour? I've currently got both versions side-by-side (light theme), and I think I like the text being coloured. |
Let me say, that I didn't read any of this while it happened. So the discussion is pretty huge for me. I skimmed through it and I realize that it involves changes to font colors, icons(shape and color) and skins? So @magao can you give me a summary on what is happening here? |
@sledgehammer999 As the PR currently stands:
But see earlier comment.
Note: I've though about reversing this - make the icons files be the larger size, then add transparent space in memory. That way we wouldn't be relying on the amount of excess space in the icon files. Thoughts? One issue is that the icons would then always be larger in the web UI.
|
--HG-- branch : magao-dev
@sledgehammer999 @evsh @LordNyriox @Chocobo1 Rebased on top of master. Addressed review comments. Top two commits are for discussion: beb94e9: status icons in own directory |
Quick review from your #6186 (comment): |
If I create the Appearance options tab as discussed in PR #6196 then the issue could become moot by having options to override the colours. |
True. So we can keep the current defaults and user can change them. |
In that case do you want me to resubmit this PR without the colour change, and if so do you want the status icons in their own directory? I think at that point it should be ready for merging. |
For the moment keep it as it is. Let's see how the Appearance tab implementation will go. |
To me, the new look is superb (with smaller icons)! |
Will this be ready for 3.4? |
What is the status of this PR? Is it still relevant? |
This pull request addresses issue #6174.
Swaps the colours used for the paused downloading and stalled states, and makes the stalled state use maroon for a light theme (which I find easier to read).
The reasoning here is that paused downloading is a "normal" state, whereas stalled is a "warning" state, so the colours should reflect that.