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

LayoutLoading with support for loading node from Data #90

Closed
mariob opened this issue Dec 6, 2017 · 8 comments
Closed

LayoutLoading with support for loading node from Data #90

mariob opened this issue Dec 6, 2017 · 8 comments

Comments

@mariob
Copy link

mariob commented Dec 6, 2017

Hi,

What do you think about adding support for loading xml data from a Data instance in the LayoutLoading extension?. Currently there's only support for loading from a local file or URL.

Suggested function signature (which pretty much mirrors the LayoutNode's init):

public func loadLayout(
        xmlData: Data,
        url: URL? = nil,
        state: Any = (),
        constants: [String :Any] = [:]
        ) 

This could be used, amongst other things, to load xml blobs from CoreData or having xml data as strings in the code.

I know that it's already possible to do this if you create the LayoutNode yourself using the init(xmlData: Data) initialiser and set it as the layoutNode instance on the LayoutViewController.

However, I think it would be convenient to have the option available in the LayoutLoading extension and use it inside viewDidLoad. I guess the API found in LayoutViewController is what most developer will use to setup the view, right?

Could probably do a PR for this if it's of any interest?

Some neat this that could be done is showing a spinner while loading content from a remote server.

let data = """
<UIView backgroundColor="#767676" text="Hello" 
    xml="https://raw.githubusercontent.com/schibsted/layout/master/SampleApp/PageTemplate.xml">

    <param name="text" type="String"/> <!-- This is required when loading async -->
    <UIActivityIndicatorView top="50% - height/2" left="50% - width/2"
        activityIndicatorViewStyle="whiteLarge" isAnimating="true" color="gray" />
    
</UIView>
"""

loadLayout(xmlData: data.data(using: .utf8)!)

Actually, I think I found an issue while trying this out. If I point to a remote resource, the loading fails with an "unknown property 'text'" error. If I point to the same file locally, it works.

Adding <param name="text" type="String"/> to the body fixes the issue but I don't think it's the correct behaviour, right? Should I file another issue for this?

As a side note... In a post 1.0 world, I think it would be cool to support custom url protocol providers. A protocol provider would be responsible for fetching and providing xml data based on the protocol in the url. For example, I could register a protocol provider, myresources://. When the layout engine finds any xml resources to load with that protocol, it hands over the loading to the provider. A typical provider could be one that fetches a signed resources and before it's handed over to the Layout engine the signature is verified :)

@nicklockwood
Copy link
Owner

I'd like to have a more concrete idea of the use cases for this before adding the new API, as each additional method introduces more maintenance burden if I need to deprecate stuff or add new parameters later.

Storing XML templates in Core Data doesn't seem like a very common use case, and probably isn't optimal if you're trying to implement a cache, and as you say there is already a way to create a LayoutNode from XML in the rare cases that you might need to.

@mariob
Copy link
Author

mariob commented Dec 7, 2017

I guess you're right. If you want more advanced options you should use the LayoutNode API instead of LayoutViewController.

I just think Data is a great abstraction for providing data in APIs 😉 But it's already available in LayoutNode and could be considered for more advanced usage.

Do you think the remote vs. local resource issues where param is required only for remote resources is still a valid bug or is it an expected behaviour?

@nicklockwood
Copy link
Owner

The <param> should be required because text is not a known property of UIView, so it sounds like the fact that it isn’t required when loading a local file might be a bug.

@mariob
Copy link
Author

mariob commented Dec 7, 2017

This is my guess FWIW :)

The file referenced by the UIView tag looks like this (removed irrelevant content):

<UIView>
    <param name="text" type="String"/>
    <UILabel  text="{text}" />
</UIView>

The param is required, but I think that the <UIView xml="my_file.xml"> tag is replaced by the content from the local file system before its body is evaluated. So if the referenced file has a <param> tag all will be fine.

When referencing a remote resource the body of the <UIView xml="my_file.xml"> is evaluated and, of course, the lack of a text property in the body will generate an error.

Does it make sense?

@nicklockwood
Copy link
Owner

@mariob yes, I believe that's the reason. I should probably introduce some concept of "file scope" to make this behavior more consistent.

@mariob
Copy link
Author

mariob commented Dec 8, 2017

Agree. I'll close the issue. If you need it as reminder feel free to reopen it again :)

@mariob mariob closed this as completed Dec 8, 2017
@cadrega
Copy link

cadrega commented Nov 13, 2018

Actually, I have a use case for this: I'm evaluating Layout for use in a new iOS app which will show a dynamic user interface, which will be communicated by another computer in the local network.
I evaluated all my needs and it seems that Layout is the way to achieve this (and I greatly thank you, @nicklockwood for this great work!), but as of now I'm forced to receive the XML data from the computer, save it to a temporary location, and then load it from file.
It would be really better to keep the XML in memory, since I don't really need them to persist in any way.
I'll start the process to try and extend the LayoutLoading protocol for this now, but hey, @mariob , if you have some code, please contact me so that I won't be duplicating your work!
I would love to see this included in the mainline Layout, and will send here the code should I succeed to implement it.

@cadrega
Copy link

cadrega commented Nov 13, 2018

It was less difficult than I thought ;-)
I created this pull request with the needed changes.
Please note that I'm not used at all with git nor GitHub, and this is my very first pull request.
It works for me, but I'd really love if this could be merged to the next official 0.6.x or 0.7.x release.
Thanks!

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

No branches or pull requests

3 participants