From b8459984e9f887117798f0cd761881de747e36a2 Mon Sep 17 00:00:00 2001 From: Thomas Bui Date: Mon, 4 Jan 2021 14:12:15 -0800 Subject: [PATCH 01/16] Added script to check svg fill and viewBox --- .github/scripts/build_assets/filehandler.py | 68 +++++++++++++++------ .github/scripts/check_all_icons.py | 18 ++++++ .github/scripts/icomoon_build.py | 2 +- .github/scripts/icomoon_peek.py | 37 ++++++++++- 4 files changed, 105 insertions(+), 20 deletions(-) create mode 100644 .github/scripts/check_all_icons.py diff --git a/.github/scripts/build_assets/filehandler.py b/.github/scripts/build_assets/filehandler.py index 65a1234cd..f5d24353b 100644 --- a/.github/scripts/build_assets/filehandler.py +++ b/.github/scripts/build_assets/filehandler.py @@ -44,12 +44,15 @@ def is_not_in_icomoon_json(icon, icomoon_json) -> bool: return True -def get_svgs_paths(new_icons: List[dict], icons_folder_path: str) -> List[str]: +def get_svgs_paths(new_icons: List[dict], icons_folder_path: str, + icon_versions_only: bool=False) -> List[str]: """ Get all the suitable svgs file path listed in the devicon.json. :param new_icons, a list containing the info on the new icons. :param icons_folder_path, the path where the function can find the listed folders. + :param icon_versions_only, whether to only get the svgs that can be + made into an icon. :return: a list of svg file paths that can be uploaded to Icomoon. """ file_paths = [] @@ -59,27 +62,56 @@ def get_svgs_paths(new_icons: List[dict], icons_folder_path: str) -> List[str]: if not folder_path.is_dir(): raise ValueError(f"Invalid path. This is not a directory: {folder_path}.") - # TODO: remove the try-except when the devicon.json is upgraded - try: - aliases = icon_info["aliases"] - except KeyError: - aliases = [] # create empty list of aliases if not provided in devicon.json + if icon_versions_only: + get_icon_svgs_paths(folder_path, icon_info, file_paths) + else: + get_all_svgs_paths(folder_path, icon_info, file_paths) + return file_paths + - for font_version in icon_info["versions"]["font"]: - # if it's an alias, we don't want to make it into an icon - if is_alias(font_version, aliases): - print(f"Not exist {icon_info['name']}-{font_version}.svg") - continue - file_name = f"{icon_info['name']}-{font_version}.svg" - path = Path(folder_path, file_name) +def get_icon_svgs_paths(folder_path: Path, icon_info: dict, file_paths: List[str]): + """ + Get only the svg file paths that can be made into an icon from the icon_info. + :param: folder_path, the folder where we can find the icons. + :param: icon_info, an icon object in the devicon.json. + :param: file_paths, an array containing all the file paths found. + """ + try: + aliases = icon_info["aliases"] + except KeyError: + aliases = [] # create empty list of aliases if not provided in devicon.json - if path.exists(): - file_paths.append(str(path)) - else: - raise ValueError(f"This path doesn't exist: {path}") + for font_version in icon_info["versions"]["font"]: + # if it's an alias, we don't want to make it into an icon + if is_alias(font_version, aliases): + print(f"Skipping this font since it's an alias: {icon_info['name']}-{font_version}.svg") + continue - return file_paths + file_name = f"{icon_info['name']}-{font_version}.svg" + path = Path(folder_path, file_name) + + if path.exists(): + file_paths.append(str(path)) + else: + raise ValueError(f"This path doesn't exist: {path}") + + +def get_all_svgs_paths(folder_path: Path, icon_info: dict, file_paths: List[str]): + """ + Get all the svg file paths of an icon. + :param: folder_path, the folder where we can find the icons. + :param: icon_info, an icon object in the devicon.json. + :param: file_paths, an array containing all the file paths found. + """ + for font_version in icon_info["versions"]["svg"]: + file_name = f"{icon_info['name']}-{font_version}.svg" + path = Path(folder_path, file_name) + + if path.exists(): + file_paths.append(str(path)) + else: + raise ValueError(f"This path doesn't exist: {path}") def is_alias(font_version: str, aliases: List[dict]): diff --git a/.github/scripts/check_all_icons.py b/.github/scripts/check_all_icons.py new file mode 100644 index 000000000..914e7c49e --- /dev/null +++ b/.github/scripts/check_all_icons.py @@ -0,0 +1,18 @@ +from pathlib import Path +import json + + +# pycharm complains that build_assets is an unresolved ref +# don't worry about it, the script still runs +from build_assets import filehandler + + +if __name__ == "__main__": + """ + Use as a cmd line script to check all the icons of the devicon.json. + """ + devicon_json_path = str(Path("./devicon.json").resolve()) + icons_folder_path = str(Path("./icons").resolve()) + with open(devicon_json_path) as json_file: + devicon_json = json.load(json_file) + svgs = filehandler.get_svgs_paths(devicon_json, icons_folder_path) \ No newline at end of file diff --git a/.github/scripts/icomoon_build.py b/.github/scripts/icomoon_build.py index 303a7ffc6..f11255219 100644 --- a/.github/scripts/icomoon_build.py +++ b/.github/scripts/icomoon_build.py @@ -22,7 +22,7 @@ def main(): runner = SeleniumRunner(args.download_path, args.geckodriver_path, args.headless) runner.upload_icomoon(args.icomoon_json_path) - svgs = filehandler.get_svgs_paths(new_icons, args.icons_folder_path) + svgs = filehandler.get_svgs_paths(new_icons, args.icons_folder_path, True) runner.upload_svgs(svgs) zip_name = "devicon-v1.0.zip" diff --git a/.github/scripts/icomoon_peek.py b/.github/scripts/icomoon_peek.py index cccfce5d7..0c855436a 100644 --- a/.github/scripts/icomoon_peek.py +++ b/.github/scripts/icomoon_peek.py @@ -2,6 +2,7 @@ import re import sys from selenium.common.exceptions import TimeoutException +import xml.etree.ElementTree as et # pycharm complains that build_assets is an unresolved ref # don't worry about it, the script still runs @@ -32,8 +33,13 @@ def main(): runner = None try: - runner = SeleniumRunner(args.download_path, args.geckodriver_path, args.headless) + # check the svgs svgs = filehandler.get_svgs_paths(filtered_icons, args.icons_folder_path) + check_svgs(svgs) + + # check icon + runner = SeleniumRunner(args.download_path, args.geckodriver_path, args.headless) + svgs = filehandler.get_svgs_paths(filtered_icons, args.icons_folder_path, True) screenshot_folder = filehandler.create_screenshot_folder("./") runner.upload_svgs(svgs, screenshot_folder) print("Task completed.") @@ -62,5 +68,34 @@ def find_object_added_in_this_pr(icons: List[dict], pr_title: str): return [] +def check_svgs(svg_file_paths: List[str]): + """ + Check the viewBox and style of each svgs passed in. + The viewBox must be '0 0 128 128'. + The style must not contain any 'fill' declarations. + If any error is found, they will be thrown. + :param: svg_file_paths, the file paths to the svg to check for. + """ + # batch err messages together so user can fix everything at once + err_msgs = [] + for svg_path in svg_file_paths: + tree = et.parse(svg_path) + root = tree.getroot() + namespace = "{http://www.w3.org/2000/svg}" + + if root.tag != f"{namespace}svg": + err_msgs.append(f"Root is '{root.tag}'. Root must be an 'svg' element: {svg_path}") + + if root.attrib["viewBox"] != "0 0 128 128": + err_msgs.append("SVG 'viewBox' is not '0 0 128 128': " + svg_path) + + style = root.findtext(f".//{namespace}style") + if style != None and "fill" in style: + err_msgs.append("Found 'fill' in Artboard 13 \ No newline at end of file +Artboard 13 \ No newline at end of file diff --git a/icons/jenkins/jenkins-plain.svg b/icons/jenkins/jenkins-plain.svg index c7ccce6f4..dc36bbece 100644 --- a/icons/jenkins/jenkins-plain.svg +++ b/icons/jenkins/jenkins-plain.svg @@ -1,22 +1,21 @@ - - - - - - - - - - - - - - - - - - - - - - \ No newline at end of file + + + + + + + + + + + + + + + + + + + + + diff --git a/icons/twitter/twitter-original.svg b/icons/twitter/twitter-original.svg index 8d95b1aab..84a8578e3 100644 --- a/icons/twitter/twitter-original.svg +++ b/icons/twitter/twitter-original.svg @@ -1 +1,6 @@ - \ No newline at end of file + + + + + + diff --git a/icons/yunohost/yunohost-plain.svg b/icons/yunohost/yunohost-plain.svg index 5f7a27596..e2d56ffa0 100644 --- a/icons/yunohost/yunohost-plain.svg +++ b/icons/yunohost/yunohost-plain.svg @@ -1,322 +1,16 @@ - - - - - - - - - - - - - - - - + + + + + + + + + + + + + + From 1a0ec51ba7e3839843c00a8e406448fff38f036d Mon Sep 17 00:00:00 2001 From: Thomas Bui Date: Mon, 4 Jan 2021 15:13:28 -0800 Subject: [PATCH 04/16] Added check for height and width attr --- .github/scripts/icomoon_peek.py | 12 ++++++++++-- CONTRIBUTING.md | 5 +++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/.github/scripts/icomoon_peek.py b/.github/scripts/icomoon_peek.py index 0c855436a..430204192 100644 --- a/.github/scripts/icomoon_peek.py +++ b/.github/scripts/icomoon_peek.py @@ -70,8 +70,9 @@ def find_object_added_in_this_pr(icons: List[dict], pr_title: str): def check_svgs(svg_file_paths: List[str]): """ - Check the viewBox and style of each svgs passed in. + Check the width, height, viewBox and style of each svgs passed in. The viewBox must be '0 0 128 128'. + If the svg has a width and height attr, ensure it's '128px'. The style must not contain any 'fill' declarations. If any error is found, they will be thrown. :param: svg_file_paths, the file paths to the svg to check for. @@ -86,9 +87,16 @@ def check_svgs(svg_file_paths: List[str]): if root.tag != f"{namespace}svg": err_msgs.append(f"Root is '{root.tag}'. Root must be an 'svg' element: {svg_path}") - if root.attrib["viewBox"] != "0 0 128 128": + if root.get("viewBox") != "0 0 128 128": err_msgs.append("SVG 'viewBox' is not '0 0 128 128': " + svg_path) + acceptable_size = [None, "128px", "128"] + if root.get("height") not in acceptable_size: + err_msgs.append("SVG 'height' is present but is not '128' or '128px': " + svg_path) + + if root.get("width") not in acceptable_size: + err_msgs.append("SVG 'width' is present but is not '128' or '128px': " + svg_path) + style = root.findtext(f".//{namespace}style") if style != None and "fill" in style: err_msgs.append("Found 'fill' in

Before you submit your logos/svgs, please ensure that they meet the following standard:

From 5b43dad57ed04a325f07b28ffea68cd18c6fa716 Mon Sep 17 00:00:00 2001 From: Thomas Bui Date: Tue, 5 Jan 2021 21:16:31 -0800 Subject: [PATCH 05/16] Added check_svgs workflow --- .github/scripts/build_assets/arg_getters.py | 23 ++++++ .../drafts/check_devicon_object.py | 37 +++++++++ .github/scripts/build_assets/filehandler.py | 5 +- .github/scripts/check_svgs.py | 77 +++++++++++++++++++ .github/scripts/icomoon_peek.py | 42 +--------- .github/workflows/check_svgs.yml | 19 +++++ 6 files changed, 158 insertions(+), 45 deletions(-) create mode 100644 .github/scripts/build_assets/drafts/check_devicon_object.py create mode 100644 .github/scripts/check_svgs.py create mode 100644 .github/workflows/check_svgs.yml diff --git a/.github/scripts/build_assets/arg_getters.py b/.github/scripts/build_assets/arg_getters.py index fe2893a23..4e1d59c06 100644 --- a/.github/scripts/build_assets/arg_getters.py +++ b/.github/scripts/build_assets/arg_getters.py @@ -3,6 +3,10 @@ def get_selenium_runner_args(peek_mode=False): + """ + Get the commandline arguments for the icomoon_peek.py and + icomoon_build.py. + """ parser = ArgumentParser(description="Upload svgs to Icomoon to create icon files.") parser.add_argument("--headless", @@ -33,4 +37,23 @@ def get_selenium_runner_args(peek_mode=False): parser.add_argument("--pr_title", help="The title of the PR that we are peeking at") + return parser.parse_args() + + +def get_check_svgs_args(): + """ + Get the commandline arguments for the chec_svgs.py. + """ + parser = ArgumentParser(description="Check the SVGs to ensure their attributes are correct.") + parser.add_argument("icomoon_json_path", + help="The path to the icomoon.json aka the selection.json created by Icomoon", + action=PathResolverAction) + + parser.add_argument("devicon_json_path", + help="The path to the devicon.json", + action=PathResolverAction) + + parser.add_argument("icons_folder_path", + help="The path to the icons folder", + action=PathResolverAction) return parser.parse_args() \ No newline at end of file diff --git a/.github/scripts/build_assets/drafts/check_devicon_object.py b/.github/scripts/build_assets/drafts/check_devicon_object.py new file mode 100644 index 000000000..b610d88ed --- /dev/null +++ b/.github/scripts/build_assets/drafts/check_devicon_object.py @@ -0,0 +1,37 @@ +from typing import List + +# abandoned since it's not too hard to check devicon objects using our eyes +# however, still keep in case we need it in the future + +def check_devicon_objects(icons: List[dict]): + """ + Check that the devicon objects added is up to standard. + """ + err_msgs = [] + for icon in icons: + if type(icon["name"]) != str: + err_msgs.append("'name' must be a string, not: " + str(icon["name"])) + + try: + for tag in icon["tags"]: + if type(tag) != str: + raise TypeError() + except TypeError: + err_msgs.append("'tags' must be an array of strings, not: " + str(icon["tags"])) + break + + + if type(icon["versions"]["svg"]) != list or len(icon["versions"]["svg"]) == 0: + err_msgs.append("Icon name must be a string") + + if type(icon["versions"]["font"]) != list or len(icon["versions"]["svg"]) == 0: + err_msgs.append("Icon name must be a string") + + if type(icon["color"]) != str or "#" not in icon["color"]: + err_msgs.append("'color' must be a string in the format '#abcdef'") + + if type(icon["aliases"]) != list: + err_msgs.append("'aliases' must be an array of dicts") + + if len(err_msgs) > 0: + raise Exception("Error found in devicon.json: \n" + "\n".join(err_msgs)) diff --git a/.github/scripts/build_assets/filehandler.py b/.github/scripts/build_assets/filehandler.py index f5d24353b..502ea00f4 100644 --- a/.github/scripts/build_assets/filehandler.py +++ b/.github/scripts/build_assets/filehandler.py @@ -77,10 +77,7 @@ def get_icon_svgs_paths(folder_path: Path, icon_info: dict, file_paths: List[str :param: icon_info, an icon object in the devicon.json. :param: file_paths, an array containing all the file paths found. """ - try: - aliases = icon_info["aliases"] - except KeyError: - aliases = [] # create empty list of aliases if not provided in devicon.json + aliases = icon_info["aliases"] for font_version in icon_info["versions"]["font"]: # if it's an alias, we don't want to make it into an icon diff --git a/.github/scripts/check_svgs.py b/.github/scripts/check_svgs.py new file mode 100644 index 000000000..21480969e --- /dev/null +++ b/.github/scripts/check_svgs.py @@ -0,0 +1,77 @@ +from typing import List +import sys +import xml.etree.ElementTree as et + + +# pycharm complains that build_assets is an unresolved ref +# don't worry about it, the script still runs +from build_assets import filehandler, arg_getters + + +def main(): + args = arg_getters.get_check_svgs_args() + new_icons = filehandler.find_new_icons(args.devicon_json_path, args.icomoon_json_path) + + if len(new_icons) == 0: + sys.exit("No files need to be uploaded. Ending script...") + + # print list of new icons + print("SVGs being checked:", *new_icons, sep = "\n", end='\n\n') + + try: + # check the svgs + svgs = filehandler.get_svgs_paths(new_icons, args.icons_folder_path) + check_svgs(svgs) + print("All SVGs found were good.\nTask completed.") + except Exception as e: + sys.exit(e) + + +def check_svgs(svg_file_paths: List[str]): + """ + Check the width, height, viewBox and style of each svgs passed in. + The viewBox must be '0 0 128 128'. + If the svg has a width and height attr, ensure it's '128px'. + The style must not contain any 'fill' declarations. + If any error is found, they will be thrown. + :param: svg_file_paths, the file paths to the svg to check for. + """ + # batch err messages together so user can fix everything at once + err_msgs = [] + for svg_path in svg_file_paths: + tree = et.parse(svg_path) + root = tree.getroot() + namespace = "{http://www.w3.org/2000/svg}" + + if root.tag != f"{namespace}svg": + err_msgs.append(f"Root is '{root.tag}'. Root must be an 'svg' element: {svg_path}") + + if root.get("viewBox") != "0 0 128 128": + err_msgs.append(" 'viewBox' is not '0 0 128 128': " + svg_path) + + acceptable_size = [None, "128px", "128"] + if root.get("height") not in acceptable_size: + err_msgs.append(" 'height' is present but is not '128' or '128px': " + svg_path) + + if root.get("width") not in acceptable_size: + err_msgs.append(" 'width' is present but is not '128' or '128px': " + svg_path) + + if root.get("style") is not None and "enable-background" in root.get("style"): + err_msgs.append(" uses 'enable-background' in its style. This is deprecated: " + svg_path) + + if root.get("x") is not None: + err_msgs.append(" has an 'x' attribute, this is not needed: " + svg_path) + + if root.get("y") is not None: + err_msgs.append(" has an 'y' attribute, this is not needed: " + svg_path) + + style = root.findtext(f".//{namespace}style") + if style != None and "fill" in style: + err_msgs.append("Found 'fill' in