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

Possibility to overnight writer methods #14

Closed
lafriks opened this issue Oct 17, 2019 · 15 comments
Closed

Possibility to overnight writer methods #14

lafriks opened this issue Oct 17, 2019 · 15 comments

Comments

@lafriks
Copy link

lafriks commented Oct 17, 2019

Currently it is impossible to override HTML writer methods to fine-tune them. For Gitea project we need custom link/image URLs but only way to implement this is to copy all HTML writer and change single method. Mostly it is because of nodeAsString method that clones writer and also WriteNode usage that passes HTML writer all over the place

@lafriks
Copy link
Author

lafriks commented Oct 17, 2019

For good example you can check blackfriday v2 renderer interface/HTML writer implementation that is easy to customize for specific needs

@niklasfasching
Copy link
Owner

niklasfasching commented Oct 18, 2019

I'd love to do that and actually tried to do it with the current interface (i was mistaken about how embedding works and thought it to be like inheritance), see here (note the comment below the diff).

At a glance I don't see how blackfriday v2 makes this possible, could you elaborate? In any case I'll look into this next week and hope blackfriday presents an elegant solution that we can imitate.

@lafriks
Copy link
Author

lafriks commented Oct 18, 2019

Check this PR in Gitea how we are overriding it for blackfriday:
go-gitea/gitea#8560

@niklasfasching
Copy link
Owner

Thx! I missed WithRenderer while skimming the code yesterday. Looks simple and clean enough. Will add it end of next week :)

@niklasfasching
Copy link
Owner

niklasfasching commented Oct 27, 2019

The blackfriday approach requires extensive code changes (no nested WriteNodes). I went for something simple for the MVP - let's see if we run into limitations of this approach and iterate from here.

Implemented in 14900e9.
Usage:

type ExtendedHTMLWriter struct { *HTMLWriter }
func (w *ExtendedHTMLWriter) WriteText(t Text) {}

func NexExtendedHTMLWriter() *ExtendedHTMLWriter {
	htmlWriter := NewHTMLWriter()
	extendedWriter := &ExtendedHTMLWriter{htmlWriter, 0}
	htmlWriter.ExtendingWriter = extendedWriter
	return extendedWriter
}

@niklasfasching
Copy link
Owner

feel free to re-open this if you have anything - I just like to keep my inbox empty so I'm closing this for now.

@lafriks
Copy link
Author

lafriks commented Oct 30, 2019

Yes, it does work, thanks. It is still tricky with needing own nodesAsString and emptyClone functions but otherwise it does work great. Thanks again!

@niklasfasching
Copy link
Owner

niklasfasching commented Oct 30, 2019

Could you point me to the part where you run into this problem? I thought about getting rid of cloning but in the end thought it wasn't needed. In any case the code is still there and maybe this solves your problems? f0ebfce

We could also expose NodesAsString on the writer interface if that helps - i didn't see a reason to until now but whatever you run into, feel free to suggest improvements

@lafriks
Copy link
Author

lafriks commented Oct 30, 2019

I need them because I'm overriding WriteRegularLink function and it needs NodesAsString function to get rendered nodes for link, see:

https://github.com/go-gitea/gitea/pull/8560/files#diff-ec1158a92d009b3ac2d4241bac8c8e63R92

@niklasfasching
Copy link
Owner

niklasfasching commented Oct 30, 2019

Thx!

Don't have time to look into this tonight but sounds like this would be solved by exporting nodesAsString (which already forwards to the extendingWriter). How about 3c380c8 ?

@lafriks
Copy link
Author

lafriks commented Oct 30, 2019

It does not work correct with that commit, it renders text that should be between <a> tags before link but not inside it:

        	            	expected: "<p>\n<a href=\"http://localhost:3000/gogits/gogs/WikiPage\" title=\"WikiPage\">WikiPage</a>\n</p>"
        	            	actual  : "<p>\nWikiPage<a href=\"http://localhost:3000/gogits/gogs/WikiPage\" title=\"\"></a>\n</p>"

@lafriks
Copy link
Author

lafriks commented Oct 30, 2019

I had to make clone function this way to get it working for me:

func (r *ExtendedHTMLWriter) emptyClone() *ExtendedHTMLWriter {
	wcopy := *(r.HTMLWriter)
	wcopy.Builder = strings.Builder{}

	rcopy := *r
	rcopy.HTMLWriter = &wcopy

	wcopy.ExtendingWriter = &rcopy

	return &rcopy
}

func (r *ExtendedHTMLWriter) nodesAsString(nodes ...org.Node) string {
	tmp := r.emptyClone()
	org.WriteNodes(tmp, nodes...)
	return tmp.String()
}

@niklasfasching
Copy link
Owner

niklasfasching commented Oct 31, 2019

Can you try again? #15
I remembered why I removed cloning 😅 (circular references, will amend commit message) - without it it works afaict. You should be able to just use WriteNodesAsString from the parent now.

@niklasfasching
Copy link
Owner

niklasfasching commented Nov 2, 2019

Thx for testing, merged in #15 (v0.1.8). Again - feel free to re-open / open another ticket if there's anything else :)

@lafriks
Copy link
Author

lafriks commented Nov 3, 2019

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

2 participants