Ability to use custom icon styles + dark icon set. #638

Merged
merged 4 commits into from Jan 18, 2017

Conversation

Projects
None yet
2 participants
@cofi89
Contributor

cofi89 commented Jan 3, 2017

Hi David,

This being my first time playing with Cola source likelly means that there are better ways to do this.
But anyway, what I did here is hopefully good, works in my somewhat limited testing, and is in short:

  1. Added a "dark" icon set ( besides color inversion done with Inkscape, it's unmodified original one ), for use with dark themes
  2. Added ability to load custom styles set via git config ( docs updated as well )

In detail, custom icons:

  • Are placed in a subdirectory of the main "icons" dir
  • Use the same filenames and format as the original ones
  • Are selected via gitconfig with cola.iconstyle style_name, where "style_name" coresponds to the name of the subdir containing the icons
    • If cola.iconstyle is unset, or "style_name" doesn't match any subdir in "icons", or "style_name" == "light", fallback to the default icons

What's missing:

  1. Perhaps look for icons in "~/.config/git-cola/icons" as well?
  2. Expose style setting in Preferences GUI
  3. Reload icons on the fly ( at least when set from GUI )?

Tested and working on:

  1. Debian Stretch ( py2/3.5 + qt4/5 )
  2. Windows 7 ( py3.5 + qt4 ). 馃槂

In action:

....

References: #568

cofi89 added some commits Jan 3, 2017

Add dark icon style
These icons are the color inverted original ones ( otherwise umodified )

Signed-off-by: Filip Danilovi膰

<filip@openmailbox.org>
doc: document cola.iconstyle
Signed-off-by: Filip Danilovi膰

<filip@openmailbox.org>
Add ability to load custom icons
Custom icon theme consists of a subdirectory of the main "icons" dir
containing all icons existing in the original theme, using the same
names and same file format (svg)

Theme is set, as usual, with git config:
git config (--global) cola.iconstyle style_name

Where "style_name" is the exact name of the dir containing the icons.

If the specified style doesn't exist, is set to "light", or not set at
all, original icons will be used.

Signed-off-by: Filip Danilovi膰

<filip@openmailbox.org>
setup: install dark icon set
Signed-off-by: Filip Danilovi膰

<filip@openmailbox.org>

@cofi89 cofi89 changed the title from Ability to use custom icon styles + dark icon set. References #568 to Ability to use custom icon styles + dark icon set. Jan 3, 2017

+ if not style or style == "light":
+ icon_dir = resources.icon_dir()
+ else:
+ icon_dir = resources.icon_dir(style)

This comment has been minimized.

@davvid

davvid Jan 7, 2017

Member

tiny tweak: for keyword arguments, prefer doing icon_dir(style=style) at the callsite.

@davvid

davvid Jan 7, 2017

Member

tiny tweak: for keyword arguments, prefer doing icon_dir(style=style) at the callsite.

This comment has been minimized.

@cofi89

cofi89 Jan 8, 2017

Contributor

馃憤 Will do, thanks for the correction.

@cofi89

cofi89 Jan 8, 2017

Contributor

馃憤 Will do, thanks for the correction.

This comment has been minimized.

@davvid

davvid Jan 18, 2017

Member

I think this is really the main change that I keep coming back to as a slightly unfortunate change in the relative dependencies. Previously the icons module was more pure, and didn't import anything, and we didn't have to call into git so early (during module import) so that we can defer a few things.

Sorry it's taken so long to get back to this one, but it's become a little clearer with time. Would it be okay if we made this something that's controlled via the environment instead?

There's already precedent for a few of these in the ENVIRONMENT VARIABLES section of docs/git-cola.rst, namely GIT_COLA_SCALE which is directly related to the UI. The reason these are done via the environment is that they affect the critical startup code paths that we've tried to keep as minimal as possible.

Would it be okay if we changed the style to be a lookup of os.environ.get('GIT_COLA_ICON_THEME', 'light')? Presumably we can then make the style optional argument into a required argument, and special-case it inside of icon_dir() instead.

I may merge this soon with these changes applied locally, which hopefully won't be too much of a hassle to upgrade to.

Example .bashrc/.xsessionrc:

GIT_COLA_ICON_THEME=dark
export GIT_COLA_ICON_THEME
@davvid

davvid Jan 18, 2017

Member

I think this is really the main change that I keep coming back to as a slightly unfortunate change in the relative dependencies. Previously the icons module was more pure, and didn't import anything, and we didn't have to call into git so early (during module import) so that we can defer a few things.

Sorry it's taken so long to get back to this one, but it's become a little clearer with time. Would it be okay if we made this something that's controlled via the environment instead?

There's already precedent for a few of these in the ENVIRONMENT VARIABLES section of docs/git-cola.rst, namely GIT_COLA_SCALE which is directly related to the UI. The reason these are done via the environment is that they affect the critical startup code paths that we've tried to keep as minimal as possible.

Would it be okay if we changed the style to be a lookup of os.environ.get('GIT_COLA_ICON_THEME', 'light')? Presumably we can then make the style optional argument into a required argument, and special-case it inside of icon_dir() instead.

I may merge this soon with these changes applied locally, which hopefully won't be too much of a hassle to upgrade to.

Example .bashrc/.xsessionrc:

GIT_COLA_ICON_THEME=dark
export GIT_COLA_ICON_THEME

This comment has been minimized.

@davvid

davvid Jan 19, 2017

Member

Just following up -- not sure if you noticed but I ended up keeping the config-driven approach, and ended up augmenting it with --icon-theme=<theme> as well as allowing GIT_COLA_ICON_THEME in the environment. The only other minor difference is that the config variable is named cola.icontheme whereas it was originally cola.iconstyle.

@davvid

davvid Jan 19, 2017

Member

Just following up -- not sure if you noticed but I ended up keeping the config-driven approach, and ended up augmenting it with --icon-theme=<theme> as well as allowing GIT_COLA_ICON_THEME in the environment. The only other minor difference is that the config variable is named cola.icontheme whereas it was originally cola.iconstyle.

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jan 7, 2017

Member

Awesome, this is pretty cool. I'm currently in the middle of switching ISPs so I have spotty internet access until next week, but I'll try to take a look soon so that we can move forward with this topic.

I like several aspects of this idea:

  • it's opt-in, which is great
  • we ship an alternate set, which makes it easy for folks to copy it and customize as needed

I like the idea of allowing ~/.config/git-cola/icons/ to provide alternative icon theme. Would cola.icontheme be a more descriptive name than iconstyle?

I have an idea that we could also use to generalize cola.icontheme. If it's set, and it's an absolute path that resolves to a directory, then we will use that path for icons. That means that users have the option of a) using a short name that corresponds to a theme shipped by cola or provided in their ~/.config/git-cola/icons directory, or b) using an absolute path to a directory.

Member

davvid commented Jan 7, 2017

Awesome, this is pretty cool. I'm currently in the middle of switching ISPs so I have spotty internet access until next week, but I'll try to take a look soon so that we can move forward with this topic.

I like several aspects of this idea:

  • it's opt-in, which is great
  • we ship an alternate set, which makes it easy for folks to copy it and customize as needed

I like the idea of allowing ~/.config/git-cola/icons/ to provide alternative icon theme. Would cola.icontheme be a more descriptive name than iconstyle?

I have an idea that we could also use to generalize cola.icontheme. If it's set, and it's an absolute path that resolves to a directory, then we will use that path for icons. That means that users have the option of a) using a short name that corresponds to a theme shipped by cola or provided in their ~/.config/git-cola/icons directory, or b) using an absolute path to a directory.

@cofi89

This comment has been minimized.

Show comment
Hide comment
@cofi89

cofi89 Jan 8, 2017

Contributor

Thanks. No problem & no rush.

Would cola.icontheme be a more descriptive name than iconstyle?

It would indeed. I guess I was leaning towards "style" because of "light" vs. "dark". Seemed more fitting...

I have an idea that we could also use to generalize cola.icontheme. If it's set, and it's an absolute path that resolves to a directory, then we will use that path for icons.

+1 馃憤
From the top of my head:
https://gist.github.com/cofi89/2a9931c9e3ec630366f6a68a5577ec59
Works fine, judging by the quick test ( on Linux ).
I'll get back to you once I test it properly ( Win included ).


There's one issue with custom icons, and that is if you specify an existing directory which has only a partial set, or no icons at all, Cola will start, but there will be missing icons ( or none at all ).
I guess it would be for the best to just fallback to the default theme for all, or just missing icons, however I'm not sure what would be the best way to handle/implement this, besides checking files in "theme" dir against a list!?
And while I'm at it, if we fix the above, it would be nice to provide an config option for a fallback theme, both of which would essentially allow for partial themes/inheritance ( linux like ).

Contributor

cofi89 commented Jan 8, 2017

Thanks. No problem & no rush.

Would cola.icontheme be a more descriptive name than iconstyle?

It would indeed. I guess I was leaning towards "style" because of "light" vs. "dark". Seemed more fitting...

I have an idea that we could also use to generalize cola.icontheme. If it's set, and it's an absolute path that resolves to a directory, then we will use that path for icons.

+1 馃憤
From the top of my head:
https://gist.github.com/cofi89/2a9931c9e3ec630366f6a68a5577ec59
Works fine, judging by the quick test ( on Linux ).
I'll get back to you once I test it properly ( Win included ).


There's one issue with custom icons, and that is if you specify an existing directory which has only a partial set, or no icons at all, Cola will start, but there will be missing icons ( or none at all ).
I guess it would be for the best to just fallback to the default theme for all, or just missing icons, however I'm not sure what would be the best way to handle/implement this, besides checking files in "theme" dir against a list!?
And while I'm at it, if we fix the above, it would be nice to provide an config option for a fallback theme, both of which would essentially allow for partial themes/inheritance ( linux like ).

@davvid davvid merged commit d65eb0b into git-cola:master Jan 18, 2017

1 check passed

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

davvid added a commit that referenced this pull request Jan 18, 2017

Merge pull request #638 from cofi89/dark-icons
* cofi89/dark-icons:
  setup: install dark icon set
  Add ability to load custom icons
  doc: document cola.iconstyle
  Add dark icon style

Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Jan 18, 2017

icons: set the icon theme using the environment
Control the git-cola icon theme using the GIT_COLA_ICON_THEME
environment variable.

Related-to: #638
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Jan 18, 2017

icons: dos2unix the dark icons README
Related-to: #638
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Jan 18, 2017

icons: dos2unix the dark icon theme
Related-to: #638
Signed-off-by: David Aguilar <davvid@gmail.com>

davvid added a commit that referenced this pull request Jan 18, 2017

icons: allow specifying the icon theme via config and command line
Allow `--icon-theme=<theme>` to override the icon theme.
Add back the original config-based theme setting as well, since there is
relatively little overhead to doing so.

Related-to: #638
Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Jan 19, 2017

Member

Regarding having a directory with a partial set of icons -- I think we can fix that by allowing cola.icontheme to contain multiple values.

e.g.

git config --add cola.icontheme /path/to/custom/icons
git config --add cola.icontheme dark

... and then in cola we just need to call addSearchPath(...) with the resolved paths in that order. Qt should then look in the custom path first, and fallback to dark icons.

I'll try implementing that shortly.

Member

davvid commented Jan 19, 2017

Regarding having a directory with a partial set of icons -- I think we can fix that by allowing cola.icontheme to contain multiple values.

e.g.

git config --add cola.icontheme /path/to/custom/icons
git config --add cola.icontheme dark

... and then in cola we just need to call addSearchPath(...) with the resolved paths in that order. Qt should then look in the custom path first, and fallback to dark icons.

I'll try implementing that shortly.

davvid added a commit to davvid/git-cola that referenced this pull request Jan 19, 2017

icons: allow specifying multiple icon themes
Allow cola.icontheme and GIT_COLA_ICON_THEME to contain multiple values.
This allows for partial icon theme overriding by specifying a partial
theme before specifying a fallback theme.

Related-to: #638
Signed-off-by: David Aguilar <davvid@gmail.com>

@davvid davvid added the usability label Jan 19, 2017

@cofi89

This comment has been minimized.

Show comment
Hide comment
@cofi89

cofi89 Mar 8, 2017

Contributor

Hey @davvid,
Sorry for responding this late, I've been AFK due to unexpected workload combined with ongoing home renovation/moving.

Awesome job, esp. on the addition of the cmd line parameter. Thank you very much for accepting and much improving this! 馃憤 馃槂

Now, while this is perfectly fine as-is ( IMO atleast ), I'd like to mention/talk about one tidbit I left out originally to keep PR on topic:
Is there a reason/need for default icons to be as large as they are ( ~896 x 1024 )?
Excluding the cola logo, all the icons are essentially oversized, even considering HiDPI screens. And also, why svg's?

AFAIK, raster images render faster and ( when downsized ) are certanly going to end up much smaller in file size, thus reducing start-up time, however small and negligible the impact may be ( atleast on modern machines with SSD's )...?

In short, is there a reason not to downsize the default icons to, say 32-48px ( which should also cover for HiDPI ), and convert them to png?

Contributor

cofi89 commented Mar 8, 2017

Hey @davvid,
Sorry for responding this late, I've been AFK due to unexpected workload combined with ongoing home renovation/moving.

Awesome job, esp. on the addition of the cmd line parameter. Thank you very much for accepting and much improving this! 馃憤 馃槂

Now, while this is perfectly fine as-is ( IMO atleast ), I'd like to mention/talk about one tidbit I left out originally to keep PR on topic:
Is there a reason/need for default icons to be as large as they are ( ~896 x 1024 )?
Excluding the cola logo, all the icons are essentially oversized, even considering HiDPI screens. And also, why svg's?

AFAIK, raster images render faster and ( when downsized ) are certanly going to end up much smaller in file size, thus reducing start-up time, however small and negligible the impact may be ( atleast on modern machines with SSD's )...?

In short, is there a reason not to downsize the default icons to, say 32-48px ( which should also cover for HiDPI ), and convert them to png?

@davvid

This comment has been minimized.

Show comment
Hide comment
@davvid

davvid Mar 8, 2017

Member

Oh, there's not a reason for them to be as large as they are, that's just what's specified at the top of the file. A lot of the icons came from github's "octicons" icon set. What I really like about them is how tiny they are -- many of them are only ~300 bytes.

svg is nice because it's resolution-independent. On Linux, at least, the icons are reasonably sized since Qt just scales it down to the size of the button. If we could just edit the svg text and size them down that would be okay, as long as the embedded paths are still valid.

Are you seeing cases where the icons are too large when rendered by the app? I'd be interested in fixing those. We did have an open issue about the icons getting larger on OS X after switching to svg icons, and in that issue #550 there's additional notes about some things that were tried.

I would like to avoid pngs. We had png icons in the past and they were ugly, but maybe there's something we can do app-side. What kinda issues are you sseing?

My main test machines these days are a HiDPI linux laptop and a non-hidpi linux desktop, so I appreciate help from other angles.

Member

davvid commented Mar 8, 2017

Oh, there's not a reason for them to be as large as they are, that's just what's specified at the top of the file. A lot of the icons came from github's "octicons" icon set. What I really like about them is how tiny they are -- many of them are only ~300 bytes.

svg is nice because it's resolution-independent. On Linux, at least, the icons are reasonably sized since Qt just scales it down to the size of the button. If we could just edit the svg text and size them down that would be okay, as long as the embedded paths are still valid.

Are you seeing cases where the icons are too large when rendered by the app? I'd be interested in fixing those. We did have an open issue about the icons getting larger on OS X after switching to svg icons, and in that issue #550 there's additional notes about some things that were tried.

I would like to avoid pngs. We had png icons in the past and they were ugly, but maybe there's something we can do app-side. What kinda issues are you sseing?

My main test machines these days are a HiDPI linux laptop and a non-hidpi linux desktop, so I appreciate help from other angles.

@cofi89

This comment has been minimized.

Show comment
Hide comment
@cofi89

cofi89 Mar 9, 2017

Contributor

No, actually I haven't run into any issues per se, I'm just curious as to your thoughts, for the (primary) reason I'll get to later, and 'cause the size seems to be a bit unnecessarily large, which makes me think it makes Cola slower to start, due to resizing overhead and vector vs. raster, though it's nothing I've tested extensively/have any hard data on.

But, I've had some issues with Inkscape color inversion preparing this PR, which made all svg's unrenderable by Cola/QT, so before finding/fixing the cause I converted them to XX*48 png's, ran a sed -i 's/.svg/.png/g' icons.py and it just worked, at least on Linux. No rendering issues, no missing icons, and on an quite old Sempron box it seemingly made Cola start faster.
However I did somehow manage to lose those png's, so once I have enough free time I'll do the conversion once more, try to come up with meaningfull tests/data and report back.

If we could just edit the svg text and size them down that would be okay, as long as the embedded paths are still valid.

Nope, I beleive that paths need to be changed as well ( not 100% sure, but AFAIK, it's either still gonna render at original size, or not at all ).


And the other reason for asking about png vs svg I've mentioned, is that I've had this idea to make a set for Cola based on Faenza action icons, however I suck at vector graphics ( raster as well, though not as badly ). 馃槃
Which, besides me, might also be a "blocker" for other people wishing to contribute a set.
Point being, it would be great to make Cola able to load icons just by file name ( withouth extension that is ), and let QT deal with the actual format, instead of hard-coding svg's. Eg:

def merge():
    return icon('git-merge')   # Returns either png or svg

So you can just drop a correctly named icons into "icons" dir, and have it JustWork鈩, regardless of image format.
I'm more than willing to work on it, and AFAIK it shouldn't require much changes/bloat code wise, however it might take a while before I get to it.

Contributor

cofi89 commented Mar 9, 2017

No, actually I haven't run into any issues per se, I'm just curious as to your thoughts, for the (primary) reason I'll get to later, and 'cause the size seems to be a bit unnecessarily large, which makes me think it makes Cola slower to start, due to resizing overhead and vector vs. raster, though it's nothing I've tested extensively/have any hard data on.

But, I've had some issues with Inkscape color inversion preparing this PR, which made all svg's unrenderable by Cola/QT, so before finding/fixing the cause I converted them to XX*48 png's, ran a sed -i 's/.svg/.png/g' icons.py and it just worked, at least on Linux. No rendering issues, no missing icons, and on an quite old Sempron box it seemingly made Cola start faster.
However I did somehow manage to lose those png's, so once I have enough free time I'll do the conversion once more, try to come up with meaningfull tests/data and report back.

If we could just edit the svg text and size them down that would be okay, as long as the embedded paths are still valid.

Nope, I beleive that paths need to be changed as well ( not 100% sure, but AFAIK, it's either still gonna render at original size, or not at all ).


And the other reason for asking about png vs svg I've mentioned, is that I've had this idea to make a set for Cola based on Faenza action icons, however I suck at vector graphics ( raster as well, though not as badly ). 馃槃
Which, besides me, might also be a "blocker" for other people wishing to contribute a set.
Point being, it would be great to make Cola able to load icons just by file name ( withouth extension that is ), and let QT deal with the actual format, instead of hard-coding svg's. Eg:

def merge():
    return icon('git-merge')   # Returns either png or svg

So you can just drop a correctly named icons into "icons" dir, and have it JustWork鈩, regardless of image format.
I'm more than willing to work on it, and AFAIK it shouldn't require much changes/bloat code wise, however it might take a while before I get to it.

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