search

torhve / weechat-matrix-protocol-script

A WeeChat script in Lua that implements the matrix.org chat protocol
349 stars 52 forks source link

Add partial support for formatting incoming HTML #75

Open her001 opened 7 years ago

her001 commented 7 years ago

Enable the html_formatting var to use this. Currently supports bold, italics, and underlines with weechat attributes. Code, strikethroughs, quotes, and breaks/rules are supported with plain text decorating them.

It would be reasonable to add support for colors and lists in the future. Unfortunately, some things will never be possible with the limitations of Weechat, but there may be ways to approximate them better. All unsupported HTML tags are simply stripped from the message.

This mostly fixes #60.

erdnaxeli commented 6 years ago

This look terribly complex for just a string substitution. What am I missing?

her001 commented 6 years ago

@erdnaxeli I adapted a longest common subsequence algorithm to find the difference between the html and unformatted messages, which extracts just the html tags without needing to do full parsing. Should be faster in many cases while avoiding adding a parsing library just for the limited set of html that matrix clients support and can be displayed nicely in weechat.

...which I really should have explained in a commit message. I'll probably edit the first commit before this is merged.

torhve commented 6 years ago

Do you think we should merge this currently? How well does it work in practice?

her001 commented 6 years ago

In practice, a terminal can only emulate so much of standard html rendering, but it works completely fine (for me) for the most common cases of bold and italics. Support for lists may be the most urgent feature missing from this. Colors shouldn't be too hard to implement, but are rarely used in practice.

In my opinion, it is okay to merge as the feature is off by default and described as experimental. This is fairly low risk.

I was never able to identify or reproduce the issue that @andrewshadura reported in the review, so I cannot comment on that.

andrewshadura commented 6 years ago

@her001, this was happening to me all the time. Even if you cannot trigger it that doesn’t mean the code is correct, and it is definitely not okay to merge it as is.

ptman commented 6 years ago

maybe you could provide an example so that everyone can reproduce?

andrewshadura commented 6 years ago

Sorry, I haven’t got time to debug it. I just enabled the plugin, and it crashed almost immediately.