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(swayconfig): floating_modifier #14827

Closed
wants to merge 1 commit into from

Conversation

jamespeapen
Copy link
Contributor

Restores correct highlighting of swayconfig's floating_modifier with the normal and inverse options. Both i3 and sway have this keyword, but i3 does not have the normal|inverse. When this match was set as contained in 679f5ab, it prevented the sway options from being recognized.

Added the none option which was not supported but is a valid option in sway config.

Ideally floating_modifier would be a keyword as it is in i3config.vim, but I'm still working that out.

Fixes #14826

@jamespeapen jamespeapen marked this pull request as draft May 22, 2024 21:49
@jamespeapen
Copy link
Contributor Author

setting as draft since some issues remain:
image

The first two lines should show bad normal as errors but do not.

Using

syn match i3ConfigKeyword /floating_modifier \(none$\|$[a-zA-Z0-9+]\+ \(normal\|inverse\)\)$/ contains=i3ConfigVariable,i3ConfigBindModkey,swayConfigFloatingModifierOpts

with the $ outside the alphanumeric match gets
image

Here it looks better except that Mod keys aren't allowed anything following them.

@jamespeapen
Copy link
Contributor Author

Made the capture groups a little more explicit:
image

Let me know if you have any suggestions @JosefLitos. Otherwise I'm happy with this. Since both i3 and sway don't share too many different options for the same keyword like floating_modifier, I'm okay with the current fix.

@jamespeapen jamespeapen marked this pull request as ready for review May 22, 2024 23:49
@JosefLitos
Copy link
Contributor

JosefLitos commented May 23, 2024

The root cause is a change in i3config.vim floating_modifier implementation. I went from a match to a keyword, which takes precedence.

To make them work both properly, I'd suggest leaving the contained keyword in the swayconfig part without the added \s* and reverting the i3config part like:

" 4.7 Floating modifier
- syn keyword i3ConfigKeyword floating_modifier contained skipwhite nextgroup=i3ConfigVariable,i3ConfigBindModkey
+ syn match i3ConfigKeyword /floating_modifier [$a-zA-Z0-9+]\+$/ contained contains=i3ConfigVariable,i3ConfigBindModkey

I prefer this because it sticks to the general file structure - every keyword is under the TopLevelDirective group, which alleviates the need to make changes to all keywords because of some changes with the beginning of the line and so on.

Also, I understand that the 3 separate matches are more correct, but is such preciseness really necessary for such a simple case that gets a single use at most in the entire config? I was thinking about something simpler like

syn keyword swayConfigFloatingModifierOpts normal inverse none contained
syn match i3ConfigKeyword /floating_modifier \(none\|[$a-zA-Z0-9+]\+ \(normal\|inverse\)\)$/ contained contains=i3ConfigVariable,i3ConfigBindModkey,swayConfigFloatingModifierOpts

@jamespeapen jamespeapen force-pushed the floating_modifier branch 2 times, most recently from 53cde3f to c6e0dde Compare May 23, 2024 14:35
@jamespeapen
Copy link
Contributor Author

Thanks! I've reverted the i3 change that caused the issue @hiqua. For the precise matches, I think its more consistent to be precise since other syntax elements are precisely matched, but you have a point that its not going to be used more than once and vim needn't act as a sway linter. I'll go with the simpler option for now but will keep trying to see if there's another way to get that precision.

For a start I did notice two of the errors are recognized if there are any leading spaces:
image

 - fix floating_modifier $mod normal|inverse was being hightlighted as error
   reverting the floating_modifier change from dd83b63
 - will currently allow invalid syntax after floating_modifier

Co-authored-by: JosefLitos <litosjos@fit.cvut.cz>
@chrisbra
Copy link
Member

Thanks. So is this good to be included? Or are you still working on it? @JosefLitos can you please ACK as well please?

@jamespeapen
Copy link
Contributor Author

Yes its good to go.

@chrisbra chrisbra closed this in 22ac941 May 23, 2024
clason added a commit to clason/neovim that referenced this pull request May 23, 2024
…ifier none; revert broken highlighting

- fix floating_modifier $mod normal|inverse was being hightlighted as error
  reverting the floating_modifier change from dd83b63
- will currently allow invalid syntax after floating_modifier

fixes: vim/vim#14826
closes: vim/vim#14827

vim/vim@22ac941

Co-authored-by: James Eapen <james.eapen@vai.org>
Co-authored-by: JosefLitos <litosjos@fit.cvut.cz>
clason added a commit to neovim/neovim that referenced this pull request May 24, 2024
…ifier none; revert broken highlighting

- fix floating_modifier $mod normal|inverse was being hightlighted as error
  reverting the floating_modifier change from dd83b63
- will currently allow invalid syntax after floating_modifier

fixes: vim/vim#14826
closes: vim/vim#14827

vim/vim@22ac941

Co-authored-by: James Eapen <james.eapen@vai.org>
Co-authored-by: JosefLitos <litosjos@fit.cvut.cz>
@@ -67,7 +67,7 @@ syn keyword i3ConfigBindKeyword bindsym bindcode contained skipwhite nextgroup=i
syn region i3ConfigModeBlock matchgroup=i3ConfigKeyword start=/mode\ze\( --pango_markup\)\? \([^'" {]\+\|'[^']\+'\|".\+"\)\s\+{$/ end=/^}\zs$/ contained contains=i3ConfigShParam,@i3ConfigStrVar,i3ConfigBindKeyword,i3ConfigComment,i3ConfigParen fold keepend extend

" 4.7 Floating modifier
syn keyword i3ConfigKeyword floating_modifier contained skipwhite nextgroup=i3ConfigVariable,i3ConfigBindModkey
syn match i3ConfigKeyword /^floating_modifier [$0-9A-Za-z]*$/ contains=i3ConfigVariable,i3ConfigBindModkey
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have miscommunicated the changes in i3. The reverted changes in i3 introduce back the issue for which they were made - leading spaces.
We didn't need to put back the start-of-line anchor, but we should have kept in the contained keyword. I wrote the exact changes in the original comment, but a revert was probably not the best way to describe them.

Sorry for the late review.

@JosefLitos
Copy link
Contributor

JosefLitos commented May 24, 2024

For a start I did notice two of the errors are recognized if there are any leading spaces...

The reason leading spaces play a role is the inclusion of ^ in the i3config regex. Otherwise, the bottom two get recognized as a valid syntax too. That is because of the simplified highlighting. Since that could be quite a common mistake to write it that way, a small simple change could be made for detecting the first character ->

i3config:

syn match i3ConfigKeyword /floating_modifier [$A-Z][0-9A-Za-z]*$/ contained contains=i3ConfigVariable,i3ConfigBindModkey

swayconfig:

syn match i3ConfigKeyword /floating_modifier \(none\|[$A-Z][a-zA-Z0-9+]\+ \(normal\|inverse\)\)$/ contained contains=i3ConfigVariable,i3ConfigBindModkey,swayConfigFloatingModifierOpts

(added [$A-Z] before the rest of the key/var match)

@jamespeapen
Copy link
Contributor Author

Ah I did focus more on the revert and the sway suggestion than the i3 diff. Thanks @JosefLitos, and apologies @chrisbra the new pull request should supersede this one.

huangyingw pushed a commit to huangyingw/neovim that referenced this pull request May 31, 2024
…ifier none; revert broken highlighting

- fix floating_modifier $mod normal|inverse was being hightlighted as error
  reverting the floating_modifier change from dd83b63
- will currently allow invalid syntax after floating_modifier

fixes: vim/vim#14826
closes: vim/vim#14827

vim/vim@22ac941

Co-authored-by: James Eapen <james.eapen@vai.org>
Co-authored-by: JosefLitos <litosjos@fit.cvut.cz>
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.

Minor syntax highlighting issue with swayconfig files
3 participants