Skip to content

Comments

Add the option showOriginalLineNumber,#51

Closed
GehDoc wants to merge 4 commits intoborgar:masterfrom
GehDoc:show-original-line-number
Closed

Add the option showOriginalLineNumber,#51
GehDoc wants to merge 4 commits intoborgar:masterfrom
GehDoc:show-original-line-number

Conversation

@GehDoc
Copy link

@GehDoc GehDoc commented Jun 25, 2018

Add the option showOriginalLineNumber, to map the elements of HTML output, with the line numbers of input text.

@GehDoc
Copy link
Author

GehDoc commented Nov 5, 2018

Hi @borgar ,
I've pushed this (and done a bug report to your repo some months ago), because I'm working on a Textile viewer which needs to synchronize scrolling between the source Textile code, and the visual HTML representation.
Tell me please, if it interests you to integrate my works in your library (yes/no/fix the FIXME before/...), or If you prefer that I continue working on my fork without bothering you !
Best regards

@borgar
Copy link
Owner

borgar commented Nov 8, 2018

I am sincerely sorry for the failure to reply to this. The notification came up just before I had to take a leave for health reasons and I had forgotten everything once I returned. It was by no means my intention to make little of the time and work you have put into this.

I am onboard with having this kind of offset info in the output but I feel this would be an opportunity to take things a little further than you have done here. I have some ideas that I want to explore over the weekend that might make this reduce complexity instead of adding to it. Allow me to intrude a bit more on your already tested patience to hack at this over the weekend before I make up my mind.

My ideas, should you be curious:

  • All nodes in the parse tree can just always return input offset, it is up to the renderer to decide what to do with them. We can just assign a "no render" prefix (such as _ or .). It has no real downside in praxis to just have "mostly empty" attributes for most nodes: [ 'p', { _offset: 123 }, ... ]. I think this might remove a lot of "if (pba)" cases in the parser.

  • There should be an intermediary step that allows visiting the nodes in the tree and mutate them. This could be used to solve things like Xss attack #42, and in your case can do something like: attr['data-line'] = charPosToLine(attr._offset);

  • My original intention was the the ribbon() interface would do more of the work which ended up scattered over the parser. It should be possible to use it to pass "current offset" down the callstack.

@GehDoc
Copy link
Author

GehDoc commented Nov 17, 2018

Nice to see that you are interested by this feature ! I had to stop looking at this after June for personal reasons, and now I'm back on it : Mainly, publishing my work based on it (If you are interested, we can talk about it by mail : see my profile). If you want some help to update your live test page with it, I can do it once you have stabilized the API : I'm not interested in maintaining my fork, and I think it's better when people can rely on only one repository.

My feelings :

  • your idea of having the ribbon doing more of the work seems good : it can remove my FIXME (creating a map of : charpos -> offset line) by computing it on the fly. And at the end, getting the total input line numbers.
  • I don't like to add loops on data. But, having a mutation step is interesting, if you provide a way to set, through the options, a function hook called at this step. No idea yet, about parameters feeding such a hook, but It may alllow for exemple, to rewrite relative links for a non-web textile editor.

@craigkovatch
Copy link

Resurrecting a very old thread here :) Did this come close to any kind of completion? We would very much like this feature, too.

@GehDoc
Copy link
Author

GehDoc commented Feb 25, 2021

@craigkovatch : you can use the master branch of my fork.
It is up to date with this repo regarding textile language support, but I didn't care much on lintering / packaging, and have no time to polish the solution.

The feature you need is totally operational, tested, and used in the VSCode textile extension published in another repo of my github space.

@borgar
Copy link
Owner

borgar commented Mar 7, 2021

I am working on a much needed update of the library that includes a better parse tree where nodes are tagged with source offsets, making it easy to link output nodes to input lines. I am hoping to be able to release the update around the start of April.

@craigkovatch
Copy link

@borgar that sounds great! It turns out the fork is not super useful to us because we want to manipulate the textile parse tree, i.e. before it has been transformed to jsonml. Is it possible to hook into this stage somehow? Seems unexposed, even internally, currently.

@borgar
Copy link
Owner

borgar commented Mar 8, 2021

we want to manipulate the textile parse tree, i.e. before it has been transformed to jsonml.

I am replacing jsonml entirely with a better structure (more akin to HTML DOM elements). For the very reason that I wanted the option to apply transformations to the tree before rendering.

To solve things like #42, you would do something this:

const tree =  textile.parseTree(`"google 2":javascript:alert('XSS')`);
tree.visit(node => {
  if (node.attr.href) {
    node.attr.href = escapeJSUrl(node.attr.href);
  }
});

Is it possible to hook into this stage somehow? Seems unexposed, even internally, currently.

I will get this up here as a pull request asap. The branch is functional but needs more tests and cleanup work. It would be good to get some feedback, so if you want to test then that could be ideal for everyone. 🙂

@borgar
Copy link
Owner

borgar commented Mar 13, 2021

An work-in-progress next version is now accessible: #69

@borgar
Copy link
Owner

borgar commented Mar 13, 2021

I'm closing this PR as the #69 supersedes it.

@borgar borgar closed this Mar 13, 2021
@craigkovatch craigkovatch mentioned this pull request Mar 20, 2021
12 tasks
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

Successfully merging this pull request may close these issues.

3 participants