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

Swift 4.2 and quality of life improvements #82

Closed robbiet480 closed 5 years ago

robbiet480 commented 5 years ago

This adds support for Swift 4.2. It also allows for setting a custom name for the font via the Iconizer.sh script.

dzenbot commented 5 years ago

Wow this is pretty awesome! Thanks for doing this @robbiet480. I will review this PR really soon 🙏

jkyin commented 5 years ago

@robbiet480 I pushed one PR fix for swift 4.2 compile error. Could you check it out.

robbiet480 commented 5 years ago

@dzenbot Just checking in on the review of this!

For those using this work, just a heads up that I have noticed significant slowdowns in my archive times in Xcode 10.2 and Swift 5, mostly due to Iconic. According to this comment I found when researching the problem, it's due to our very large [String: Any] dictionaries. As the other commenters suggested, fixing the problem might require moving to something like an external JSON file. This one hurts twice as bad for me because I need Iconic on iOS and watchOS. Looks like this issue has already been reported to Swift.

Here's what my build looks like, notice "Compile Swift Files" is taking the longest amount of time, almost 30 minutes:

image

image

robbiet480 commented 5 years ago

@dzenbot Also, not sure if the delay in review is perhaps due to you no longer using this library. If so, I am more than happy to takeover long term development of it and transfer it to my GitHub. Let me know!

dzenbot commented 5 years ago

Hey @robbiet480! Yes, I'm sorry, I no longer use this library for any projects I'm working on, so it's been in my lower-priority-bucket unfortunately.

Do you want to be a contributor? You'd have privileges to push changes and everything, but the repository would still be under my account. I would love to keep it alive, specially since it seems quite a few people are using it.

robbiet480 commented 5 years ago

@dzenbot Sure, but can you make me full admin in case I need to clean up spam and the like, as well as be able to manage versions and labels.

robbiet480 commented 5 years ago

@dzenbot Hi, I still haven't heard back from you in a while and this PR has still gone unreviewed and unmerged after sitting here for almost 9 (!) months now. I need to know what your short and long term plans are for Iconic. There's a lot of people that are continuing to depend on it, both visible in this issue and elsewhere, and it hasn't seen love for a long time now. I worry at this point about even becoming a admin let alone contributor because in case I needed you for something I don't think I could contact you in a timely manner.

Here's what I'm proposing for now:

  1. Transfer this repository over to my GitHub. I promise it will receive a long term home with updates as needed as I am using this in my one and only app, Home Assistant Companion, which has a large audience (150k MAU) keeping me motivated (an audience which also has been annoying me constantly about icons not being updated for a long time now since I haven't been able to get Swift 2.3 running...)
  2. I will gladly mention you in the README.md and AUTHORS.md and other relevant files as the original author of the library.
  3. I am happy for you to continue with contributor access to the project.

If you aren't comfortable with my plan above, then I'm open to alternatives, but whatever it is I just need to be comfortable in our shared understanding about timeliness of reviews and merges as well as some alternative communication method that I can contact you at. I don't ever expect you to jump on anything happen instantly, overnight, or even within a week really, but 9 month delays on PRs that will actually make the library work again are unfair to me and the users of Iconic. If it was a simple feature add, this would be a different thing, but as it stands Iconic just doesn't work for someone finding it today. If we can't come to a shared understanding then I plan to create a entirely new project and advertise it as a modern and supported replacement for Iconic so that users aren't left out in the cold any longer because my research hasn't found anything like Iconic available. I'd rather not do this, because Iconic is a great name, it has a great community and a good history which I'd rather get back to perfect from its less than perfect status right now vs having to build all of that for myself again. You have done great work on Iconic, it's a fantastic library which is the most powerful of any Swift icon font library available, even today, and you should be commended on it. I understand that situations change and you aren't using this library anymore, that's totally fine. But since it seems you won't be a good steward moving forward (since you aren't using it anymore) I am hoping to pick up where you left off. I have been using Iconic for almost 2 years now and have made it a cornerstone of Home Assistant Companion.

There's a lot of work that I want to do to Iconic in the near term like upgrading to Swift 5 and getting a workaround in place for the Swift 5 build times, but I can't do that if PRs aren't getting merged, reviewed or even acknowledged for months at a time. I've just now submitted a PR to add support to SwiftGen for extracting code points from a font to be used in a template. This will allow Iconic to finally move away from the hacked version of SwiftGen contained in this repo that isn't even buildable anymore because it hasn't been updated since Swift 2.3 (and I've been unable to update it as such a old version of Swift seems impossible to run on a modern Mac).

Finally, I'll apologize for coming off so strongly about all of this, but I am just supremely frustrated now with how long this pretty straightforward PR has taken to get merged or for me to even hear back from you. I'm sure that as a fellow developer, if the shoe was on the other foot, you would be just as frustrated as I am now.

Thanks for reading @dzenbot and hopefully understanding my feelings on this.

dzenbot commented 5 years ago

Hey @robbiet480, I understand your frustration, I've been there many times myself in the past. I totally forgot about this tbh and I also blame GH notifications being very silent. Anyhow, I'm willing to transfer the ownership of this repository to the home-assistant account. I feel less excited to transfer it to an individual account than to an actual community group.

Mention on the README.md and AUTHORS.md and other relevant files as the original author of the library, sounds fair and more than enough to me. Finally, having admin access to this repo, even though I might not really use these rights, would make me happy.

Let me know. I can flick the switch whenever you want.

robbiet480 commented 5 years ago

@dzenbot Transferring to @home-assistant is totally fine too, I am a admin and can accept the transfer any time. I have no problem with adding you as an admin either.

robbiet480 commented 5 years ago

@dzenbot Oh sorry, one other modification to the plan because we are transferring to @home-assistant: All repos under the organization are required to have a CODE_OF_CONDUCT.md file and a LICENSE.md file. I assume you are fine with both (you already have the LICENSE file, I'll just reformat it to be Markdown and add the .md file extension). We prefer to have everything licensed Apache 2.0 as per our governance requirements but can make an exception for this if you feel strongly about keeping MIT. License would be in effect as of the next release of Iconic.

robbiet480 commented 5 years ago

@dzenbot Finally, can you just let me know once you start the transfer? Thanks!

robbiet480 commented 5 years ago

@dzenbot Just tested transferring a new repo in with a user who doesn't have permissions. Seems I need to give you admin permissions to the organization temporarily to allow you to transfer in the repo. Doing so now.

robbiet480 commented 5 years ago

You should be able to accept the invite at https://github.com/home-assistant (or via the link in your email). Once you've done that I can increase your permissions. Alternatively to all of this, you should be able to add me as a admin to dzenbot/Iconic and then I can start the transfer process.

dzenbot commented 5 years ago

@robbiet480 I've accepted the invite. Mind bumping the permissions temporarily?

robbiet480 commented 5 years ago

@dzenbot Yep, standby. I am now giving you permissions to transfer. You had to accept the invite first. :)

robbiet480 commented 5 years ago

@dzenbot You should be good to transfer now, let me know.

dzenbot commented 5 years ago

@robbiet480 Ok, Iconic has a new home now! https://github.com/home-assistant/Iconic Thanks for wanting to carry on with this project. I hope one day to use it for another app project, on the meantime, happy coding!

robbiet480 commented 5 years ago

@dzenbot Thanks so much! Just wanted to confirm if you had a reason for me to keep the license as MIT or if I can migrate it to Apache 2.0. Also, don't be alarmed if you see some "removed from team" notifications from GitHub in the next few minutes, just organizing teams in general. Will ensure you are left with admin access.

robbiet480 commented 5 years ago

@dzenbot Another thing, can you please add me to the Pod so I can publish? Should just need to run pod trunk add-owner Iconic me@robbiet.us to get it done. Thanks!

dzenbot commented 5 years ago

@robbiet480 Apache 2.0 sounds good to me. Thanks.

homeassistant commented 5 years ago

Hi @jkyin, @stevenp, @zvving, @joeljfischer,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

robbiet480 commented 5 years ago

Everyone that just got notified, don't worry about the CLA, it's not gonna apply on this PR.