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: Ranged comment signs not showing #305

Conversation

jakubbortlik
Copy link
Collaborator

Addresses #302.

This PR also makes it possible to hide the diagnostic sign (e.g., I if the diagnostic level is "INFO") - this makes the diagnostic range icons better aligned. The default behaviour of the plugin does not change, i.e., the diagnostic signs are shown.

I've noticed one small bug that existed already with nvim 0.9, if some signs overlap, the GitlabComment icons are hidden by the GitlabRange icons the first time the signs are placed. This is because for some reason, this line in lua/gitlab/indicators/signs.lua:33 does not return any already placed signs:

    local existing_signs =
      vim.fn.sign_getplaced(vim.api.nvim_get_current_buf(), { group = discussion_sign_name })[1].signs

On subsequent runs (e.g., when jumping to a diagnostic or when switching files in the review), the overlapping signs are placed correctly and the GitlabComment sign takes precedence. If I found the reason for this behaviour I would like to fix it in this PR, but I haven't been successful so far. Maybe it has to do with calling a vim function inside a lua script.

Regarding the small modifications of the documentation, there are still some inconsistencies between the default setup settings and the ones described in the README and docs (e.g., skip_old_revision_discussion), but right now I don't have the time to fix those. Have you considered keeping the settings in let's say just two places? The README could maybe only contain a link to the docs. It would be easier to maintain consistency.

Despite the signature of `vim.fn.sign_place`, the sign id should be a number.
This makes the ranged diadnostics look a little nicer.
@jakubbortlik jakubbortlik force-pushed the fix-ranged-comment-signs-not-showing branch from a4b0853 to 9a5eb5a Compare June 1, 2024 16:55
@jakubbortlik
Copy link
Collaborator Author

Maybe the issue with incorrect signs could be solved by using the nvim_buf_set_extmark function instead of the sign_place function that is called "legacy" in the news for nvim 0.10.

@harrisoncramer harrisoncramer merged commit 75e5536 into harrisoncramer:develop Jun 10, 2024
6 checks passed
@harrisoncramer
Copy link
Owner

The remark about linking to the settings block is a great idea. I'll add an MR to do that.

@jakubbortlik jakubbortlik deleted the fix-ranged-comment-signs-not-showing branch September 6, 2024 05:46
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.

2 participants