Pass element (tree) to custom render

[erickok]

Proposal for #584 to add element (tree) to custom render, allowing to wrap the default implementation.

This also ever so slightly cleans the API surface as in a custom render you can grab the attributes from the context.tree.attributes so no need to pass this as extra param.

[DFelten]

I wouldn't remove for example the attributes parameter. It's easier to use for the developer then grab it from the context.

Also this would be a breaking change.

[erickok]

Yes, it would be a breaking change, but not something we did before and we are going to 2.0 now anyways so it would be the perfect time to change this. Because now there are two ways to do the same thing. I also like having all context for custom rendering in the context instead of partly in a context object and party in separate parameters.

[erickok]

@ryan-berger I'd like your opinion on this.

[ryan-berger]

@erickok I think my comment on this would requires I understand @DFelten 's workaround and why making the use case available would warrant a breaking change.

@DFelten Is there anything wrong with wrapping the table in a parent tag? It seems like the library is working exactly as it is supposed to.

This also seems like we are doing a lot of extra work when it comes to parsing. We are saying that we will aggressively parse everything, even if we aren't using it, and then give it to the user to decide if they want to use? I'm not sure how much I like the implications of that, but am opened to being swayed.

[DFelten]

@ryan-berger We would like to edit each table and include a ScrollView if necessary. When we search for the table tag, we find it, but the child is not the entire table. Therefore, as far as I know, we would have to rebuild the table on our own.

We cannot assume that all tables have a special tag as parent. Therefore, we edit the HTML within the app and wrap a custom tag around it. So it's possible to find the custom tag with the full table as a child:

Map<String, CustomRender> customRenderer2(BuildContext context) => {
      'tmxtable': (renderContext, child, attributes, element) => SingleChildScrollView(
            scrollDirection: Axis.horizontal,
            child: SizedBox(width: _calculateTableWidth(), child: child),
          ),
    };

The HTML after we added the tag within the app:

<tmxtable>
      <table>
            ... 
      </table>
</tmxtable>

[erickok]

@DFelten would my proposal make your 'hack' obsolete, or at least much easier to implement. I think so right?

@ryan-berger actually, this is the current situation. If a custom render states support for some tag (say, table) than we parse it's subtree and process the tags (create the widgets) and then pass this to the custom render. It has always worked this way. If the custom render than does something entirely custom (say, return an empty container), this 'work' has indeed been done for nothing. What I propose here is just that we also give the context to the custom render, so it has more info (namely the StyledElement) to override rendering.

Of course as this is Flutter, creating a Widget doesn't mean actually rendering anything in terms of pixels. It is just a spec. Just a side note.

The only thing that is breaking is that I want to omit the two custom render parameters as you can already get them from the render context. BTW we 'broke' the API a bunch of times before, such as when we added the context/attrs/element to onTap and onImageTap.

[ryan-berger]

@erickok I didn't realize that's how we did custom render.... That feels really icky to me, but this isn't the time to be messing with that.

I'm feeling a bit better about adding the tree attribute, as it does allow for a little bit of recursion and searching to be done(right?) in order to custom render something.

However, it seems to me that it would be a good idea to only do work that we need to, and not have that attribute in the long run, and then delegate parsing responsibility solely to the customRender. In that way, if someone needs to do something like this, we can have them return a scrollview with the child of TableElement() and give them relevant information to pass into the constructor of TableElement to parse it rather than building it for them and then handing them info to cut down on the work we are doing. This obviously can't be accomplished in the short term.

"Breaking" the API is okay as long as we are slow about it and do a preview or give good heads up. I'm not sure how much I want to break (i.e. pulling the attributes out), but I think that some breakage will be required

[erickok]

Just to be clear, the tree here is the tree of StyledElement, that is, the parsed html that is still unprocessed (not converted to Widgets). It's the child parameter here (which we always had) which has the 'heavy' element-to-widget processing done already (even when we won't use it).

Once we go to flutter_html 3, with modularisation, we should fix this indeed. And I have a good idea how. Just need to find time (and we certainly want to create a solid test setup first to avoid regression).

[tneotia]

Technically a little off-topic, but do we want to create a roadmap for these planned features? Prior to the above, I didn't know we were planning to do modularization and the refactor for customRender in v3.0. It might be good to include a roadmap in the README or a pinned issue, and any discussion on the future of the library (e.g. large breaking changes or huge new features like .css parsing support) can take place there. This way everyone is informed on the new and breaking features being worked on.

On the topic of customRender, the child widget parsing is definitely not ideal. I'm not the best at API design, I'm just throwing something that popped into my head out there:

1. Have a sort of constructor/variable/function (I don't know what this would look like) that can be returned as a Widget in customRender to be the "child" - this will make the children parse only when needed
2. Otherwise the user can make their own child or not have one at all.

customRender: {
   "p": (context, element, getChildren) {
        return TextSpan(text: context.tree.element.text, children: getChildren.call(context.tree));
    }
}

and

getChildren(StyledElement tree) {
   return tree.children.map((tree) => parseTree(newContext, tree)).toList();
}

I don't know if something like this would work... and I may be under-complicating it.

[erickok]

@ryan-berger So in the end, do we merge this? Then we can take the next step and improve custom rendering (and more) in the next step.

[ryan-berger]

Yeah, lets merge this. It does look like it needs a rebase though

[Shvet]

Is there any simple example to use SingleChilldScrollView inside table tage? I am really in of this as i have right side overflow problem in html tag. I am using Coloumn->Flexible->Row->Flexible->SingleChildScrollView->Html which is not working,

[erickok]

@Shvet I added this to the README for (unreleased) 2.0.0: https://github.com/Sub6Resources/flutter_html/pull/603/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R274

[Shvet]

@erickok I have checked above link, i am still using flutter_html: ^2.0.0-nullsafety.0, so it might not be working as expected because it is showing me table selection when i use code for scrollview in custom rendering.

i would like to know time delay of release of version 2.0.0 as it shows merge is block.

[erickok]

I don't know I'm waiting for the project owners to approve my pull requests. Sorry I can't help any further now.

[Shvet]

@erickok Well thanks for help, I just went to your code repo and downloaded it for temporary. when they will update on pub.dev i will change it back. I need this update in urgent anyway, i can not wait for them to update it.