Proper support for third-party statusline plugins

[marwing]

This variable sets whether vim-cmake sets a custom statusline in the terminal window opened by :CMake commands.

This is an implementation of my proposed changes in #46.

I'm currently not too sure about the name of the variable added. Please comment with ideas regarding this or other parts of this change. The name from #46 would create a double negative though and doesn't seem like a good idea.

Feel free to close this if you don't think this change is a good idea since you haven't answered on the issue yet.

[cdelledonne]

I've at last found some time to get to this, apologies for the huge delay.

First of all: thanks a lot for starting this. The proposed changes make sense, I agree that we should give the user the option to disable overriding the statusline option.

I have a few general comments:

1. I think it's pretty common for regular Vim/Neovim users to install some statusline plugin. Given that most of these plugins do not require setting statusline, we should probably set g:cmake_statusline to 0 by default. The majority of people would find that most useful I believe.

2. Regarding naming, I think g:cmake_statusline works. I would just be slightly more precise in the documentation of that option, to emphasize that the option only controls whether Vim-CMake sets the statusline option of Vim/Neovim. E.g.

   Whether to override the |statusline| option for the CMake console window.

Other than that, I think the PR is good for what we need.

BONUS COMMENT: In the latest versions of this plugin, I have tried to define all "external" API functions in autoload/cmake.vim. To preserve compatibility with vim-airline's extension, I had left statusline API functions in autoload/cmake/statusline.vim. However, I think we can still add some structure to the API, while leaving the old statusline functions to not break vim-airline. What I had in mind is:

1. In autoload/cmake/statusline.vim, define two "internal" functions s:statusline.GetBuildInfo() and s:statusline.GetCmdInfo(), which simply return the variables build_info and cmd_info. These two internal functions would be in line with how other components of Vim-CMake expose their interface.

2. The internal components of Vim-CMake would now use this interface to get build and cmd info, so we'd update the setlocal statusline statements in autoload/cmake/terminal.vim to use this interface.

3. In autoload/cmake.vim, define an "external" API function cmake#GetInfo(), which returns a dict containing build_info and cmd_info. One of the advantages of this is that we can easily add other generic info to this dict, e.g. cmake_version, but we can have this in a later PR. Now other plugins or user configurations would use this API, e.g. a configuration for lualine would look like

   local vimcmake_extension = {
      sections = {
          lualine_a = {function() return 'CMake' end},
          lualine_b = {'%{cmake#GetInfo().build_info}'},
          lualine_c = {'%{cmake#GetInfo().cmd_info}'},
      filetypes = {'vimcmake'} ,
   }
   require('lualine').setup{
      extensions = {
          vimcmake_extension,
      },
   }

Feel free to address the bonus comment if you feel like it, but if you don't have the time I can just do that in another PR.

[marwing]

I actually thought about setting g:cmake_statusline to 0 by default before but didn't want to break default behavior in my first pr as I don't know your policy on (if small) breaking changes. Nevertheless I addressed your general comments in the latest commit. Will look at your bonus comment next.

Working on the bonus comment transforms this pr from a 'please don't break my statusline' pr into an actual statusline support pr so we might want to update the title to something similar as your original issue #46 (e.g. Support statusline plugins)

[cdelledonne]

Great, thanks! Renaming the PR sounds good, feel free to either use the title of the original issue or something that seems more appropriate to you.

[marwing]

Looking further into this (and vim-cmake's internals) I have some comments:

1. s:statusline.GetCmdInfo() and s:statusline.GetBuildInfo() already exist (as you probably know) and are not simple getters. Instead both do formatting in some form or another. For a proper statusline support API we maybe should convert both to simple getters and leave the formatting to the consumers of these functions. This however would break the current vim-airline extension for vim-cmake. This could be addressed by creating a pr with vim-airline to update their API use to the then new 'external' API cmake#GetInfo().
2. For the cmake#GetInfo() function I would change the keys in the dictionary to something like current_config or current_build_type and status. build_info and cmd_info are internal names that are hard to understand for people new to vim-cmake's internals (including me). Seeing as cmake#GetInfo() is supposed to be used by third-party consumers more intuitive names may be appropriate.

What are your takes on these comments? Anything you would change/not do about this?

BTW, lualine.nvim has recently (in this commit) fixed my original problem by resetting itself as the statusline (based on timers and filetype I believe... Their commits are hard to understand). Disabling the internal statusline override is still a good feature though as lualine was obviously not the only statusline plugin out there that suffered from this problem.

[cdelledonne]

Regarding 1.: The current getters cmake#statusline#GetBuildInfo() and cmake#statusline#GetCmdInfo() are external functions. My suggestion was to introduce some internal simple getters, named as per your comment and that indeed return the unformatted info. For backwards compatibility with vim-airline, I would also suggest to not remove the current external getters, and to submit a PR to vim-airline later. In any case, the old external getters should not be removed immediately after submitting the PR to vim-airline, in case one uses an old version of vim-airline but a new version of vim-cmake (unlikely I guess, but not impossible).

Regarding 2.: Makes sense. I like current_config (matches the notion of "config" mentioned in the docs) and status.

One more thing that came to my mind: I think we should also document the usage of cmake#GetInfo() (shortly in the README, and a little more in depth in the docs), so that people are aware of its existence. What do you think?

By the way, I've also switched to lualine, it had been on my to-do list for a while, but had never found the time. For now I'm defining a custom extension that I pass to lualine.setup(), but later on we could consider submitting a PR to lualine to support vim-cmake out of the box.

[marwing]

1. This definitely makes sense. So s:statusline.GetCmdInfo() and s:statusline.GetBuildInfo() as internal getters and cmake#statusline#GetBuildInfo() and cmake#statusline#GetCmdInfo() as the existing external ones? I totally missed they were two different things in VimScript.
2. Will do.

Regarding documentation for cmake#GetInfo() this is definitely a good idea (I sort of took that as a given) especially since it's supposed to be the public interface.

I myself have since switched away from lualine to a way more customizable statusline plugin called heirline.nvim (or actually still am in the process, this plugin takes lots of time to do right). Anyway a PR to lualine is of course a good thing to consider, however the problem with that is that not everyone likes the same configuration (as can be seen when comparing our lualine vim-cmake extensions) but I guess this is the reason I moved on from lualine. For lualine this is not too much of a problem because the philosophy is more focused on good defaults than total customizability.

[cdelledonne]

So s:statusline.GetCmdInfo() and s:statusline.GetBuildInfo() as internal getters and cmake#statusline#GetBuildInfo() and cmake#statusline#GetCmdInfo() as the existing external ones?

Yes, that should do.

[marwing]

Using the new internal api for vim-cmake's statusline poses a problem because the statusline itself doesn't have access to script local variables. Any reason not to use the public api? E.g.:

setlocal statusline=[CMake]
setlocal statusline+=\ [%{cmake#GetInfo().current_config}]
setlocal statusline+=\ %{cmake#GetInfo().status}

instead of

setlocal statusline=[CMake]
setlocal statusline+=\ [%{cmake#statusline#Get().GetBuildInfo()}]
setlocal statusline+=\ %{cmake#statusline#Get().GetCmdInfo()}

This looks nicer and gives a practical example for the usage of this api.

Also I noticed you already have s:buildsys.GetCurrentConfig() in addition to s:statusline.GetBuildInfo(). Any reason to have this twice instead of just calling into s:buildsys.GetCurrentConfig() where necessary?

[cdelledonne]

Yes indeed, we need to pass public functions to the statusline option. Using the new API should be fine and looks nicer too, agreed.

Regarding your second comment, GetBuildInfo() is effectively redundant, because vim-cmake has access to the internal s:buildsys.GetCurrentConfig(), and external components have access to the public API. I guess we could skip GetBuildInfo() and thus also remove SetBuildInfo(), and make all external components use the public API. What do you think?

For consistency, GetCmdInfo() and SetCmdInfo() could be removed as well, and we could add an internal function to terminal.vim to retrieve what we now call the "status". In which case statusline.vim would just contain the deprecated API used by vim-airline and the Refresh() function.

[marwing]

Having everything external use the public API is a good idea as this reduces interlocking between modules/files just to get information. Especially since the current setup is effectively 'pushing' the information to the statusline module even when not necessary instead of being a pull configuration. The GetInfo() function is the only one that has to know how to query information from all modules we want to provide information from. (This also makes it easy to extend the available information.)

In the end I would only keep statusline.vim to host the (deprecated?) statusline functions as long as the airline extension has not been updated (with a transition period). As long as it still exists this too should be updated to use the public API to get its information effectively slimming down the whole file to a two deprecated functions and the refresh code.

Also I would change the name of current_config to just config as the API by design only returns current information.

[cdelledonne]

Agreed very much

[marwing]

Docs will follow when we have the rest figured out. For terminal.vim I named the variable cmd_info as this is what it's called in the other location in the file. This can be changed later. We now might want to consider what other information we want to provide. Interesting would probably be the cmake version, the project root directory and the path to the current config directory. The cmake version to display to the user and the directories to display and for other plugins or user functions to use. What do you think?

[cdelledonne]

That is indeed what I had in mind, I was just not sure you wanted to address this in this PR. But given that you brought it up it'd be great if you could include these in this PR :).

[marwing]

At least the low hanging fruit I want to address in this PR, anything further can be added later. Also, do we actually need getters instead of just reading the variables directly? As far as I can see there is no real technical reason why we couldn't. VimScript is definitely not my strong suit though so I may well be missing something.

[cdelledonne]

As far as I know, there's no technical reason that would prevent you from accessing the variables directly, since they are just entries of the dictionaries returned by the Get() functions. Conceptually, I was trying to emulate access control for classes, where I would typically declare data members as private and define public getters when necessary. Even if all "data members" of our dictionaries are effectively public, I think it's nice to indicate which members are meant to be read by other "classes" by defining a getter for such members.

[marwing]

But isn't the concept of a private 'data member' a script variable (s:)? These can be accessed by everything in the 'class' but not from the outside. I was just asking because I would have to define some getters at least for the cmake version and status and having to write 3 lines accessor and 6 lines doc comment just to read a variable intended to be read from another file in the same project seems excessive. If that's what you want to do though I won't mess with that, this is your project after all...

Also I cleaned up cmake version detection and storage to be usable by both the s:buildsys.Generate() function and for public consumption by storing a dict with the version parts (major, minor, patch) instead of the precomputed value and computing that when necessary.

[cdelledonne]

just to read a variable intended to be read from another file in the same project

Yes, it's just one project, but I am looking at the various components of the project (buildsys, terminal, etc.) as separate "classes", each of which has its "private data members" and its "public interface". These interfaces are internal to the project, yes, but there are still boundaries between the internal components. I'd like to keep this as is, preferably.

One could argue that writing documentation for internal getters is a bit of an overkill, and I might agree with that, however Vimscript function definitions make it hard to understand what type of value functions return, and what/how many arguments they take. Thus a bit of verbosity can help other contributors. That's perhaps a matter of preference, but I'd also like to keep it this way.

Also I cleaned up cmake version detection and storage to be usable by both the s:buildsys.Generate() function and for public consumption by storing a dict with the version parts (major, minor, patch) instead of the precomputed value and computing that when necessary.

This was needed indeed, thanks.

[marwing]

Writing docs for the functions seems right, my concern was about writing them in the first place. I can see where your coming from though so getters it is. I will do documentation now. Anything you would change?

[cdelledonne]

Looks great! Just left a couple of minor comments, other than that you can move on to documentation.

[marwing]

Addressed your comments, added documentation and rebased on master. If you don't want to change anything this is ready to be merged now.

[cdelledonne]

Looks great. Thanks a lot for this, it was very much needed :)