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

Invert Framing for Torrent Status Icons #7583

Merged
merged 3 commits into from Oct 18, 2017
Merged

Invert Framing for Torrent Status Icons #7583

merged 3 commits into from Oct 18, 2017

Conversation

LordNyriox
Copy link
Contributor

@LordNyriox LordNyriox commented Oct 13, 2017

Also recolor the icons to match the text-color used for the torrent-status, as well.

Icons optimized using SVGOMG [https://jakearchibald.github.io/svgomg/].

Ping @evsh, @Chocobo1, @sledgehammer999.

Also recolor the icons to match the text-color used for the torrent-status as well. 
Optimized using SVGOMG [<https://jakearchibald.github.io/svgomg/>].
@LordNyriox LordNyriox mentioned this pull request Oct 13, 2017
12 tasks
@silverqx
Copy link
Contributor

silverqx commented Oct 13, 2017

I tried grunt build and the library is old or doesn't understand the svg format of your files, it generates only some text anyway @evsh wrote pngs are not needed :)

@sledgehammer999
Copy link
Member

@LordNyriox are you willing to install node.js? If yes, then install svgo and svgexport.
Run svgo with:

 svgo --pretty --indent=2 --multipass -i input.svg

svgexport

svgexport input.svg output.png width:height

width:height expressed in pixels

@sledgehammer999
Copy link
Member

@LordNyriox I pushed 2 commits to your branch(apparently github allows this for PRs).
One uses svgo on the SVGs. The second one generates the pngs.

About the recoloring, I have mixed feelings.
Also, is there any reason why preserved the padding around the images?

@zeule
Copy link
Contributor

zeule commented Oct 14, 2017

@sledgehammer999: could you disable short names in svgo, as well as omitting black colors, please? That would simplify code for colours conversion for colour themes PR.

@sledgehammer999
Copy link
Member

could you disable short names in svgo, as well as omitting black colors, please?

Do you know which switches control that?

@zeule
Copy link
Contributor

zeule commented Oct 14, 2017

No, sorry. I have never used the converter. Just found those possibilities in https://github.com/svg/svgo/blob/master/plugins/convertColors.js

@sledgehammer999
Copy link
Member

Can you tell how are you going to "color convert" from code (Qt) so I can understand what is going on?
I mean, code-wise or API-wise is there an example?

@sledgehammer999
Copy link
Member

programmically overrides the fill= values of each SVG file

Can you link to the specific commit/file/line that does this?

@sledgehammer999
Copy link
Member

I just pushed 2 temporary commits. They have the padding removed. In my opinion, it looks better in the GUI.
Should we keep them or remove them?

@zeule
Copy link
Contributor

zeule commented Oct 14, 2017

Sorry for the late reply. I need to replace colour in SVG file and save it back to disk, because QIcon, initialised with a SVG file, does lazy loading. I've tried to use QSvgRenderer + QSvgGenerator, supplying a custom engine, but this pair incorrectly handles transform matrices, so I ended up with simple regexps: fcf7231#diff-ef0fd4c980145c689f4b98c55277888dR78
Additionally, this handles CSS-defined colours as well.

@sledgehammer999
Copy link
Member

Sorry for the late reply. I need to replace colour in SVG file and save it back to disk, because QIcon, initialised with a SVG file, does lazy loading. I've tried to use QSvgRenderer + QSvgGenerator, supplying a custom engine, but this pair incorrectly handles transform matrices, so I ended up with simple regexps: fcf7231#diff-ef0fd4c980145c689f4b98c55277888dR78
Additionally, this handles CSS-defined colours as well.

Ok I understand. Then I'll manually place back the fill= attribute for the svgs that have black color. Is that all you wanted?
PS: I assume you tried a custom QIconEngine approach like the one mentioned here (SO comment): https://stackoverflow.com/a/44757951

If noone objects I'll use the svgs with removed padding. (in a couple days)

@sledgehammer999
Copy link
Member

I am pretty sure that @evsh also requested that the fill= values that use named shortcuts (like fill="salmon" in paused.svg) be reverted back to hex values instead.

I think it doesn't matter what comes after "fill=". The regexp seem to capture the value anyway.

@zeule
Copy link
Contributor

zeule commented Oct 16, 2017

Then I'll manually place back the fill= attribute for the svgs

Thank you.

PS: I assume you tried a custom QIconEngine approach…

No, I didn't. I want to keep icons in svg format.

I think it doesn't matter what comes after "fill=". The regexp seem to capture the value anyway.

And even if it does not, it is easy to change the regexp.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Oct 16, 2017

No, I didn't. I want to keep icons in svg format.

Apparently you didn't read the SO comment. It implements a custom QIconEngine, which internally just uses QSvgRenderer to output the pixmap or do the paint. It is a hack to support loading an svg from a buffer.
This is offtopic, so I am not continuing. I just wanted to inform you of another solution.

Could you also re-remove the width= and height= values from each SVG?

Are you sure that it is harmless to remove them? svgo didn't remove them automatically.

@LordNyriox
Copy link
Contributor Author

LordNyriox commented Oct 16, 2017

@sledgehammer999: All the SVGs affected by this PR now have their width/height values removed (for consistency with the rest of the project, and also because the SVGs did not have those values previously).

In addition, I manually added a fill="#000000" to the path fields of both stalledDL.svg and stalledUP.svg, as part of their respective width/height commits.

@sledgehammer999
Copy link
Member

@LordNyriox I fixup yours and my commits into one. I hope you don't mind. (I kept the 1st ones from you).

If noone objects, I am going to merge this tomorrow.

@sledgehammer999
Copy link
Member

Thx for this.

@sledgehammer999 sledgehammer999 merged commit eec6009 into qbittorrent:master Oct 18, 2017
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.

None yet

5 participants