Asset Catalog - Named Colors #51

djbe · 2017-08-13T21:01:32Z

Templates PR: SwiftGen/templates#68
Fixes SwiftGen/SwiftGen#315

AliSoftware · 2017-08-15T23:20:34Z

Sources/Stencil/AssetsCatalogContext.swift

+        return [
+          "type": "color",
+          "name": name,
+          "value": value


The "value" used here is not the "color value" as in "RGB components". That name could be misleading.
I suggest we structure that context with "type"="color", "name"="basename-of-the-color-without-group-path", "fullname"="name-of-the-color-with-group-path-to-use-in-init" but also red, green blue and alpha keys to match what's output by the ColorParser, in case some people want to use that for anything other than UIColor(name: "full name") — e.g. for generating some some HTML to display the colors or whatnot…

See my comment in the PR, we can't do that for components, colors can be different on a device basis (and other criteria).

Should I still rename value to something else here?

Well I think fullname would be better here.

Only problem is, for consistency we should do the same naming for images which would then be breaking 😥

yeah, so might be better to leave it as is? It's not like value is a bad name, it's the value you eventually pass to UIImage(named: ) or UIColor(named: ), the rest is window dressing.

Yeah, could be a misleading context variable name, more misleading for colors that it was for images, but ¯\(ツ)/¯ let's keep it simple.
We could always decide to rename both in a future 6.0, let's not do breaking changes again to the context variables that soon after 5.0 for now 😉

AliSoftware · 2017-08-17T22:26:14Z

Sources/Parsers/AssetsCatalogParser.swift

     */
-    static let supported = [imageSet]
+    static let supported = [colorSet, imageSet]


So glad we handled that whitelist, we were ready already 👍

I'm wondering though if we shouldn't have used cases here, i.e.

enum Item: String case colorSet case imageSet static let supported: [Item] = [.colorSet, .imageSet]

That way would would have ensured at compile time that everywhere we have a switch to handle each case we never forget to handle every supported cases and we have an exhaustive switch never forgetting places to add new code with new types being supported.

Of course that would mean handling the case of standard folders without extension (the last case on line 110) outside the switch, like in a separated if block or whatnot, but at least that would add safety provided by the compiler, so…

AliSoftware · 2017-08-17T22:44:01Z

Sources/Parsers/AssetsCatalogParser.swift

@@ -104,6 +107,9 @@ extension AssetsCatalogParser {
    let type = item.extension ?? ""

    switch (type, AssetCatalog.Item.supported.contains(type)) {


Remark about the code below on lin 110:

case ("", _), (_, true):

I think that was meant to handle when the folder either don't have an extension (("", _), or have an extension just because it happens that the group/folder name contains a . in its name, but that has nothing to do with the extensions like .colorset, .iconset or other extensions we can encounter in the asset catalog structure. But that latter case doesn't match the behaviour tested by the second condition (_, true), right?

The case (_, true) is matched if AssetCatalog.Item.supported.contains(type) and given than AssetCatalog.Item.supported is an array containing the two extensions previously already tested (colorset and imageset) then those case are guaranteed to already be handled

And what we intended to do instead is for cases like a group named "my.icons" where icons would be considered as the extension while that folder should be considered as a standard group. But is that even possible (do asset catalog allow group names with . in them?), and if it's accepted, we should instead test the folder extension against unsupported extensions (and I'm pretty sure that's what was done in the first version introducing that code before we decided to change from a blacklist to a whitelist, overlooking that change in the condition meaning doing that)

It would be nice to take advantage of that PR or do a separate one fix that bug while we're at it (test if . are allowed by Xcode and if they are either test against unsupported or think of a better logic)

Good catch, that should have been case ("", _), (_, false):. I think that was a remainder from when we had a blacklist. Xcode allows group names with . in them. Groups with extensions that match an Xcode set extension are considered of that set type.

We have 2 options:

maintain a list of all asset catalog types, and have a supported & unsupported list so we can correctly match actual sets <-> groups.

ignore all folders with "extensions" in them that we don't support, potentially erroneously thinking that a group is an unsupported set type.

I've implemented option 2 for now, but I think we're better off long term using option 1?

According to the docs (https://developer.apple.com/library/content/documentation/Xcode/Reference/xcode_ref-Asset_Catalog_Format/AssetTypes.html), the group type has no extension, and no periods are allowed in the name of the group.

Following that documentation, option 2 is actually the correct one, and we should ignore groups with periods in them. If, by some quirk in Xcode, periods are allowed, I don't think we should support it unless the documentation changes.

👌 good catch in that documentation, I forgot they published that! (Worth putting a link to it as a code comment at the top of the XCAssetsParser if not already done)

Let's not allow periods in group names and follow the documentation, option 2 it is!

AliSoftware · 2017-08-17T22:45:15Z

Tests/SwiftGenKitTests/AssetCatalogTests.swift

+    XCTDiffContexts(result, expected: "images.plist", sub: .xcassets)
+  }
+
+  func testColors() throws {


Add a third test for an asset catalog mixing colors + images, generating an heterogeneous context, to ensure that's a working use case too.

Done in the templates PR. The "colors" catalog now actually (also) contains an image set.

AliSoftware · 2017-08-19T21:51:31Z

Documentation/Assets.md

-The assets parser accepts one (or more) asset catalogs, which it'll parse for `image set`s and folders.
+The assets parser accepts one (or more) asset catalogs, which it'll parse for supported set types and groups. We currently support the following types:
+- Color Set Type (colorset)
+- Group Type


nitpicking: feels strange to have the "Group Type" in that list be in the middle of the two others, while it's kind of a special case (no extension). Maybe rather put it either at the very beginning or the very end of that list?

AliSoftware · 2017-08-20T14:58:07Z

Sources/Parsers/AssetsCatalogParser.swift

     */
-    static let supported = [imageSet]
+	static let supported: [Item] = [.colorSet, .imageSet]


Now that we use enum cases then that supported constant is actually no longer needed!

AliSoftware · 2017-08-20T15:00:47Z

Sources/Parsers/AssetsCatalogParser.swift

      let name = item.lastComponentWithoutExtension
      return .image(name: name, value: "\(prefix)\(name)")
-    case ("", _), (_, true):
+    default:


Avoid default, prefer case nil: instead. The whole point of having an enum with only the supported cases is also to be sure that if we add a new supported type later the compiler will warn about the places where we forgot to update the code, and default will defeat that purpose 😉

djbe added 3 commits August 13, 2017 22:25

xcode 9 changes

39b533d

parse colors and generate in context

a166445

test colors (and an all context)

be0ac77

djbe added status: WIP type: enhancement labels Aug 13, 2017

djbe mentioned this pull request Aug 13, 2017

Asset Catalog - Named Colors SwiftGen/templates#68

Merged

djbe changed the title ~~Asset Catalog Colors~~ Asset Catalog - Named Colors Aug 13, 2017

AliSoftware reviewed Aug 15, 2017

View reviewed changes

djbe added this to the 2.1.0 milestone Aug 17, 2017

AliSoftware suggested changes Aug 17, 2017

View reviewed changes

catalog item: use cases instead of constants

1122066

djbe removed the status: WIP label Aug 19, 2017

AliSoftware reviewed Aug 19, 2017

View reviewed changes

Add documentation & changelog

bafdc45

djbe force-pushed the feature/xcassets-colors branch from 30ee6ca to bafdc45 Compare August 20, 2017 00:31

AliSoftware suggested changes Aug 20, 2017

View reviewed changes

AliSoftware approved these changes Aug 20, 2017

View reviewed changes

don't use default, remove unused static array

6d0fe41

djbe force-pushed the feature/xcassets-colors branch from bf6946e to 6d0fe41 Compare August 20, 2017 15:24

djbe merged commit a8b2ceb into master Aug 20, 2017

djbe deleted the feature/xcassets-colors branch August 20, 2017 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset Catalog - Named Colors #51

Asset Catalog - Named Colors #51

djbe commented Aug 13, 2017 •

edited

Loading

AliSoftware Aug 15, 2017

djbe Aug 15, 2017

djbe Aug 15, 2017

AliSoftware Aug 15, 2017

djbe Aug 15, 2017

AliSoftware Aug 15, 2017 •

edited

Loading

AliSoftware Aug 17, 2017

AliSoftware Aug 17, 2017

djbe Aug 18, 2017

djbe Aug 18, 2017

djbe Aug 18, 2017

AliSoftware Aug 18, 2017

AliSoftware Aug 17, 2017

djbe Aug 18, 2017

AliSoftware Aug 19, 2017

AliSoftware Aug 20, 2017

AliSoftware Aug 20, 2017

djbe Aug 20, 2017

		@@ -104,6 +107,9 @@ extension AssetsCatalogParser {
		let type = item.extension ?? ""

		switch (type, AssetCatalog.Item.supported.contains(type)) {

Asset Catalog - Named Colors #51

Asset Catalog - Named Colors #51

Conversation

djbe commented Aug 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Aug 13, 2017 •

edited

Loading

AliSoftware Aug 15, 2017 •

edited

Loading