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

runtime(sway + i3): Allow leading spaces for sway keywords #14753

Closed
wants to merge 4 commits into from

Conversation

jamespeapen
Copy link
Contributor

Solution for #14752
It was not just bindsym that was affected - any keywords or keyword matches with leading spaces were considered errors if they were not surrounded by {} in a multiline block.

The only solution I could find was to revert the introduction of the i3/swayConfigBlockOrphan group. However, since that swayConfigBlockOrphan is defined multiple times it may be better to remove them and rewrite with less repetition if necessary. I haven't seen the use case where that is necessary in #14544 so @JosefLitos could you show where that's needed?

Taking out the swayConfigBlockOrphan does not fix the original issue. I think part of the error is that may be because the i3ConfigTopLevelDirective was not allowing spaces. I thought skipwhite would allow that, but apparently it doesn't.

Allowing leading spaces for keywords fixes the original issue, but causes the same issue for commented lines with leading spaces. This necessitated a fix for the i3Comment keyword, removing leading spaces from its definition as they are captured by the i3ConfigTopLevelDirective.

@hiqua for i3 review

We can reintroduce it once the need is properly established.
This will break comments with leading spaces since i3ConfigComment
explicitly sets leading spaces and doesn't work with this fix
fix: Allows comments with leading spaces showing up as errors from
previous commit. Makes it more consistent with other match definitions.
The previous commit allowed leading spaces for i3ConfigTopLevelDirective,
but broke it for comments.
The previous fix for keywords with leading spaces doesn't apply to these
as these are matches rather than keywords. They need the explicit
setting of 0 or more leading spaces so that they are correctly parsed
when indented.
@JosefLitos
Copy link
Contributor

JosefLitos commented May 12, 2024

The issue with the Orphan blocks is that normal commands are considered a keybind name. I narrowed down the match even further - modifiers or special keys (starting with capital), or a simple char (one letter, usually used in custom modes).

The reason for Orphan blocks is that, when you have a bind-block starting before the first displayed line, neovim (I would guess also vim) doesn't look before what is being displayed. Therefore the displayed bindings of that block would be highlighted as errors.

I found some docs on how to make a special match for detecting required context or something, but this seems to be more performant.

@jamespeapen
Copy link
Contributor Author

Before #14757 is merged would you mind showing an example of the three orphan blocks in action?
Do you mean the first line is a block?

@hiqua
Copy link
Contributor

hiqua commented May 12, 2024

I'll wait for you folks to agree on something before I start reviewing anything, if that's fine for you!

@JosefLitos
Copy link
Contributor

JosefLitos commented May 12, 2024

... would you mind showing an example of the three orphan blocks in action?

obrazek

Do you mean the first line is a block?

I am not sure I'm using the right terms but as an example take the first picture of the bindsym orphans, this is the initial block part, that vim needs to recognize the entire block:
obrazek
As you can see, at this point the orphan highlighting isn't in use, because the bindsym block is correctly recognized. But without the keyword (the line being outside the displayed text when scrolling from the bottom up), it cannot match the block, therefore the Orphan match is used to guess and highlight the lines that look like the contents of a bindsym block.


It works the same way with bindgesture and bindswitch, though probably most people won't have such long blocks of bindings for these two, making them unnecessary, but just for the sake of completeness and uniformity I added Orphan matches for them too.

I know this is kind of hacky, but I couldn't think of a better way. But I would like to also try some other way at some point - at least to see and compare.

Edit: Equivalent examples for the other bind modes:
obrazek
obrazek

@jamespeapen
Copy link
Contributor Author

I see - not orphans in the file itself, but orphan based on the current buffer view. Thanks for explaining and for the fix!
Closing in favor of #14757.

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.

None yet

3 participants