Fix breeze dialog background with Qt 5.12.2
ClosedPublic

Authored by fvogt on Mar 17 2019, 11:24 AM.

Details

Summary

style elements without type="text/css" were ignored before, but now they act as
if type was set. Set id and style properly to restore the working behaviour.

BUG: 405548

Test Plan

Installed new files on a system with Qt 5.12.2, wiped the SVG cache
and it looks fine again.

Diff Detail

Repository
R242 Plasma Framework (Library)
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
fvogt created this revision.Mar 17 2019, 11:24 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 17 2019, 11:24 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
fvogt requested review of this revision.Mar 17 2019, 11:24 AM
fvogt edited the summary of this revision. (Show Details)Mar 17 2019, 11:24 AM
Zren added a subscriber: Zren.Mar 17 2019, 3:49 PM

Another option I believe is to keep the <style> tag but add the id="current-color-scheme" type="text/css" attributes. I assume the only difference is plasma replaces the contents of the existing style tag instead of creating a new one, though I'm not sure where the code inserts it.

rooty added a subscriber: rooty.EditedMar 17 2019, 5:00 PM
In D19821#432780, @Zren wrote:

Another option I believe is to keep the <style> tag but add the id="current-color-scheme" type="text/css" attributes. I assume the only difference is plasma replaces the contents of the existing style tag instead of creating a new one, though I'm not sure where the code inserts it.

Yes that works for me (adding both attributes)

This was after modifying both tooltip.svgz files

fvogt updated this revision to Diff 54143.Mar 17 2019, 8:20 PM

Keep the style element, but assign id and type. Fix tooltip.svgz as well.

fvogt edited the summary of this revision. (Show Details)Mar 17 2019, 8:21 PM
falqueto added a subscriber: falqueto.EditedMar 17 2019, 11:58 PM

I believe that src/desktoptheme/breeze/translucent/widgets/tooltip.svgz must be corrected too. It has an <style> node without a type attribute and with a wrong id value.

edit: spelling

fvogt updated this revision to Diff 54176.Mar 18 2019, 8:03 AM

One more file.

fvogt added a comment.Mar 18 2019, 8:04 AM

There's a special case in ./src/desktoptheme/breeze/icons/audio.svgz and .../battery.svgz: It has color scheme definitions both in a <style id="current-color-scheme" type="text/css"> and a <style type="text/css" id="style5"> element.
It shouldn't have an impact on this bug in particular, but it does still look wrong.

filipf added a subscriber: filipf.Mar 18 2019, 9:57 PM
fvogt added a comment.Mar 19 2019, 8:51 AM

Is anyone able to review this? If there's no review in 24h I'll merge it as it affects users severely and the fix is trivial.

rooty accepted this revision.Mar 19 2019, 9:04 AM

If it works for you ship it, I'm nowhere near a computer right now

This revision is now accepted and ready to land.Mar 19 2019, 9:04 AM

Does this patch cover tooltips as shown in https://i.imgur.com/WrISfNg.png ?

Does this patch cover tooltips as shown in https://i.imgur.com/WrISfNg.png ?

Yes.

I guess it is very likely, but all the same for the record I will request that this results in a 5.56.2 release of plasma-framework.

fvogt added a comment.Mar 19 2019, 1:52 PM

I guess it is very likely, but all the same for the record I will request that this results in a 5.56.2 release of plasma-framework.

I agree - as this touches binary files, applying a patch wouldn't work (unless using git apply).

I just gave this diff a try again and could not find any place with broken SVG colors anymore.

This revision was automatically updated to reflect the committed changes.
rooty added a comment.Mar 19 2019, 1:55 PM

This wasn't supposed to land in Plasma 5.15 as well as master?

fvogt added a comment.Mar 19 2019, 1:56 PM

This wasn't supposed to land in Plasma 5.15 as well as master?

Plasma Framework is (in this case unfortunately) a Framework, so no stable branches.

rooty added a comment.Mar 19 2019, 1:57 PM

This wasn't supposed to land in Plasma 5.15 as well as master?

Plasma Framework is (in this case unfortunately) a Framework, so no stable branches.

My bad!