rokucommunity / brighterscript

A superset of Roku's BrightScript language
MIT License
160 stars 46 forks source link

Set m.[xmlElementId] for every component #164

Open TwitchBronBron opened 4 years ago

TwitchBronBron commented 4 years ago

BrighterScript should add a feature to expose every xml element by its ID onto m. For example:

<Rectangle id = "Rectangle1"/>

should somehow generate

sub init()
    m.Rectangle1 = m.top.findNode("Rectangle1")
end sub

This should also work for node inheritance.

Currently, the plan is:

  1. generate a function called something like bs_assignNamedNodesToM() for every component.
  2. If a component extends a parent, update the function to handle whether the parent is calling the function or IT is calling the function. This can be done because we know the Roku calls init from top-to-bottom in component inheritance, so Grandparent would be call 1, parent would be call 2, and child would be call 3. This also prevents exceptions for when child xml nodes don't exists, preventing unnecessary lookups in parent functions.
  3. Inject this call into the init() function of every component. If the component doesn't have an init function, create one for it and inject it into the component's XML.

Here are a few examples (Names simplified for this example).

GrandparentComponent.brs

sub bs_assignNamedNodesToM()
    m.GrandparentGroup= m.top.findNode("GrandparentGroup")
end sub

ParentComponent.brs

sub bs_assignNamedNodesToM()
    'Grandparent
    if m.bs_assignNameNodesToM_index = invalid then
        m.GrandparentGroup= m.top.findNode("GrandparentGroup")
        m.bs_assignNameNodesToM_index = 1

    'parent
    else
            m.ParentGroup= m.top.findNode("ParentGroup")
     end if
end sub

ChildComponent.brs

sub bs_assignNamedNodesToM()
    'Grandparent
    if m.bs_assignNameNodesToM_index = invalid then
        m.GrandparentGroup= m.top.findNode("GrandparentGroup")
        m.bs_assignNameNodesToM_index = 1
    else   

        'Parent
        if m.bs_assignNameNodesToM_index = 1 then
            m.ParentGroup= m.top.findNode("ParentGroup")

        'Child
        else
            m.ChildGroup= m.top.findNode("ChildGroup")
        end if
        'increment the node index so the next init call in the component chain will call the right stuff
        m.bs_assignNameNodesToM_index++
    end if
end sub
georgejecook commented 4 years ago

personally, I'd prefer a scoped function with it's own name in each file, as then we don't duplicate parent/grandparent, and it's clearer for debugging; it's just as easy to implement, imho.

TwitchBronBron commented 4 years ago

The problem with scoped functions is that we are not guaranteed the init() function is unique for a component. Imagine this: Common.brs has init() and is shared by both Comp1.xml and Comp2.xml. If Comp1.xml has Comp1_assignNamedNodesToM and Comp2.xml has Comp2_assignNamedNodesToM, what do you inject into Common.brs init()?

While that's not the best way to structure your application, BrighterScript is not a framework, so we should not be making any assumptions about project structure.

georgejecook commented 4 years ago

Hmmm. Not something I would do but your point is valid, syntactically and perhaps someone does this (shudders). Personally, I would say we only do this for you if you follow the convention.

I like conventions for various reasons but in terms of bs, if we know that a component is XML plus bs file and the bs file is the codebehind file and your init function is in the codebehind file then we can start doing all kinds of stuff now and in the future. We have a known hook.

So I'd personally say, if someone doesn't follow the convention they don't get support for certain festures, just because that simplifies things for everyone.

I'm also not a fan of the single function. I know it's generated code but it's still a violation that base comps know about descendents and the stateful nature is fragile and hard to debug. So if the only reason to go that route (I assume you wouldn't want that function to be like that otherwise) is coz someone can't follow the convention, then ask, how many people? Is it really worth working around it? Could we not just make it work for these init sharing folks if it becomes an issue, or get them to submit a pr, coz I'd wager someone coding like that probs won't want to use bs anyhow.

georgejecook commented 4 years ago

And I know you like to play devil's advocate on these points and respect you for it; but do you really know someone who puts init in a common file?? That file can only ever be used in comps without init from that point on. Its such an extreme thing to do. So have you seen this, or is that a thought experiment, from the darker regions of your mind :p

georgejecook commented 4 years ago

Lastly, your function does not work for components with diverse subcomponent branches. What happens if component a is extended by b c and d and d is extended by component e??

Why are you designing for a world where people share init functions across their entire app, and can only extend a component once and only once??? What are you trying to do??! Madness I say. Utterly bonkers.