search

numixproject / numix-core

Builder for App Icon Themes
GNU General Public License v3.0
766 stars 146 forks source link

100% Coverage for Square #3481

Closed Foggalong closed 7 years ago

Foggalong commented 7 years ago

At the moment not every Circle icon has a Square equivalent. From now on all pull requests will be required to include both Circle and Square version (even if that means one of the Numix team adding them) but for the existing icons this will still need addressing.

This list is to be prioritised over #3482

really-need-an-api-key commented 7 years ago

Intriguing. I have square versions of all the icons I've opened PRs for, plus a few more (some jetbrains IDEs, zeal, discord-canary, avahi, processing-pde, and a few more), I'll try to clean them up and open a PR for them this week. They just go into the square dir, right?

Foggalong commented 7 years ago

@really-need-an-api-key That's brilliant :) Yeah, just stick them in there!

really-need-an-api-key commented 7 years ago

Before I start opening PRs - I've noticed that almost all of the square icons have a lot of extra metadata (as does the square template) - like the xmlns attribs on svg and some other rdf tags. Should they be preserved or removed in the optimized file? Previously I've always checked the output of scour by hand and removed everything I thought was unneeded, but I'm not sure how to proceed now.

Foggalong commented 7 years ago

@really-need-an-api-key Yeah, the Square icons are messed up in a big way in terms of the SVG source. Saving them as an optimised SVG in Inkscape (same as we do with circle) is where we're going from now.

andia89 commented 7 years ago

@palob @bil-elmoussaoui @Foggalong

Check this out:

I made a very small script that extracts the basecolour (with gradient) from the circle icons and saved them to separate files (did work for all but ~150 icons).

If we want to make square icons from the circle icons, it would make sense to use these "templates" and just move the object from circle to that template. I don't see an easy way to make that fully automatic though.

https://github.com/wa4557/numix-core/tree/templates/icons/square/48/templates

palob commented 7 years ago

Thanks muchly, that'll speed things up big time.

Any idea what's the special things with those 150 icons left? Full baseplate designs whithout a clear baseplate or baseplate with a solid colour in liue of a gradient?

andia89 commented 7 years ago

yes, mainly multiple gradients in the original circle file, whcih makes it hard to extract the correct one

bilelmoussaoui commented 7 years ago

Awesome work:+1: I've tried to make the whole process automatic, and i think it's possible. But we should have used proper id's for each icon an id for the group of the logo object called say for example logo and an id for the gradient & the baseplate. But we don't have that and it's going to take too much time to modify all those icons manually then just creating them using Inkscape :p

But your templates are going to be super helpful! :D

bilelmoussaoui commented 7 years ago

Talking about icons there's three categories of icons on both themes:

palob commented 7 years ago

That's because different cleaning methods where used, svgcleaner and scour, each with different options enabled respectively.

Rearranging SVG code is not that trivial. While it might stay valid SVG per spec Inkscape editability could be impaired. And then QtSVG and rsvg vary in completeness and bug-freeness of SVG spec implementation.

I'll get our script up to snuff after the next svgcleaner release which will bring new stuff relevant to us.

In Square we still have many one-line files (a PITA) but I think we shouldn't start SVG transforming before #3482.

bilelmoussaoui commented 7 years ago

Totally agree with that :) we should concentrate on finishing this huge list first and the cleaning will happen later

bilelmoussaoui commented 7 years ago

@Foggalong I would like to update the content of this issue with only the missing icons on master branch. As there was a lot of work on this recently, and it's really hard to follow those changes...

Foggalong commented 7 years ago

@bil-elmoussaoui I'm not sure what you mean?

bilelmoussaoui commented 7 years ago

The list of missing icons on Square icon theme is outdated. Ticking one checkbox after another will take a lot of time. Instead we can just reuse your code to generate this list again

Foggalong commented 7 years ago

@bil-elmoussaoui do you know where that code was? I can't seem to find it now

bilelmoussaoui commented 7 years ago

@Foggalong I've taken care of that. I have no idea where the original code is :P But I've rewritten the script so it doesn't count the missing symlinks too (if the original icon is missing, all the symlinks will be reported as missing too.)

The good news is there's still only 835 missing icons (the number was 2300 at the beginning). Really nice work guys!! :tada: 

from os import path, listdir

CIRCLE_DIR = "numix-icon-theme-circle/Numix-Circle/48/apps/"
SQUARE_DIR = "numix-icon-theme-square/Numix-Square/48/apps/"

def list_icons(directory):
    return set(listdir(directory))

def sort(icons_list):
    return sorted(icons_list, key=lambda x: x.lower())

def diff_icons(dir1, dir2):
    icons1 = list_icons(dir1)
    icons2 = list_icons(dir2)
    diff = list(icons1 - icons2)
    diff_icons = []
    for icon in diff:
        if not path.islink(path.join(dir1, icon)):
          diff_icons.append(icon)
    return sort(diff_icons)

def display(icons_list):
    for icon in icons_list:
        print("- [ ] {}".format(icon.replace(".svg", "")))

circle_icons = diff_icons(CIRCLE_DIR, SQUARE_DIR)

print("Not in Square Icon Theme:")
display(circle_icons)
print("Total: {}".format(len(circle_icons)))
W-Floyd commented 7 years ago

See PRs:

3846

3845

3844

3843

3840

3839

3838

3837

3834

3833

3832

3831

3829

3828

They should cover everything :)

Edit: Looks like they've all been merged! :tada: Sorry for flooding the commit history, but I'm glad I was able to help :)

bilelmoussaoui commented 7 years ago

This can now be closed :+1: