SwiftGen icon indicating copy to clipboard operation
SwiftGen copied to clipboard

Inconvenient access to Assets of type color & image

Open Jeehut opened this issue 7 years ago • 20 comments

Currently, when I use the default swift4 template for xcassets I need to access my colors like so:

view.backgroundColor = Asset.Colors.Theme.accent.color

When accessing images it looks like so:

imageView.image = Asset.Images.SomeList.add.image

In my opinion this is unnecessarily long. It should rather be like these calls:

view.backgroundColor = Colors.Theme.accent
imageView.image = Images.SomeList.add

Note that I'm aware that we can use our own custom templates (actually we're doing this, see here and there). But I think the default template could be improved in this perspective.

I mean, the Strings call also doesn't look like Asset.Strings.Onboarding.Header.title.string. It's L10n.Onboarding.Header.title which is to the point. The context L10n makes clear the last component is a String and I'm not able to get the key name either so why do I need it for images and colors, but not for Strings? Also why isn't there at least a typealias for the start like:

typealias Colors = Asset.Colors
typealias Images = Asset.Images

I don't seem to understand these points. I'd be glad to hear the rationale.

Jeehut avatar Feb 11 '19 17:02 Jeehut

The very major difference is that assets can be quite memory heavy. In your example templates, all images are stored in memory, which is absolutely not the way to go. If someone has a few MBs of image or data assets, they'd all be at all times in memory.

Something you can do in your codebase is to add extensions to for example UIImageView, to have a function like:

extension UIImageView {
  func set(asset: ImageAsset) {
    image = asset.image
  }
}

djbe avatar Feb 11 '19 17:02 djbe

Note, there is a typealias at the start:

typealias AssetColorTypeAlias = UIColor
typealias AssetImageTypeAlias = UIImage

The reason they have a somewhat long name, is to avoid conflicts with the other templates' output (or other modules). You can always customise these using the template parameters, see the documentation for more information (https://github.com/SwiftGen/SwiftGen/blob/master/Documentation/templates/xcassets/swift4.md#customization).

djbe avatar Feb 11 '19 17:02 djbe

I agree that there's some inconsistency with Assets exposing a .color or .image property vs Strings exposing the string directly. There was a reason why we did that back in the day, but I have to admit I don't remember it well. It might be because of @availablility issues (color assets are only available starting iOS10 or iOS11 I think?).

@djbe I thought about the memory-heavy problem, but thinking back about it, global constants are lazy by default, so that might not actually be a problem after all? (See the PR where we discuss the .color accessor being CPU-intensive)

--

As for the Asset.Colors.… vs Asset.Images.…, I have to admit I have a doubt, do we really create those in the template in any circumstances? I mean, aren't those sub-enums instead derived from your groups (folders) in your Asset Catalogs — so maybe you created a "Colors" group in your asset catalog to group your colors and checked the "provides namespace" on it, and same for "Images" — and not there by default? I'll have to check the templates, but from what I recall sounds strange to me that we create Assets.Color.xx for .xcassets that only have images and no colors for example…

So in your case @Dschee just un-checking the "provides namespace" on your "Colors" and "Images" groups you might have created in your Asset Catalog would solve that second point…

AliSoftware avatar Feb 11 '19 17:02 AliSoftware

The problem still remains that those assets would, once loaded, remain in memory, so that'd still be a very bad idea.

djbe avatar Feb 11 '19 17:02 djbe

Another note: the swift4 color templates also work with a .color property. The literal-swift4 template allows direct access to the colors.

djbe avatar Feb 11 '19 17:02 djbe

@Dschee Another reason why you might have those .Image and .Color sub-enums is if you have separate asset catalogs for each.

The cases when we create sub-enums under Asset. are:

  • if there's more than one Asset Catalog, we generate one sub-enum per catalog name. (If there's only one catalog, we don't add the sub-enum level)
  • when, in a given Asset Catalog, you have created groups (folders) for which you have checked the "provide namespace" checkbox in the Inspector on the right (or if you passed the forceProvidesNamespaces param to your template)

Otherwise, we don't create subenums and colors and images should be directly accessible under Assets.Theme.accent.color

AliSoftware avatar Feb 11 '19 17:02 AliSoftware

@AliSoftware We separate colors from images, so we have a Colors.xcassets file and a separate Images.xcassets file. We have a project template where you can see our configuration.

Jeehut avatar Feb 11 '19 17:02 Jeehut

So, if I get this right then we should add the above typealiases to our project tempalte since it's a result of our setup. That's okay with me, but I still think the .color and .image calls should be removed from the default template if there's no good reason against it.

Thanks for the quick answers by the way @djbe & @AliSoftware! 👍

Jeehut avatar Feb 11 '19 17:02 Jeehut

@Dschee So that's where your sub-enum is coming from, that's because you use separate asset catalogs so we create one sub-enum per catalog, because people separating assets in separate catalogs usually do it for semantic reasons, and expected that separation to be re-transcrived in the generated constants. That's the expected behaviour, at least for 95% of our users.

--

So only question that remains from that issue is your first point: would it be possible to remove the need for .color and .image, without any side-effect — especially like increasing the memory footprint of apps using those constants?

AliSoftware avatar Feb 11 '19 17:02 AliSoftware

@AliSoftware Exactly. That would be interesting to know.

Jeehut avatar Feb 11 '19 17:02 Jeehut

If you want to avoid that sub-enum in your case, you can either:

  • Create a custom template that removed those lines from the template ({% if catalogs.count > 1 %}…)
  • Merge your 2 separate Asset Catalogs into one in your project templates — even if you choose to then create groups (folders) in that merged Asset Catalog to keep them separate and organised, and just not tick the "provides namespace" checkbox)
  • Add a typealias Colors = Assets.Colors as you suggested above

I think the most appropriate solution for you should be solution 2 (only have one asset catalog, and use folders to organise them), but YMMV.

AliSoftware avatar Feb 11 '19 17:02 AliSoftware

The other major reason we have these structs with image/data/... properties (besides the obvious need for lazy-ness), is that most of these types are not available in all iOS/macOS/... versions. @AliSoftware mentioned it above, but it's something important to keep in mind.

Now, we could use computed vars, annotated with available, but that'd be a major breaking change for our users, so no-go until SwiftGen 7.0.

djbe avatar Feb 11 '19 18:02 djbe

Last note: image assets can have trait variations (so they can change with the horizontal/vertical size classes).

djbe avatar Feb 11 '19 18:02 djbe

Good point. Example where it makes it useful to have an ImageAsset rather than the generated constants returning an UIImage directly is that you can pass an ImageAsset and query it's image property at different contexts, returning different UIImages depending on the trait collection.

class MyVC: UIViewController {
  let asset: ImageAsset!
  @IBOutlet let imageView: UIImageView!

  static func instantiate(asset: ImageAsset) {
    let vc = MyVC()
    vc.asset = asset
    return vc
  }

  override func traitCollectionDidChange(_ previousTraitCollection: UITraitCollection?) {
    self.imageView.image = asset.image // recompute with new traitCollection
  }
}

And even if it's not the case yet, in the future we could imagine adding a func image(traitCollection: UITraitCollection?) -> UIImage accessor to ImageAsset too in case people need to specify which one they want to get instead of assuming the default one like we do today. That need hasn't been asked yet, but you see how that could allow more flexibility in our API and how that wouldn't be possible if our generated constants returned UIImage instances directly.

AliSoftware avatar Feb 11 '19 18:02 AliSoftware

Okay, I can see now why you would want to keep the default template this way. But I still can't convince my colleagues to simply use the default template for reasons of wider community knowledge and community maintenance. Since they have never used trait collections (trait collections are not particularly useful if you're developing mainly iPhone apps, they can't even differentiate between a tiny iPhone SE screen and a huge iPhone Xs Max screen which has 5 times the pixels amount) it's hard to understand the need for the last part of the call.

The last hope for me really is an alternative template now: What do you think about an xcassets template named traitless-swift4.stencil (or you name it) where the last part of the call is removed, which also would prevent any breaking changes? Maintenance should be pretty easy since the rest of that template would be unchanged.

Jeehut avatar Feb 12 '19 07:02 Jeehut

Well, for these cases custom templates is what we usually recommend. TBH I tend to avoid shortcuts like these, just in case I need to add iPad support (for example) to an existing app.

Any built-in template we provide should protect the user against their own "mistakes". It'd be unfortunate if our users would have to switch templates and modify their whole codebase if at some point they need trait variations in their apps.

Like @AliSoftware mentioned, we maybe should add the func image(...) accessor that accepts traits, just to cover all use cases.

djbe avatar Feb 12 '19 09:02 djbe

iPad support doesn't make us want to use trait collections. But even if it would, we still wouldn't provide different images for trait collections and even if that was the case, why should we ever want to provide different colors for different traits? To me, the correct naming for the current template would be traits-swift4.stencil and the default one should just not support traits at all.

How many people are using trait-specific images and would need that API, honestly? My gut feeling formed from my personal experience tells me that 95% of the users won't ever need that feature. You guys may live in a world where things are different, but in my world this is just unnecessary API clutter.

Having that said, I don't much care about still using a custom template like we already do in our projects. I just wanted to discuss here if the current templates APIs couldn't be improved. If you're fine with even the .color call at the end and think that the issue should be solved by custom templates, then feel free to close this issue. There's no point of it staying open in that case.

Jeehut avatar Feb 12 '19 11:02 Jeehut

I've just created a simplified template which I'm suggesting to add to the project in #605.

Even if you decide not to merge it, maybe someone will come across this thread and wants to use the simplified template rather than the default one.

Jeehut avatar Feb 13 '19 14:02 Jeehut

Last note: image assets can have trait variations (so they can change with the horizontal/vertical size classes).

Doesn't UIImage handle this internally? Otherwise just storing an image in a variable like in a view model would mean the image doesn't change when the trait collection changes.

Also doesn't UIImage handle caching etc itself, that along with static variables being lazy loaded, should mean that the ImageAsset struct isn't needed

Jon889 avatar Jul 01 '21 00:07 Jon889

Yep UIImage handles caching itself when using the UIImage(named:) init. https://developer.apple.com/documentation/uikit/uiimage/1624154-init

So if the memory gets low it will purge unused images.

I think ImageAsset is basically implementing UIImage which is just a wrapper around the image itself and the details. So if you put a UIImage in a UIImageView and the traitCollection changes, UIImageView will ask UIImage for the relevant image.

Jon889 avatar Jul 01 '21 17:07 Jon889