Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconvenient access to Assets of type color & image #603

Open
Jeehut opened this issue Feb 11, 2019 · 20 comments
Open

Inconvenient access to Assets of type color & image #603

Jeehut opened this issue Feb 11, 2019 · 20 comments

Comments

@Jeehut
Copy link
Collaborator

Jeehut commented Feb 11, 2019

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.

@djbe
Copy link
Member

djbe commented Feb 11, 2019

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
Copy link
Member

djbe commented Feb 11, 2019

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).

@AliSoftware
Copy link
Collaborator

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…

@djbe
Copy link
Member

djbe commented Feb 11, 2019

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

@djbe
Copy link
Member

djbe commented Feb 11, 2019

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

@AliSoftware
Copy link
Collaborator

@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

@Jeehut
Copy link
Collaborator Author

Jeehut commented Feb 11, 2019

@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
Copy link
Collaborator Author

Jeehut commented Feb 11, 2019

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! 👍

@AliSoftware
Copy link
Collaborator

@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?

@Jeehut
Copy link
Collaborator Author

Jeehut commented Feb 11, 2019

@AliSoftware Exactly. That would be interesting to know.

@AliSoftware
Copy link
Collaborator

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.

@djbe
Copy link
Member

djbe commented Feb 11, 2019

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
Copy link
Member

djbe commented Feb 11, 2019

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

@AliSoftware
Copy link
Collaborator

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.

@Jeehut Jeehut changed the title Inconvenient access to Assets of typecolor & image Inconvenient access to Assets of type color & image Feb 12, 2019
@Jeehut
Copy link
Collaborator Author

Jeehut commented Feb 12, 2019

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.

@djbe
Copy link
Member

djbe commented Feb 12, 2019

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.

@Jeehut
Copy link
Collaborator Author

Jeehut commented Feb 12, 2019

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
Copy link
Collaborator Author

Jeehut commented Feb 13, 2019

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.

@Jon889
Copy link

Jon889 commented Jul 1, 2021

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
Copy link

Jon889 commented Jul 1, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants