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

Skins: transform qss icon urls into absolute paths #3877

Merged
merged 1 commit into from
May 20, 2021

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented May 18, 2021

work around https://bugs.launchpad.net/mixxx/+bug/1922966

actually we could use the background shorthand in qss instead of the affected image #3863
but that is a lot of work with (see that PR description) and also doesn't work satisfactory for library widgets, since background wouldn't scale images to the actual button size (which varies depending on the library font and the width of the preview column for example).

This pr simply adds absolute file paths where we use skin:/colortheme/icons.

@Holzhaus Holzhaus requested a review from ywwg May 19, 2021 07:50
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I can confirm that this doesn't break anything for me. But I'm also not affected by the bug that this is supposed to fix.

Comment on lines 2033 to 2037
// This replaces relative icon urls in stylesheets (external qss or inline
// <Style> nodes) with absolute file paths.
// TODO Can be removed/disabled as soon as https://bugs.launchpad.net/mixxx/+bug/1922966
// is fixed on all target OS/distros, or rather all target OS have
// the fixed package in their repo.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have an upstream QTBUG that we can mention here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And any release of kiconthemes != 5.80 will work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e., should we try to only apply this fix for cases where the package is installed and the wrong version?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't think this breaks anything for unaffected distros. works fine on my machine.

@Holzhaus Holzhaus added this to the 2.3.0 milestone May 19, 2021
Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works on Ubuntu 21.04. Brilliant fix

@ronso0 ronso0 force-pushed the skin-absolute-qss-icon-urls branch from e0876ac to 4d6b69e Compare May 19, 2021 22:24
@ronso0 ronso0 marked this pull request as ready for review May 19, 2021 22:24
@ronso0
Copy link
Member Author

ronso0 commented May 19, 2021

I mentioned the kde bug in the comment.

the replace function is now separate and used for the launch image as well as the actual skin stylesheet.
@ywwg please take another look.

(i.e., should we try to only apply this fix for cases where the package is installed and the wrong version?)

thought about that, too, but I have nooo idea how to detect that.
any downside with this fix except that it creates a slightly bigger temporary QString?

btw there are other plugins possibly interfering --or possibly helpful-- like qt5ct (apply system styles to menubar) or some dbus plugin (prints Debug [Main]: D-Bus global menu: no)

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checks fail.

@@ -2262,3 +2266,15 @@ void LegacySkinParser::addShortcutToToolTip(WBaseWidget* pWidget,
QString LegacySkinParser::parseLaunchImageStyle(const QDomNode& node) {
return m_pContext->selectString(node, "LaunchImageStyle");
}

QString LegacySkinParser::stylesheetAbsIconPaths(QString& style) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutable references are confusing IMHO, but in this case it's okay I guess.

to work around missing SVG icons when broken libKF5IconThemes5 5.80
is installed
@ronso0 ronso0 force-pushed the skin-absolute-qss-icon-urls branch from 4d6b69e to cbfada1 Compare May 19, 2021 22:36
@ronso0
Copy link
Member Author

ronso0 commented May 19, 2021

Checks fail.

ahhmm, yes...got distracted. one more Ctrl+S and it works.

@ronso0 ronso0 requested a review from ywwg May 19, 2021 22:48
@ronso0
Copy link
Member Author

ronso0 commented May 19, 2021

btw @ywwg the preferences icons were not affected? 'only' skin icons and the checkmarks in the manubar, right?

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still works after update

@ywwg
Copy link
Member

ywwg commented May 20, 2021

preferences look fine

@ronso0
Copy link
Member Author

ronso0 commented May 20, 2021

alright, thanks.

An affected user in the forum managed to build this PR on Ubuntu Studio 21.04 and noticed the issues I watched during my VM testing as well (Ubuntu 21.04): the launch image, logo and some icons are distorted.
https://mixxx.discourse.group/t/icons-not-showing-in-ubuntu-studio/22201/6

meeeh..
Let's merge this, better than no icons.
And I'll see if I can fix it. Maybe something with icon size not matching the button size.

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@Holzhaus Holzhaus merged commit bfa2808 into mixxxdj:2.3 May 20, 2021
@ronso0 ronso0 deleted the skin-absolute-qss-icon-urls branch May 20, 2021 08:49
@ywwg
Copy link
Member

ywwg commented May 23, 2021

thanks so much for this!

@ronso0
Copy link
Member Author

ronso0 commented May 23, 2021

yeah, welcome!
Tango is broken though due to skin:../Tango/ which is required for Tango64.
have fix ready that I'll push tonight.

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

Successfully merging this pull request may close these issues.

3 participants