Adjust component Image

[uziasferreirazup]

Describe the problem
Actual has two components Image and NetworkImage and they do the same think load an image.

Expected behavior
Remove component NetworkImage and add support to load image from network in component Image

Actual Component

data class Image(
    val name: String,
    val contentMode: ImageContentMode? = null
) : Widget()

Expected Component

data class Image(
   val url: String,
    val path: String,
    val contentMode: ImageContentMode? = null
    val typePathImage: TypePathImage
) : Widget()

enum class TypePathImage {
Network,
Local
}

[pedropnaves]

@uziasferreirazup @victormr
I suggest a change in the proposed names.
For me, the following pattern would be clearer.
What do you think?

data class Image(
    val url: String,
    val typeURLImage: TypeURLImage
    val name: String,
    val contentMode: ImageContentMode? = null
) : Widget()

enum class TypeURLImage {
 Network,
 Local
}

[uziasferreirazup]

@pedropnaves I agree with this.

[theffc]

@pedropnaves @uziasferreirazup @victormr
Wouldn't it be better to use something like sealed classes to decrease the number of possible parameters on Image? Because currently you could pass a url and name, but only one of them would be used depending on typeURLImage parameter. If we could describe this dependency on the API itself, it would be clearer for users.

Example creating an image in Swift (I don't know how exactly it would be in Kotlin):

let localImage = Image(type: .local("imageName"))
let remoteImage = Image(type: .remote("https://www.google.com"))

[LG95]

@theffc

Wouldn't it be better to use something like sealed classes to decrease the number of possible parameters on Image? Because currently you could pass a url and name, but only one of them would be used depending on typeURLImage parameter. If we could describe this dependency on the API itself, it would be clearer for users.

But that would essentially mean that separating local and network images isn't an issue, as you would have two classes for images.

Example creating an image in Swift (I don't know how exactly it would be in Kotlin):

let localImage = Image(type: .local("imageName"))
let remoteImage = Image(type: .remote("https://www.google.com"))

This example is problematic for serializing and deserializing, since both would become something like {"url": "<local-or-remote-url>"}. If each type were to have different names, then how is it different than the proposed?

[LG95]

@pedropnaves

data class Image(
    val url: String,
    val typeURLImage: TypeURLImage
    val name: String,
    val contentMode: ImageContentMode? = null
) : Widget()

enum class TypeURLImage {
 Network,
 Local
}

What is the purpose of the name field here? Shouldn't it just be url and the type, as such:

data class Image(
    val url: String,
    val typeURLImage: TypeURLImage,
    val contentMode: ImageContentMode? = null
) : Widget()

[theffc]

@LG95

But that would essentially mean that separating local and network images isn't an issue, as you would have two classes for images.

I talked about "sealed classes" because that's usually what is referred in Kotlin to be semantically equal to "enum with associated values" in Swift, but maybe there is a better solution. The solution that I was talking about is to encapsulate that 3 parameters (name, url, and type) into 1 parameter with 2 possible values (local or remote with a String parameter value).

This example is problematic for serializing and deserializing, since both would become something like {"url": ""}. If each type were to have different names, then how is it different than the proposed?

Yeah, the json itself would still need 2 different properties (e.g: name and url), but the Kotlin API would be better: not allowing users to provide both properties at once.

data class Image(
    val url: String,
    val typeURLImage: TypeURLImage
    val contentMode: ImageContentMode? = null
) : Widget()

The only problem with this is that when it's a local image, the value of url would not look as an URL. So we would loose some expressiveness in the json. But I'm ok with that.

[Tiagoperes]

@LG95

Suppose we want to show the Beagle logo and the application logo in a view. The apllication logo is stored locally and the Beagle logo is remote, in the bff.

• Beagle logo

  • In iOS, Android and Web, we refer to this image as "/images/beagle-logo.png" and the entire location of the image is "https://url.tobff.com/images/beagle-logo.png".

• Application logo

  • In iOS and Android, we refer to this image as "logo"
  • In Web, there are no "names" or "ids" for images. Here, we need a URL. It could be something like "/assets/images/logo.png". The entire location of the image would be "http://url.tomyapp.com/assets/images/logo.png". Note that now the URL is relative to my application URL and not to the BFF URL.

We can't suppress "typeURLImage". If we do so, we have no way of knowing if the url is relative to the bff or to the local application (web). Making it impossible to represent both of the use-cases above.

We can't suppress "name". If we do so, we can't specify an image value to the mobile applications and another one to the web application. It would make it impossible to represent the second use case. If we really want to use only "url" and ditch "name", we could make the identifier of an image (mobile) equal to the url of the same image in the web application. But, I really don't think we should relate an id in iOS or Android to a random URL of the web app.

[LG95]

@theffc @Tiagoperes
I understand both of your ideas, but the thing that bothers me about them is that they still maintain an obscure dichotomy between local and network images, now at the field level instead of the class level. However, I believe the heart of this issue is obscuring this dichotomy. As far as I understand, this dichotomy has an inevitable factor, the image's source (local or remote) must be explicit, which is why I am in favor of the type field. While it does overload the semantics of what is a URL, using only this field provides a nice unification, which is the goal of this issue. Perhaps path is a better name for the unified field?

[LG95]

@theffc
Representing the type and URL via sealed class in this case requires custom serialization and deserialization to end up with the same as a type and URL field anyway, without clear benefits. Regarding a sealed class approach, I think it might be interesting, but it might as well be a sealed Image class, which has local and remote (or network) variants. But then we depart from the unification approach proposed originally in favor of a semantic clarity one.

[Tiagoperes]

@LG95

There's still the second observation of my commentary ("we can't suppress name"). As I said, by unifying the fields "name" and "url", we can't say that the local image is "logo" for mobile applications and "/assets/images/logo.png" for web applications.

Can we do it? Yes, but with a clear loss: the url of the web application would have to be used as the identifiers of the images in the mobile apps. Instead of "logo", in both iOS and Android, this image would be mapped with the key "/assets/images/logo.png". Which I really don't think is a good idea.

We could also process this string "/assets/images/logo.png", using the complete value in web applications and extracting only the filename to use as the image id in the mobile apps. But I think it can be a bit obscure to the developer.

[LG95]

@Tiagoperes
My bad, I didn't fully grasp this part of your comment. Based on it and what @theffc is proposing with sealed classes, I believe this might suit our needs (the names are totally open for improvement):

data class Image(val path: ImagePath, val mode: ImageContentMode? = null)

sealed class ImagePath(val url: String)

class LocalImagePath(webUrl: String, val mobileId: String): ImagePath(webUrl)

class RemoteImagePath(remoteUrl: String): ImagePath(remoteUrl)

It would be used as follows:

val localImage = Image(LocalImagePath("/assets/images/logo.png", "logo"))
val remoteImage = Image(RemoteImagePath("/images/beagle-logo.png"))

I find it better than the initial proposed class because name would be useless for remote source images. However, I question if it is not too similar to a semantically clearer version of our current classes, as such:

sealed class Image(val url: String, val mode: ImageContentMode?)

class LocalImage(
        webUrl: String,
        val mobileId: String,
        mode: ImageContentMode? = null
): Image(webUrl, mode)

class RemoteImage(
        remoteUrl: String,
        mode: ImageContentMode? = null
): Image(remoteUrl, mode)

Which is used like:

val localImage = LocalImage("/assets/images/logo.png", "logo")
val remoteImage = RemoteImage("/images/beagle-logo.png")

[theffc]

@LG95 just to be sure that I understood, this code you are proposing still have a complexity overhead when serializing and deserializing, right?

I preferred the first one. And maybe we could even get more scoped types, like:

data class Image(val path: ImagePath, val mode: ImageContentMode? = null)

sealed class ImagePath(val url: String) {
    class Local(webUrl: String, val mobileId: String): ImagePath(webUrl)
    class Remote(remoteUrl: String): ImagePath(remoteUrl)
}

val local = Image(ImagePath.Local("/assets/images/logo.png", "logo"))
val remote = Image(ImagePath.Remote("/images/beagle-logo.png"))

[LG95]

@theffc only the first option requires custom serialization and deserialization. The second is handled by the beagle type serialization mechanism. I prefer the scoped version of option 1 as you proposed and they are legal Kotlin.

[Tiagoperes]

I'm fine with both approaches, we can express all we need in both of them.

[Tiagoperes]

A question considering the mode "Network":

If the mode is Network, it means that, if the url is relative, it is relative to the backend URL, right?

Suppose the URL to fetch the json with an image is at "http://www.zup.com/beagle/movies". If the image returned in this json is of type Network and has url = '/images/poster36.png'. Which one of the following is the full URL of this image?

• http://www.zup.com/beagle/movies/images/poster36.png
• http://www.zup.com/beagle/images/poster36.png
• http://www.zup.com/images/poster36.png

I believe I have this question because we didn't mess too much with the navigation in beagle-web. I'd appreciate if someone could explain to me how beagle-ios and beagle-android deal with these Urls.

[LG95]

@Tiagoperes I believe we should follow our URL convention and according to it, the final URL would be http://www.zup.com/beagle/movies/images/poster36.png.

[Tiagoperes]

@LG95 how could I express that the URL is actually http://www.zup.com/beagle/images/poster36.png? we don't accept ../ as part of the URL, do we?

[LG95]

@Tiagoperes unfortunately, we only support a very simple relative path. Therefore, http://www.zup.com/beagle/images/poster36.png will need to be expressed as absolute. Alternatively, maybe your base URL should be http://www.zup.com/beagle/. We documented the complete URL convention in https://docs.usebeagle.io/conventions/urls.

[Tiagoperes]

This problem with the URL is deeper and should be discussed in greater details elsewhere. If any changes to the image component emerge from this discussion, I'll post it here.

[victormr]

I have an observation, in this usage:
val local = Image(ImagePath.Local("/assets/images/logo.png", "logo"))

Even if i don't have a web or a mobile app i still have to pass both parameters for no reason. To minimize this problem i have a proposal to instead of the "Local" enum is associated with 2 strings we create a struct to associate with this type. This struct will have multiple initializers so we can ensure that we have the properties we need.

I propose 2 structs:

public struct ImageProperty {
    let name: String
    let path: String
    let imageExtension: String
    
    init(name: String) {
        self.name = name
        path = ""
        imageExtension = ""
    }
    
    init(name: String, path: String, imageExtension: String) {
        self.name = name
        self.path = path
        self.imageExtension = imageExtension
    }
}

public struct ImageProperty {
    let mobileId: String
    let webUrl: String
    
    init(name: String) {
        self.mobileId = name
        self.webUrl = ""
    }
    
    init(webUrl: String) {
        self.webUrl = webUrl
        self.mobileId = ""
    }
    
    init(name: String, webUrl: String) {
        self.mobileId = name
        self.webUrl = webUrl
    }
}

[victormr]

@theffc @LG95 @Tiagoperes Any opinion on this?

[LG95]

@victormr Do you propose these classes as alternatives to the Local class or as additions?

[victormr]

Is a addition, instead of the Local be associated with 2 string to be associated with this new struct so can be used as follows:

val local = Image(ImagePath.Local(named: "logo"))
val local = Image(ImagePath.Local(webUrl: "/assets/images/logo.png"))
val local = Image(ImagePath.Local(named: "logo", webUrl: "/assets/images/logo.png"))

Instead of:
val local = Image(ImagePath.Local("/assets/images/logo.png", "logo"))

Reducing the necessity of sending both parameter every time.

[LG95]

@victormr Sounds reasonable to me.