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

Optimize various SVGs #2130

Merged
merged 34 commits into from
Dec 20, 2021
Merged

Optimize various SVGs #2130

merged 34 commits into from
Dec 20, 2021

Conversation

RiedleroD
Copy link
Contributor

@RiedleroD RiedleroD commented Nov 28, 2021

I improved many of the project's SVGs to the best of my abilites - although I left some out, either due to minimal possible improvement or due to massive amounts of work being required.

Some things I improved

  • file sizes of all SVGs now add up to about 2/3rds of what they were before (0.68%)
    (might be even better after the commits I've pushed after writing this, but I'm too sick to check rn)
  • decreased the complexity of several icons in the GUI, making rendering slightly more efficient
  • fixed some wonky text rendering with unusual fonts
  • fixed some non-compliant SVGs

Some things I messed up

  • I did not read the contribution guidelines until just now, so I guess only a squash merge can rescue this PR
  • commit titles are basically useless as well
  • the keyboard SVG was just too much - I did improve it, but there's still a lot of room for improvement (which I'm not willing to put my time into, I don't even know where the SVG is shown)
  • Some SVGs that are 'fine' are left out. Some minor compliance issues and non-optimal or empty paths remain.

@affirVega
Copy link
Contributor

affirVega commented Nov 30, 2021

Compiled and ran, looks like all icons are displayed correctly.
image
image

@borgmanJeremy
Copy link
Contributor

Thanks! I'll get this checked out a bit more and then merged. Do you think we should regenerate the png's based on these new svg's?

@RiedleroD
Copy link
Contributor Author

I think regenerating the PNGs won't make any difference. You're free to try, but don't say I wasted your time, @borgmanJeremy ;P

@borgmanJeremy
Copy link
Contributor

We are going to have a beta of V11 soon, probably by the end of the weekend. Do you think this will be ready by then?

@mmahmoudian
Copy link
Member

the keyboard SVG was just too much - I did improve it, but there's still a lot of room for improvement (which I'm not willing to put my time into, I don't even know where the SVG is shown)

I think you refer to the picture tgat is shown while setting a keyboard shortcut. In config when changing a keyboard shortcut a windows opens to capture what keys you press and in that windows a keyboard picture is displayed.

@RiedleroD
Copy link
Contributor Author

RiedleroD commented Dec 12, 2021

@borgmanJeremy I think it's already ready… I originally wanted to do the rest too (except for the keyboard one) but I got sick recently & haven't fully recovered. Besides, the rest are mostly fine, just a few standard compliance issues and slightly non-optimal paths. And some empty paths. But nothing major as far as I can see. I can always open another PR later.

@mmahmoudian Yes, that's the one. Thanks! Not sure what purpose it serves though, as it kinda just sits there… Maybe a symbol of a single key would be better there? I can open a PR for that once I feel better.

@RiedleroD RiedleroD marked this pull request as ready for review December 12, 2021 10:39
@mmahmoudian
Copy link
Member

@RiedleroD I hope you recover soon.

Regarding the current keyboard symbol, I'm not sure if changing it would help, but I'm personally open for suggestions

@borgmanJeremy borgmanJeremy merged commit a00df9c into flameshot-org:master Dec 20, 2021
@borgmanJeremy
Copy link
Contributor

Thanks!!

borgmanJeremy pushed a commit that referenced this pull request Dec 21, 2021
* first 3 optimizations

* minified 5 more SVGs

* minimized the delete icon even more

* minified white variants of previously minified SVGs

* removed unneccessary xml header from circlecount-outline SVG

* optimized invert icons

* optimized open_with SVG

* optimized the alignment SVGs

* optimized pin SVGs

* optimized mouse-off SVGs

* optimized flameshot icon SVG

* optimized more instances of the flameshot icon SVG

* optimized SVG 'marker'

* fixed problems with weird default fonts messing with the circlecount-outline SVG image

* whoops forgot to recolor a white icon

* different approach to 3f73efa
qt5 didn't like the transform attribute I added previously, but this should work as well - just not as reliably.
Better than before both commits in any case though

* somewhat decreased size & complexity of the keyboard SVG

* optimized monochrome flameshot icon

* optimized format-text SVG

* optimized SVG icon 'graphics'

* optimized SVG icon 'format_bold'

* optimized SVG icon 'colorize'

* optimized SVG icon 'config'

* optimized icon 'redo-variant'

* optimized SVG icon 'cloud-upload'

* optimized SVG icon 'arrow-bottom-left'

* optimized SVG icon 'circle-outline'

* optimized SVG icon 'content-copy'

* optimized SVG icon 'content-save'

* optimized SVG icon 'cursor-move'

* optimized SVG icon 'exit-to-app'

* optimized SVG icon 'format_italic'

* optimized SVG icon 'format_strikethrough'

* optimized SVG icon 'format_underlined'
@borgmanJeremy
Copy link
Contributor

borgmanJeremy commented Dec 22, 2021

This seems to have broken all the icons on windows so I need to revert it.

image

borgmanJeremy added a commit that referenced this pull request Dec 22, 2021
@RiedleroD
Copy link
Contributor Author

RiedleroD commented Dec 22, 2021

bruh what the hell
edit: I'll look into this later this week, hopefully I can fix it. Gonna need some time though, windows is not my kind of platform.

@mmahmoudian
Copy link
Member

@borgmanJeremy good that you tested it before releasing the v0.11.0 It would have be chaotic in the issues section 😅

@RiedleroD
Copy link
Contributor Author

RiedleroD commented Dec 22, 2021

do you by any chance use an old(er) version of rsvg on windows? I know it has some problems with parsing shorthand numbers in elliptical arcs, so if that could be fixed instead, that'd be cool.

edit: I know for a fact that version 2.40 has this problem bc wikipedia uses it & I always have to work around that there.

edit2: specifically this bug.

@borgmanJeremy
Copy link
Contributor

I'll look into which version I'm using. My windows box is fully up to date and I'm using the latest qt LTS.

borgmanJeremy added a commit that referenced this pull request Dec 22, 2021
@borgmanJeremy
Copy link
Contributor

MacOs has the same issue actually.

@RiedleroD
Copy link
Contributor Author

@borgmanJeremy I'm trying to test it rn but I can't compile for windows - there's no compilation documentation for windows in the readme and using the instructions for debian on WSL Ubuntu doesn't work. Logs are attached.

Can't test for MacOS because I don't have OSX at hand, but let's hope it's the same problem on both systems.

cmake.log.txt
make.log.txt

@borgmanJeremy
Copy link
Contributor

Building on windows is a freaking a pain IMO :)

I usually download the community version of Qt and Qt Creator. Then you can open the project as a CMake project in Qt creator.

@RiedleroD
Copy link
Contributor Author

RiedleroD commented Dec 27, 2021

@borgmanJeremy I can't get it to work... can you please build it on your end and send me the binary?

@borgmanJeremy
Copy link
Contributor

Every pull request builds all artifacts, so if you submit a PR you can download the artifacts

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.

4 participants