preservim / tagbar

Vim plugin that displays tags in a window, ordered by scope
https://preservim.github.io/tagbar
Other
6.1k stars 485 forks source link

use g:tagbar_highlight_method as default in tagbar#currenttag and forward search method from :TagbarCurrentTag #803

Closed magras closed 2 years ago

magras commented 2 years ago

Actually, maybe it's better to use tagbar_highlight_method as search method in :TagbarCurrentTag instead of forwarding new argument. What do you think?

raven42 commented 2 years ago

@magras Yes I believe you are correct that we could initialize the default to the configured value for g:tagbar_highlight_method feel free to update this PR with that change as well.

We can keep the ability to pass in a defined value however to offer that flexibility.

Thanks for the contribution!

magras commented 2 years ago

@raven42, I see no easy way to forward {search-method} from :TagbarCurrentTag and change default without modifying tagbar#currenttag. I'm a little bit more reluctant to change the default in tagbar#currenttag because it's a lower level API which used more often. Specifically airline and powerline seems to use tagbar#currenttag with default {search-method} and as they update status with every cursor movement, this change can make a noticeable difference in performance. Overall in my opinion there is little value in the ability to specify {search-method} if it defaults to g:tagbar_highlight_method.

If you are okay with changing the default only in :TagbarCurrentTag and dropping ability to specify {search-method} as an argument to the command, I'll make a new pr.

upd: On the other hand if user set g:tagbar_highlight_method it might be right to change the default in tagbar#currenttag too. I have no strong feeling here because I call :TagbarCurrentTag explicitly, not from the status line. So it's your call.

raven42 commented 2 years ago

I am fine with changing the behavior in tagbar#currenttag() to use the g:tagbar_highlight_method if no search method is specified. The default value for the highlight method is already nearest-stl, so default behavior should be maintained. The nearest-stl and scoped-stl are the slower methods anyways, so from a performance perspective the user only chances to gain performance if they have a nearest value selected for their current g:tagbar_highlight_method setting. Also the function is only called once the cursor is idle for the updatetime setting in vim, so that is tunable if someone is seeing performance issues. That is only likely on very large files with most modern systems.

There is also an argument to be made for consistency. If a user changes the definition for the highlight method, then I would expect they would also want the same behavior to be seen for the TagbarCurrentTag functionality. So it makes sense to use the same search method here too.

Go ahead and update this PR to include using g:tagbar_highlight_method in the tagbar#currenttag() function if none is specified. However if we also leave the option to specify the search method in the TagbarCurrentTag command, that will give enough flexibility either way.

@alerque what are your thoughts?

magras commented 2 years ago

If we are going to make the default configurable, there is a list of all hardcoded defaults that I'm aware of:

raven42 commented 2 years ago

For this PR I would recommend we only modify the tagbar#currenttag instances. That would be specific to the current functionality you are looking to update and is a similar idea behind that function as the highlight method. The other calls could be used for other purposes, so we can leave those alone unless someone has a strong opinion they should follow suit.

alerque commented 2 years ago

@alerque what are your thoughts?

I'm with you fellas. (In other words I haven't had the time to dig into the ramifications either way.)

raven42 commented 2 years ago

Looks good. Thanks for the contribution.