feat(diagnostics): focus preview window when invoking commands again with current argument

[rywng]

Update: updated the description according mrcjkb's newest requirements.

As the title said, running the commands twice will focus the pop up instead of closing the hover and opening it again.

According to neovim's documentation for
vim.lsp.util.open_floating_preview:

If true, and if {focusable} is also true, focus an existing floating
window with the same {focus_id}
(default: true)

Removing the focus=false, and removing call to close_hover enable users
to enter the hover.

I removed the your parameter focus=false, so that nvim will use the default parameter focus=true, then when calling the open_floating_preview twice, it would focus an existing floating window with the same {focus_id}.

If you have any other questions or understanding the code, please run :h vim.lsp.util.open_floating_preview() or check out runtime/lua/vim/lsp/util.lua:1520 in neovim's source code for their implementation.

[github-actions]

Review Checklist

Does this PR follow the Contribution Guidelines? Following is a partial checklist:

Proper conventional commit scoping:

• For example, fix(lsp): some lsp-related bugfix

• Pull request title has the appropriate conventional commit prefix.

If applicable:

• Tested
  • Tests have been added.
  • Tested manually (Steps to reproduce in PR description).
• Updated documentation.

[rywng]

Tested my changes manually, please refer to this asciinema cast:

[mrcjkb]

Thanks 🙏

• I generally avoid breaking changes until I bump the minimum required Nvim version. This will have to be kept backward compatible for now.
  If something is to be removed with the next major version bump and it has an alternative, it can be marked as deprecated with vim.deprecate.
• The diff appears to only show deletions. Did something not get staged?
• Note (for when/if command arguments are removed): The commit prefix would have to be feat! , not refactor. A refactor does not change behaviour or features. ! indicates a breaking change. See https://conventionalcommits.org.

[rywng]

Thanks 🙏

* I generally avoid breaking changes until I bump the minimum required Nvim version. This will have to be kept backward compatible for now.
  If something is to be removed with the next major version bump and it has an alternative, it can be marked as deprecated with `vim.deprecate`.

* The diff appears to only show deletions. Did something not get staged?

* Note (for when/if command arguments are removed): The commit prefix would have to be `feat!` , not `refactor`. A refactor does not change behaviour or features. `!` indicates a breaking change. See https://conventionalcommits.org.

1. Appreciate the info, I will look into vim.deprecate
2. No, everything is staged, please see the commit logs for more info (you can also run it and test it locally)
3. Thanks for the correction (I was wondering about that for a while haha), I will update the commit log when I have time.

[rywng]

The commit logs have been amended, but for the deprecation part, If you would permit, I can open up another PR adding these warnings to users.

My rationale being that it would be easier to merge this one once you bump the major version. ( but I don't have much experience with nvim plugin's deprecation process, please correct me if I'm wrong)

[mrcjkb]

The commit logs have been amended, but for the deprecation part, If you would permit, I can open up another PR adding these warnings to users.

My rationale being that it would be easier to merge this one once you bump the major version. ( but I don't have much experience with nvim plugin's deprecation process, please correct me if I'm wrong)

Sorry if I wasn't clear in my last comment.
I am requesting a rework, because I do not want to introduce breaking changes to this plugin right now. Please do not remove the existing arguments, and ensure this PR only adds behaviour, while keeping the existing behaviour intact.
We can add deprecations and removals later on if necessary.

[rywng]

Sorry for the misunderstanding, and taking your time to explain the process.

I have updated the PR according to your specifications, linted according to your contribution guidelines, and tested manually since busted doesn't work on my machine.

(also I hope it's not introducing any complexities to your code since it basically changed some parameters back to default, and moved two function calls around)

[rywng]

This would cover my use case (thanks in advance!), and for the deprecation and remove part, I can work on it later.

I personally would like to remove that argument because:

• the function caused confusion to me (and other users as noted in the issues we've already mentioned)
• the code underneath has a lot of duplication to the "dont cycle" variant, which bloats up the codebase

[mrcjkb]

The current diff only shows deletions plus two close_hover() additions.
That can't be right.

Regarding the commands, I would suggest three arguments: prev, next and current. For now, cycle can just be an alias for next. I don't see how that would cause much code duplication.

Also: From what I can tell from this PRs description, you appear to be working on two different features:

• prev/next arguments
• Focus the current diagnostic on repeated command invocation.

Please split them into separate PRs, with appropriate titles (so they are easier to grok and so that each feature gets an entry in the changelog).

The focus feature is not as simple as what I can see in the current diff. If called with a prev or next argument, the current float window should definitely be closed. Only when called with the current argument, should it be focused if already open.
The current default behaviour without any arguments is to cycle. That should remain the same for now. We can change that when I bump the minimal required Nvim version.

[rywng]

The current diff only shows deletions plus two close_hover() additions. That can't be right.

This IS right, as I have already stated many times before, please see the commit message:

According to neovim's documentation for
vim.lsp.util.open_floating_preview:

> If `true`, and if {focusable} is also `true`, focus an existing floating
> window with the same {focus_id}
> (default: `true`)
> https://github.com/field focus? boolean

Removing the focus=false, and removing call to close_hover enable users
to enter the hover.

I removed the your parameter focus=false, so that nvim will use the default parameter focus=true, then according to my commit message I've attached above, by calling the open_floating_preview twice, it would focus an existing floating window with the same {focus_id}. If you have any other questions or understanding the code, please run :h vim.lsp.util.open_floating_preview() or check out runtime/lua/vim/lsp/util.lua:1520 in neovim's source code for their implementation.

I have to admit I am new to the nvim plugin scene, however, I submitted this PR with good faith with the goal of making it more consistent with current nvim lsp integration. As I have stated before, if you don't trust me, you can test it your self.

Regarding the commands, I would suggest three arguments: prev, next and current. For now, cycle can just be an alias for next. I don't see how that would cause much code duplication.
Also: From what I can tell from this PRs description, you appear to be working on two different features:
prev/next arguments
Focus the current diagnostic on repeated command invocation.
Please split them into separate PRs, with appropriate titles (so they are easier to grok and so that each feature gets an entry in the changelog).

My initial PR is indeed doing this, however since you said:

Sorry if I wasn't clear in my last comment.
I am requesting a rework, because I do not want to introduce breaking changes to this plugin right now. Please do not remove the existing arguments, and ensure this PR only adds behaviour, while keeping the existing behaviour intact.
We can add deprecations and removals later on if necessary.

I have changed it to only include the focus feature I have mentioned, with NO changes to any arguments.

Also, as I have stated above, I think the cycle argument is un-necessary, (Please see my comment above), and I can help you add a obselete message, and clean up the code. However, I have no intention on adding more stuff.

[mrcjkb]

You stated, "Breaking changes to user-side and technical info are documented in my commit messages.".
I did not deduce from that that you were expecting me to click on the commits to find extra info hidden in the message body. Why not make things easier for reviewers and put that info in the PR description?
If all you're adding in this PR is the ability to focus on repeated invocation of the command, then why was that not clear in the PR title?
The wording, "improve navigation" led me to believe you had intended to add the ability to cycle in the opposite direction (something you had requested in #603 ). Hence my questioning about whether you could have forgotten to check in some changes.

Please keep in mind that I maintain this plugin (which includes reviewing PRs) in my free time.
Getting offended/defensive is, quite frankly, a waste of my free time. Let's keep things professional please.

I am quite busy right now and will test your PR later.

[rywng]

You stated, "Breaking changes to user-side and technical info are documented in my commit messages.".
I did not deduce from that that you were expecting me to click on the commits to find extra info hidden in the message body

First of all, I'm extremely sorry for the misunderstanding and blaming you because I honestly thought you are ignoring my commit messages.

Why not make things easier for reviewers and put that info in the PR description?

Because 1. it's already explained in the commit, and 2. github PR description is not embedded in git, so someone cloning or forking the git repository is unable to see the PR description attached to the commits.

However, I see this is a personal perference, so from now on for any future PRs I will also attach description in PR.

If all you're adding in this PR is the ability to focus on repeated invocation of the command, then why was that not clear in the PR title?
The wording, "improve navigation" led me to believe you had intended to add the ability to cycle in the opposite direction (something you had requested in #603 ). Hence my questioning about whether you could have forgotten to check in some changes.

Well, the title was my fault, I'm not a native English speaker and since the PR was supposed to have other stuff, which you rejected, and I updated the code but forgot to update it led me to do this, I hope the current title is descriptive enough. :)

Please keep in mind that I maintain this plugin (which includes reviewing PRs) in my free time.

I really appreciate your effort, and I had been a happy user for months, which led me wanting to contribute to your repo :).

Getting offended/defensive is, quite frankly, a waste of my free time. Let's keep things professional please.

Well that was caused by our misunderstanding, and I believe I have hopefully kept my language direct and plain. In the future I will take it in mind and try my best to keep my language concise.

I am quite busy right now and will test your PR later.

Thank you for taking your time to maintain this and explaining your thoughts to me. I apologize again for taking up your free time, and please take the review slow, I'm not in a hurry.

[mrcjkb]

Gave it a test drive and it seems to be working nicely.
Thanks for the contribution 🙏