martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.25k stars 258 forks source link

Scintillua #916

Closed moesasji closed 3 years ago

moesasji commented 3 years ago

This is a first stab at bringing the lexers in sync with upstream. What has been done:

1) all lexers unchanged in vis since last sync updated. 2) added upstream lexers not present in vis. 3) where patches were obvious the lexer was updated with the changes pushed upstream as pull-requests. 4) opened a pull-request for all lexers in vis not in upstream, but left them in the legacy format for now.

There are still a number of lexers to do, but those are not that obvious or the commits I see in vis seems to have removed functionality. I'll add a list of those to the bug-report on this.

moesasji commented 3 years ago

I get the idea and now working on cleaning this up made easier by @orbitalquark already merging the pull-requests that made sense.

moesasji commented 3 years ago

We will eventually have to update the lexer README with updated references to the new API and lexer template.

Agreed, just left that one for now.

Please provide this list. The removed functionality is likely due to performance issues (e.g. #797, #726), maybe upstream also has suggestions how to improve it. In general the same methodology, i.e. first committing the upstream version followed by our own changes on top, should be applicable.

Most of the ones that I haven't yet touched are ones that required a bit more thought as I opted to do the easy ones first. The list of files that still need a look with comments added:

btw) the ones labeled performance fix should be not be difficult, but I just left them out in my first stab at it.

ghost commented 3 years ago
  • [ ] fennel.lua (not the same origin - vis version seems more extensive)

Vis version is not more extensive, it's just that the vis copy of Scintillua does not support inheritance. Upstream Scintillua made it possible lexers to inherit from each other, in order to avoid duplication:

fennel.lua:9

martanne commented 3 years ago
  • [ ] bash.lua (more than just a couple)

The most recent changes are related to here documents, @silasdb can probably make sure they work as expected with the upstream version? The commit message of 21727ae3dce8ccea2e342d3b62b5fa67b1b5a936 also gives an example with variable substitution.

  • [ ] erlang.lua (vis commit removes a lot and appears wrong)

I am not exactly sure what the intention was in #632, the description is very vague. Maybe @emfa can clarify, until then drop the changes and go with the upstream version.

  • [ ] markdown.lua (not sure how vis changes fit with upstream)

Check whether #823 still happens with the upstream version. (Not sure whether it actually works correctly in vis either).

  • [ ] php.lua (more than just a couple, not an easy one)

The issue fixed in 0ff02a70f911327e8a1f92881f5ef43b7cd0e7c8 is that ?> should break out of PHP mode. The other changes just add constants and keywords, it is probably best to fetch the latest ones from the the documentation and incorporate them into the upstream version.

  • [ ] scheme.lua (more than just a couple, not an easy one)

64554981c786c7a95e64c9f4336d3624d7239672 has descriptive commit message, maybe @ii8 would be willing to update it to the new upstream API

  • [ ] text.lua (upstream empty, vis whitespace only)

This is such that replacement symbols are visible, see 807ba6f6850bf7dbf86fd65299248f2d9cba7f75. It is not relevant for upstream. We should just convert it to the new API.

  • [ ] toml.lua (initially didn't see why indent error got removed)

This might have been a misunderstanding on my part.

moesasji commented 3 years ago
  • [ ] text.lua (upstream empty, vis whitespace only)

This is such that replacement symbols are visible, see 807ba6f. It is not relevant for upstream. We should just convert it to the new API.

  • [ ] toml.lua (initially didn't see why indent error got removed)

This might have been a misunderstanding on my part.

To prevent double work. I've opened pull request on these and several other ones, see: upstream That would tell whether or not they apply to upstream.

silasdb commented 3 years ago

Hi! Is it a better practice to send lexers PRs to upstream instead of sending them to vis?

martanne commented 3 years ago

Is it a better practice to send lexers PRs to upstream instead of sending them to vis?

In principle sending them upstream is preferable. However, this assumes both projects use the same format which is not yet the case, but is the goal of this PR. Hence, if you could make sure that the upstream bash.lua works as expected and contains all the necessary fixes that would be appreciated.

moesasji commented 3 years ago

I'm hitting a bit of a glitch with the upstream markdown lexer. Out of the box the upstream version fails to render the headers in red and none of the patches that are applied to the legacy version in vis would affect that behavior.

As the code in the lexer looks fine to me....is this problem caused at this end, i.e. did we miss something in the changes made in vis.lua to be able to switch away from the legacy lexer?

ghost commented 3 years ago

Hi @moesasji!

I think the glitch is due to the Markdown lexer using themeable colors (as opposed to hard-coded ones): markdown.lua:19 markdown.lua:54

Some themes have a colors table defined and exported - solarized.lua:23, but the default one doesn't. (And even solarized.lua doesn't currently have the black color for the hr rule above.) If you do :set theme solarized, you'll see that the headers become red.

For a quick test, you can try replacing lexer.colors.red with 'red'. But in the long term, I think it's better to add a colors table to all the themes, so vis could use unmodified lexers from Scintillua.

martanne commented 3 years ago

It should be fixed by abee98578c0ea4d18aeb14b1e5ee66b67be2be89?

moesasji commented 3 years ago

It should be fixed by abee985?

That indeed fixes the issue for me. Markdown headers have the right color again. Thanks for the quick fix and @lepotic for spotting it!

btw) with this being a subset probably worth keeping fixing the color theme on the todo-list to get the colors one would expect.

ghost commented 3 years ago

It should be fixed by abee985?

Yes, and it's better than what I suggested (duplicating the table in each theme). :)

moesasji commented 3 years ago

Not sure what I did wrong to trigger build failures for this recent sets of commits. Any thoughts on where the mistake is would be greatly appreciated?

In any case this should be getting closer. Things left to do:

ghost commented 3 years ago

I found where the Markdown lexer goes wrong, but don't have a solution yet. Actually, it's not the lexer's fault (literally the same lexer works fine in Textadept), more like the vis integration is missing something.

If you do

-lex:add_style('code', lexer.styles.embedded .. {eolfilled = true})
+lex:add_style('code', {back = 'red'})

code blocks will be highlighted.

lexer.styles.embedded is a table with a metatable, adding some magic I don't understand. There are more lexers that use lexer.styles.* and are probably subtly broken as well. I'll see what I can do tomorrow.

moesasji commented 3 years ago

Actually, it's not the lexer's fault (literally the same lexer works fine in Textadept), more like the vis integration is missing something.

If you do

-lex:add_style('code', lexer.styles.embedded .. {eolfilled = true})
+lex:add_style('code', {back = 'red'})

code blocks will be highlighted.

It appears to be the same cause as the earlier markdown issues as these styles appear to be defined in the color-themes used for textadept, see for example this one. Still puzzling as this one is set in the default themes except for zenburn

martanne commented 3 years ago

The issue is indeed related. Not sure what the best/most elegant way to fix it is. This patch seems to work in my very brief test.

That also suggest we should probably review the (default) themes.

moesasji commented 3 years ago

With the last commits I've pushed we've reached the state I'm pretty much done with the bits I'm confident dealing with. To summarize what is remaining:

martanne commented 3 years ago

Thanks. I rebased once again, incorporating the latest upstream changes to the PHP, jq and text lexers.

I also moved the lexer template back into the README for now.

Another TODO item is to port existing lexers using either deprecated functions or the old style:

I saw that upstream (@orbitalquark) also has an issue tracking the progress.

eworm-de commented 3 years ago

Was this closed by intention?

moesasji commented 3 years ago

Was this closed by intention?

No, this issue is still being worked on. Github for some reason closes a pull-request when the branches are in sync as I made the mistake to push to my branch after the rebase and I can't reopen it.

moesasji commented 3 years ago

@martanne Did you have any luck figuring out the best way to fix the issue with the new color theming as mentioned here?

mcepl commented 2 years ago

According to upstream “All lexers except rest have been migrated.”