Add auto hide tabs feature to tabs layer

[dalanicolai]

Ideally it would be great to implement tabs switch with C-tab (or C-}) that
shows the tabs as long as C is pressed. However Emacs does not support key
release events so this function uses a timer as a workaround.

To me Emacs looks better when tabs are hidden, and also packages like
pdf-continuous-scroll-mode break when tabs are shown.

[lebensterben]

I think this is better to be merged to upstream.

Also, you need to use the naming convention, public function should start with "spacemacs/"
So it becomes "spacemacs/tabs-forward".
Private functions has "spacemacs//" prefix.

[dalanicolai]

I agree it would be better to merge this upstream, and I proposed this before (incl a code example), but I was asked to implement the code myself. However, that was a little more tricky than just adding it as a Spacemacs feature. Therefore I simply added this code to my private centaur-tabs layer and sending this PR now was trivial.

Anyway, I have created a PR upstream now. Let's see what is the reaction.

[smile13241324]

Thats a nice addition @dalanicolai and that's why I also think this is something to be implemented on the package level to benefit the entire emacs community and not only us.

If your fix is not going to get merged upstream I would consider merging this feature into the layer, but I would like to wait till there is a definitive answer from upstream.

[smile13241324]

Just saw that you pushed a commit to your branch, given that your PR is now waiting for 22 days in centaur-tabs-repo please give me a hint whether your PR is finished. In this case I would do a review and merge it into the layer @dalanicolai.

[dalanicolai]

As far as I remember this PR was finished and ready for merge.

However, I also found the draft in #13995, to implement tab functionality in Spacemacs core. Of course this PR could still get merged so users can test/review it. (And possibly get implemented later in Spacemacs core or separate pacakge/layer?)

Additionally I created another PR (#14287) to add C-tab buffer switch functionality. That functionality does not yet implement the auto-hide tab feature (but both functionalities can perfectly be installed together).
Personally I think I prefer the C-tab way of buffer switching as it automatically 'orders' the buffers and I don't think the tabs add much. Otherwise, if the tabs get implemented I think it would be nice if it automatically switched tabs in the order like in the C-tab switching (i.e. in order of last visited buffer). I hope you get what I mean from this minimal description, otherwise just tell me to clarify a little more.

[lebensterben]

You can simplify the cond form by using pcase instead.

(pcase arg
  ('backward (centaur-tabs-backward))
  ;; etc
  )

Also, you probably can wrap the pcase from in a let form, and don't need to set the value of centaur-tabs-local-mode:

(let ((centaur-tabs-local-mode 1))
  (pcase arg
    ;; etc
  ))

But this assumes that centaur-tabs-local-mode is off by default. I'm not sure about this but usually this should be true.

[dalanicolai]

Thanks for the review and the suggestions. Nice to get reminded about the pcase, I probably will remember its existence this time.

But I do not understand the suggestion about setting the centaur-tabs-local-mode 1 in the let form. I don't remember exactly why I set this, but it looks that I just want to enable the mode, i.e. hide the tabs, before switching the buffer. Probably so that the tabs will be hidden when you switch back to it via e.g. SPC b b. This setting should be persistent, instead of enabled only during evaluation of the let form.

[lebensterben]

Please see my comments.

[dalanicolai]

So I have updated this PR with the pcase suggestion. Also I noticed that in my last previous force-push, I accidentally replaced the version that implemented the spacemacs conventions, to the older one that di not have them implemented (I probably forgot that I switched from my Ubuntu to Fedora installation, so I did not sync before the push). Anyway, I have now pushed the version including that follows the spacemacs conventions, and I think this should be ready for merge.

As I mentioned in my previous comment, I do not use this functionality myself. I prefer to use the switch by C-tab functionality from #14287 as ivy orders the buffers nicely on last-visited. However, both these functionalities can be used together (also they use different default keybindings). The auto-hide functionality could be added to #14287 also, but I think show/hide tabs does not add much in that case...

[lebensterben]

I think this PR is very well done.
There's only one minor issue, that there are two defcustom forms.
Currently, we always use defvar when declaring layer variables or making wrapper variables in layers.

@smile13241324 Is there any policy on defcustom usages?

[smile13241324]

Hmm good question, I think not, basically it seems it was never used within spacemacs context, I'll ask the others.
I will have a look at this PR in the next few days, I think we have waited long enough for the upstream package.

[smile13241324]

Turns out defcustom is even prefered to defvar in modern lisp :), so feel free to use it for layer variables.

[lebensterben]

Turns out defcustom is even prefered to defvar in modern lisp :), so feel free to use it for layer variables.

Yes it is!

We then need to redeclare all those layer variables with defcustom.
We need to have a new policy for the :group field for defcustom forms.
And I suggest to ask contributors to use defcustom in contributing guides.

Well, the only one concern is if these variables are declared as defcustom, they would be appended to (defun dotspacemacs/emacs-custom-settings() ...) form, and we will need to update the core library for this.

[smile13241324]

Just reviewed your PR and found two smaller issues you should fix before this can be merged.

[smile13241324]

No batch processing in funcs files please. This needs to be wrapped into a defun or into a specific init function. Otherwise it will cause side effects during the multi-phase loading process which expects to only find side effect free function definitions.

[dalanicolai]

Thanks again guys for the useful feedback! So I have implemented the requested changes. Also, to keep it consistent I have converted all defvars in this layer into defcustoms.

BUT!!! I have noticed that those variables are only created to set centaur-tabs after the :config keyword in packages.el. So now these variables appear under both names/groups in the customize menu (tabs + centaur-tabs), which is a bad situation.

To solve this I think either this layer should be renamed to centaur-tabs, so that the layer can just refer to set the package variables (without naming conflict) that already use the centaur-tabs group name in the customize menu. Or we keep the name tabs for this layer and then create the layer variables using defvaralias. (I only noticed this after I had changed the defvars to defcustoms).

Let me know which approach has your preference.

[dalanicolai]

Okay, I have just created the aliases (to see if/how it works out). I think this has my preference. Also, I have removed most of the original defvar (now defvaralias) docstrings, as they were exact copies of the original centaur-tabs docstring which automatically apply to its aliases.
Also I have set in the original layer variable's values after the :config keyword in packages.el.

I guess this is the recommended way to do it; put the defvaralias in config.el and then set their values in packages.el. Otherwise let me know how to change it.

I hope, regarding #13995, that this work was useful.

[smile13241324]

Looks good now.