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

Fix(syntax): \lstset syntax highlighting #2001

Closed
wants to merge 6 commits into from
Closed

Fix(syntax): \lstset syntax highlighting #2001

wants to merge 6 commits into from

Conversation

144026
Copy link
Contributor

@144026 144026 commented Mar 22, 2021

Fixes #2000

Before fix

image

Note that the red } is somehow classified into a texGroupError:
image

After fix

20210322235220

@lervag
Copy link
Owner

lervag commented Mar 22, 2021

Thanks! I assume #1999 was a duplicate as well? I'll look into this asap. I'll close #2000 and, if necessary, we'll discuss the related issue here. I hope that's OK by you.

@144026
Copy link
Contributor Author

144026 commented Mar 22, 2021

Thanks! I got something wrong in #1999 so I closed it, and opened this PR after I got things work.

Copy link
Owner

@lervag lervag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks! I had a very minor comment - I could probably just do that myself, but I also think it was worth suggesting it to allow a reply/additional comment.

autoload/vimtex/syntax/p/listings.vim Outdated Show resolved Hide resolved
@lervag
Copy link
Owner

lervag commented Mar 23, 2021

Again, thanks! When I looked at it now, I realized: the reason I had the texLstsetGroup linked to texOpt was to make it look similar to the other option groups. I.e., to make the \lstset{...} argument look similar to the option group for \begin{lstlisting}[...]. I agree that it looks inconsistent. Perhaps a better idea is to rename that group, e.g. texLstsetOptarg or similar. I have to admit, naming things is difficult! A pragmatic choice is to leave this as it was (linked to texOpt), but renaming it (and keeping the link) may be better.

@@ -15,8 +15,8 @@ function! vimtex#syntax#p#listings#load(cfg) abort " {{{1

" Match \lstset
syntax match texCmdLstset "\\lstset\>"
\ nextgroup=texLstsetArg,texLstsetGroup skipwhite skipnl
call vimtex#syntax#core#new_arg('texLstsetGroup', {
\ nextgroup=texLstsetArg,texLstsetArg skipwhite skipnl
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for duplicate texLstsetArg. :)

@lervag
Copy link
Owner

lervag commented Mar 23, 2021

Yes, texLstsetArg might actually work very well. The reason I did not suggest it was because there is already a more specific texLstsetArg group; however, I don't think it is a problem. If the tests pass and things look good on your end, then I think we'll settle for this solution. If there should be any issues with it, we can solve them as they come.

@144026
Copy link
Contributor Author

144026 commented Mar 23, 2021

OK! Both github-ci and my local tests seem ok for now. I think the \lstset problem is almost settled.

But I would like point out why I did once choose to link to texGroup: (although it's out of the \lstset problem`)

The highlight for texCmd and texOpt are the same (under my base16 colorscheme), and it looks quite odd when I write multiple lines of texOpt & texCmd together:

image

In comparasion, the default Vim highlight is more reasonable:

image

@lervag
Copy link
Owner

lervag commented Mar 23, 2021

OK! Both github-ci and my local tests seem ok for now. I think the \lstset problem is almost settled.

Great!

But I would like point out why I did once choose to link to texGroup: (although it's out of the \lstset problem`)

The highlight for texCmd and texOpt are the same (under my base16 colorscheme), and it looks quite odd when I write multiple lines of texOpt & texCmd together:

Ah, I see. They are not really the same, actually. See here:

highlight def link texCmd Statement
highlight def link texCmdSpaceCodeChar Special
highlight def link texCmdTodo Todo
highlight def link texCmdType Type
highlight def link texComment Comment
highlight def link texCommentTodo Todo
highlight def link texDelim Delimiter
highlight def link texEnvArgName PreCondit
highlight def link texError Error
highlight def link texLength Number
highlight def link texMathDelim Type
highlight def link texMathEnvArgName Delimiter
highlight def link texMathOper Operator
highlight def link texMathZone Special
highlight def link texOpt Identifier

texCmd is by default linked to Statement, while texOpt is linked to Identifier. These groups should have different highlighting according to group-name. Thus, I would propose that this problem is really a problem with your colorscheme. You could either fix that "upstream", or you could apply some changes to your tex highlighting by customising the colors manually.

lervag added a commit that referenced this pull request Mar 23, 2021
@lervag lervag closed this Mar 23, 2021
@lervag
Copy link
Owner

lervag commented Mar 23, 2021

I think this was already good now, so I merged. Feel free to suggest further improvements/adjustments (either PR or issues).

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.

syntax highlighting in \lstset
2 participants