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

xdgiconloader: Puts the hicolor at the end of the theme hierarchy #125

Merged
merged 2 commits into from
May 4, 2017

Conversation

luis-pereira
Copy link
Member

The hicolor theme is the ultimate resort. It should be allowed only at the
end of the fallback hierarchy. We are allowing the hicolor theme to be in
the middle of the theme hierarchy. Schematically:
X -> hicolor -> Y -> Z -> hicolor
If an icon exists in the hicolor and Y theme the hicolor one, is wrongly
used.

This commit does four things:
* Stops adding the hicolor theme to each theme as a fallback.
* Stops adding the hicolor as a parent theme, even if added by the
index.theme Inherits key. The oxygen theme does it.
* Stops return the hicolor theme in the fallbackTheme().
* Adds the hicolor theme to the end of the fallback hierarchy list.

@luis-pereira luis-pereira changed the title xdgiconloader: Puts the hicolor at the end of the theme hierarchy [WIP] xdgiconloader: Puts the hicolor at the end of the theme hierarchy Apr 7, 2017
@palinek
Copy link
Contributor

palinek commented Apr 11, 2017

GTM

@agaida
Copy link
Member

agaida commented Apr 11, 2017

If i understand this right #116 and #125 should go in with #116 first? With #125 instead of #126?

@luis-pereira luis-pereira force-pushed the fix-hicolor-position-master branch 2 times, most recently from a3bc6fe to d6518c0 Compare April 12, 2017 19:26
@luis-pereira
Copy link
Member Author

@agaida It goes like this:
During my "investigations" of #116 performance I found several bugs in the already existing code. They had nothing to do with #116.
The hicolor also affects Qt's QIconLoader.

To be able to compare the performance of #116 I "ported" it to master (a simple rebase produces merge conflicts).

I already measured the performance. Will post it ASAP.

@agaida
Copy link
Member

agaida commented Apr 22, 2017

@luis-pereira any news?

@luis-pereira
Copy link
Member Author

@agaida It's ready.

@luis-pereira luis-pereira changed the title [WIP] xdgiconloader: Puts the hicolor at the end of the theme hierarchy xdgiconloader: Puts the hicolor at the end of the theme hierarchy Apr 26, 2017
@agaida
Copy link
Member

agaida commented Apr 26, 2017

cool, thanks

@agaida
Copy link
Member

agaida commented Apr 29, 2017

@luis-pereira - would you be so kind and rebase?

@luis-pereira
Copy link
Member Author

@agaida Rebased.

@agaida
Copy link
Member

agaida commented May 1, 2017

@luis-pereira sometimes i hate git - could you rebase another time?

The hicolor theme is the ultimate resort. It should be allowed only at the
end of the fallback hierarchy. We are allowing the hicolor theme to be in
the middle of the theme hierarchy. Schematically:
        X -> hicolor -> Y -> Z -> hicolor
If an icon exists in the hicolor and Y theme the hicolor one, is wrongly
used.

This commit does four things:
    * Stops adding the hicolor theme to each theme as a fallback.
    * Stops adding the hicolor as a parent theme, even if added by the
index.theme Inherits key. The oxygen theme does it.
    * Stops return the hicolor theme in the fallbackTheme().
    * Adds the hicolor theme to the end of the fallback hierarchy list.
@luis-pereira
Copy link
Member Author

@agaida git has no fault. The code changed in a way that can't be automatically rebased.

@palinek @tsujan
Does it make sense to use theme.followsColorScheme()) while searching for in /usr/share/pixmaps or for unthemed icons at /usr/share/icons ?

To allow the hicolor theme only at the end of the search hierarchy I move code from XdgIconLoader::findIconHelper() to XdgIconLoader::unthemedFallback() and XdgIconLoader::pixmapFallback().
When you guys did the followsColorScheme stuff you also used the theme.followsColorScheme()) on unthemed icons. Was this intended ?

@palinek
Copy link
Contributor

palinek commented May 2, 2017

Does it make sense to use theme.followsColorScheme()) while searching for in /usr/share/pixmaps or for unthemed icons at /usr/share/icons ?

You're right.... it is used wrongly there. ... and all the copy-paste snippets if (currentDir.exists(iconName + pngext)) { should be unified and put into one function (and this should be called on those 3 places).

@agaida
Copy link
Member

agaida commented May 2, 2017

@luis-pereira - i know :P

Search for unthemed icons only after the themed ones
    Unthemed icons should be search only one time, after the hicolor theme.
    We are using QIcon::themeSearchPaths() to provide the list of directories
    and it doesn't depend on the theme.

Puts pixmaps at the end of the search hierarchy
    Their place is after the hicolor theme.
    And they should be searched one time only, after the hicolor theme.

Makes the pixmap fallback available to all operating systems.
Remove the followColorScheme stuff from unthemed/pixamp search.
@luis-pereira
Copy link
Member Author

@agaida @palinek Done.
This is more than a rebase tough.

@agaida
Copy link
Member

agaida commented May 3, 2017

Looks great, compile, work as expected - GTM

@agaida agaida merged commit 6be5e43 into master May 4, 2017
@agaida agaida deleted the fix-hicolor-position-master branch July 26, 2017 17:31
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.

3 participants