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

Use Mumble theme icons for the system theme #3475

Merged
merged 4 commits into from Aug 20, 2018

Conversation

@davidebeatrici
Copy link
Member

commented Jul 18, 2018

Before

before_light
before

After

after_light
after

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

Does system theme mean colors provided by the OS? Then how does it look in white/grey?

@Kissaki
Copy link
Member

left a comment

What is the reasoning for this change? Can the commit describe that?

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

Uuuhm wasn't there a possibility to comment commits before? God damnit... this GitHub review interface is lacking…

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 21, 2018

ea7fdad

What is the reasoning for this change? Can the commit describe that?

@Kissaki
Copy link
Member

left a comment

e3477b8

Shouldn't this be part of the last commit which changes the path?

51dd2be

What's it the reason for this change?

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:themes-mumble-icons branch from 16a9bec to cc9e98b Jul 21, 2018

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2018

Does system theme mean colors provided by the OS? Then how does it look in white/grey?

Yes, I'm going to add screenshots with the white system theme in the pull request message.

What is the reasoning for this change? Can the commit describe that?

It's to maintain consistency and because the current icons we use for the "None" theme are not as good as the Mumble theme's ones.

ea7fdad

What is the reasoning for this change? Can the commit describe that?

e3477b8

Shouldn't this be part of the last commit which changes the path?

51dd2be

What's it the reason for this change?

I reworded the commits.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Jul 22, 2018

I agree the icons look better.

themes: change Mumble theme prefix from "builtin/themes/Mumble" to "themes/Mumble"

Could be shortened to:?

themes: Drop redundant `builtin` path prefix

When setting the skin path, wouldn't that mean it would use that skin rather than only those icons? Does this only work because we don't reload those/all resources? I'm not sure I like that. I'm also not sure how else to implement this; replicating the icons in another would be an option.

We also now depend on the themes submodule. Which is kind of okay, but being able to build Mumble without this dependency seems like an advantage. Especially since the theme(s) is(/are) licensed differently. So considering/concerning this I’m not sure if this would be a good change. Maybe we can just update our icons - if license allows that, re-licensing, or adding a third pary license.

What is the reasoning for moving icons? Those are icons we no longer use? Why not remove them?

Tango icons are removed. I believe we have third party license logic/infos that are not being removed here?

@Ytmndissue

This comment has been minimized.

Copy link

commented Jul 22, 2018

Just a comment, having "none" use the system theme is preferable to some of us and we do like using these newer/cleaner icons with our system theme opposed to the all white/black themes.

This should look similar to how Teamspeak 3 looks, cleaner/flatter icons with still the professional looking Win32 "rendering" of how QT draws GUI elements with "none" selected.

themes: drop redundant `builtin` path prefix
"builtin" is redundant because the ":" prefix is required in order to access embedded resources.
Themes: change skins resources path from ":/" to ":/themes/Mumble"
This commit sets the "skin:" prefix to the Mumble theme's resources path, which makes the system theme ("None") use the icons from our theme.

Previously, the icons embedded in the resources root (:/) were used.
@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2018

When setting the skin path, wouldn't that mean it would use that skin rather than only those icons? Does this only work because we don't reload those/all resources? I'm not sure I like that. I'm also not sure how else to implement this; replicating the icons in another would be an option.

The style is applied here:

bool Themes::apply() {
const bool result = applyConfigured();
if (!result) {
applyFallback();
}
if (g.mw != NULL) {
g.mw->qteLog->document()->setDefaultStyleSheet(qApp->styleSheet());
}
return result;
}

applyFallback() is called with the None theme, that's why our stylesheet is not loaded.

We also now depend on the themes submodule. Which is kind of okay, but being able to build Mumble without this dependency seems like an advantage. Especially since the theme(s) is(/are) licensed differently. So considering/concerning this I’m not sure if this would be a good change. Maybe we can just update our icons - if license allows that, re-licensing, or adding a third party license.

Our theme is licensed under Unlicense: https://github.com/mumble-voip/mumble-theme/blob/master/LICENSE

"A license with no conditions whatsoever which dedicates works to the public domain. Unlicensed works, modifications, and larger works may be distributed under different terms and without source code."

What is the reasoning for moving icons? Those are icons we no longer use? Why not remove them?

Yes, those are icons we no longer use.

I didn't remove them because they are our own, but I can do it if you prefer.

Tango icons are removed. I believe we have third party license logic/infos that are not being removed here?

I didn't find any info aside from:

These icons are copied from the Tango Icon Theme http://tango.freedesktop.org/Tango_Desktop_Project

@davidebeatrici davidebeatrici changed the title Use Mumble theme icons instead of the classic ones for the system theme Use Mumble theme icons for the system theme Jul 22, 2018

@davidebeatrici davidebeatrici force-pushed the davidebeatrici:themes-mumble-icons branch from cc9e98b to 3c54253 Jul 22, 2018

@Ytmndissue

This comment has been minimized.

Copy link

commented Aug 3, 2018

So, this ready to merge?

@davidebeatrici

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2018

@Kissaki What do you think?

@Natenom
Copy link
Contributor

left a comment

works for me

@davidebeatrici davidebeatrici merged commit 7bf387d into mumble-voip:master Aug 20, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.