search

home-assistant / Iconic

:art: Auto-generated icon font library for iOS, watchOS and tvOS
Apache License 2.0
1.58k stars 86 forks source link

multi-font + swifty #26

Closed connectdotz closed 7 years ago

connectdotz commented 8 years ago

addressing #23, #24, #2 and maybe #13 (although haven’t thoroughly tested yet).

outline changes:

  1. Updated Iconizer.sh and the stencil files in order to generate multiple fonts without name collision and support the refactoring below.
  2. refactor the Iconic.swift and stencils to generate more swifty code that better align with swift3 API guideline: guideline: 2.1. move most of the logic from Iconic.swift to the new protocol "IconFont" 2.2. Iconic class is now a placeholder to house the generated wrapper functions for objective-c compatibility (since they can't access enum extension and protocol). 2.3 the UI extensions become just thin wrappers around the powerful swift enum mainly for objective-c access. (note: Samples/Pods/Iconic/Source/FontAwesomeIcon.swift is generated from the new stencil/Iconizer.sh)
  3. chnage catalog location and hierarchy in code generation to accomendate multi-font.
  4. Changed Sample swift controllers to use the new style API.
  5. Adjust unit tests accordingly; I did disable the UI extension tests since they are now nothing more than generated wrappers on the enum class that have already been tested in other 2 unit tests. The UI extensions are also exercised in both swift and objective-c controllers in the Sample project and they worked fine.
dzenbot commented 8 years ago

Thanks for this. I will review all this during the weekend. I would have love if the multi-font part wasn't coupled with the swifty refactor. It's a lot of changes, almost a full rewrite.

connectdotz commented 8 years ago

yeah I know, the scope is bigger than I was originally hoped for... making the Iconizer.sh to parse the directory was the easy one, but then I realized the current stencil and src can't support multi-font due to namespace conflict... so the API will have to take on non-backward compatible changes anyway, I figure might as well just change the stencil to generate more swifty code so we don't have to introduce major API changes twice ....

Actually if you disregard the whitespace diff, the changes are more structural, many of the original implementation just got moved to different place but largely intact.

dzenbot commented 8 years ago

I've been reviewing these changes lately (haven't yet pulled the branch) and although I think this is amazing work, I might just take parts of it and build a separate PR. I'm not that worried about non-backward compatible changes, that's why it's still in beta, but I'm specially worried about having 2 separate set of APIs, one for Objc and another one for Swift: A framework like this may be hard to introduce for many teams, since it represents a non-traditional workflow for using image assets. If it also included an interface for each language, it would make it even harder to understand/debug/integrate for many projects that still support both languages.

I do see the advantage and importance of a more Swifty API. I love it! But I also think we need to have 1 standard API. What do you think @connectdotz ?

connectdotz commented 8 years ago

@dzenbot,

but I'm specially worried about having 2 separate set of APIs, one for Objc and another one for Swift:

I assume you refer to the wrapper functions under Iconic/UIBarButtonItem/UITabBarItem/UIButton?

Slightly different API across objective-c and swift has always been in Apple's framework since day 1 when swift was introduced. Simply because some powerful language feature in swift is not available in objective-c. To choose between different API or compromise swift, Apple choose prior. That was the same reason I think it is lesser evil to "generate wrapper API to bring the powerful enum to objective-c programs" than not to fully explore swift strength.

Judging more and more objective-c framework are migrating to swift, not to mention Apple's major swifty effort in ios10, it is quite clear that Swift is becoming the preferred language in apple ecosystem, its adoption will only grow... IMHO, we should almost always prioritize swift over objective-c should there be any conflict.

On the other note, I am perfectly ok if you want to remove those APIs and forgo objective-c support ;-) I adopted them mainly because they are in the original implementation. All of these API really are just thin wrappers or convenient methods, they are really unnecessary if not for objective-c access.

A framework like this may be hard to introduce for many teams, since it represents a non-traditional workflow for using image assets.

I am not sure I get this point, will you mind elaborate?

MartinP7r commented 6 years ago

I'm sorry for posting here, but since it's not clear to me from the documentation , how would I go about installing Iconic with multiple icon fonts as per change 3: chnage catalog location and hierarchy in code generation to accomendate multi-font. ?