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

Use system theme icons #458

Merged
merged 4 commits into from Jun 15, 2015

Conversation

Projects
None yet
3 participants
@benedictleejh
Contributor

benedictleejh commented May 23, 2015

With KDE's new Plasma 5 Workspace, some of the icons are looking very much out of place with the new Breeze icon theme. So I've basically made git-cola take icons from the system whenever possible, instead of using its own. It does still fallback on its own icons if it can't find a system icon with the provided name.

This should help it blend better into Plasma 5's looks, with the nice side effect of probably also blending in better with other desktop environments as well.

Signed-off-by: Benedict Lee benedictleejh@gmail.com

benedictleejh added some commits May 23, 2015

Rename undo.svg to edit-undo.svg
This allows the use of the system theme icon for undo, which by
FreeDesktop standards is named edit-undo
Use system theme icons when getting icons
Prefer system icons when getting icons, falling back to the provided
icons only when the icon doesn't exist
@Vdragon

This comment has been minimized.

Show comment
Hide comment
@Vdragon

Vdragon May 23, 2015

Contributor

Would you make a screenshot of Git Cola using system theme icons? I wonder what It'll look like after the patch?

Contributor

Vdragon commented May 23, 2015

Would you make a screenshot of Git Cola using system theme icons? I wonder what It'll look like after the patch?

@benedictleejh

This comment has been minimized.

Show comment
Hide comment
@benedictleejh

benedictleejh May 23, 2015

Contributor

Here are a couple of screenshots. Some of it is hard to take screenshots for, because they're in the menus. And a lot of the icons already use Qt to get the right icons in the first place, so, by and large, it already fit in reasonably well. It's just that it becomes very obvious when it doesn't, because Breeze is visually very different from Oxygen, which the original icons did a decent job of fitting well in.

Not all icons are changed though, some because they have no facsimile in the system, others because I'm unsure about them. The changed icons mostly lie in the menus and buttons, because those, I find, should be more integrated into the system look.

recently-modified
edit-remotes-dialog

Contributor

benedictleejh commented May 23, 2015

Here are a couple of screenshots. Some of it is hard to take screenshots for, because they're in the menus. And a lot of the icons already use Qt to get the right icons in the first place, so, by and large, it already fit in reasonably well. It's just that it becomes very obvious when it doesn't, because Breeze is visually very different from Oxygen, which the original icons did a decent job of fitting well in.

Not all icons are changed though, some because they have no facsimile in the system, others because I'm unsure about them. The changed icons mostly lie in the menus and buttons, because those, I find, should be more integrated into the system look.

recently-modified
edit-remotes-dialog

Show outdated Hide outdated cola/qtutils.py
def options_icon():
"""Return a standard open directory icon"""
return icon('options.svg')
return QtGui.QIcon.fromTheme("configure", icon('options.svg'))

This comment has been minimized.

@davvid

davvid May 24, 2015

Member

Sorry for the style nit, but can you please use single-quotes here so that it's consistent with the rest of the line?

In a follow-up commit, I might be tempted into refactoring theme_icon() to allow for this kind of invocation by allowing for an optional 2nd argument. That way these calls can all use theme_icon(). Don't worry about that now, though, this'll be fine once the quote thing is fixed-up.

@davvid

davvid May 24, 2015

Member

Sorry for the style nit, but can you please use single-quotes here so that it's consistent with the rest of the line?

In a follow-up commit, I might be tempted into refactoring theme_icon() to allow for this kind of invocation by allowing for an optional 2nd argument. That way these calls can all use theme_icon(). Don't worry about that now, though, this'll be fine once the quote thing is fixed-up.

@davvid davvid merged commit 3cab49b into git-cola:master Jun 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jun 15, 2015

Member

Merged, thanks. I made some small tweaks to accommodate older versions of Qt in aaa3543 as a follow-up.

Member

davvid commented Jun 15, 2015

Merged, thanks. I made some small tweaks to accommodate older versions of Qt in aaa3543 as a follow-up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment