bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

The function tab-bar-auto-width determines which tabs to automatically
resize based on the face applied to each tab's text.  If the face is one
of tab-bar-auto-width-faces, then the tab gets resized.  However, if
either tab-bar-tab-face-function or tab-bar-tab-group-face-function is
set to a function which does not apply one of tab-bar-auto-width-faces,
then the tabs which have a different face are not auto resized.

A real-world example of this issue is in activities.el:

https://github.com/alphapapa/activities.el/issues/76

In the proposed patch, instead of checking each tab's face, we check
that the symbol at the start of each tab keymap matches

(rx bos (or "current-tab" "tab-" "group-"))

Thank you!!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Auto-resize-based-on-keymap-symbol-not-face.patch --]
[-- Type: text/x-diff, Size: 1586 bytes --]

From 4d3f43bfeb0d13a127d161fa9c3bd1737eafe645 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Mon, 1 Jul 2024 13:34:06 -0700
Subject: [PATCH] Auto resize based on keymap symbol, not face

* lisp/tab-bar.el (tab-bar-auto-width-faces): Remove defvar.
(tab-bar-auto-width): Match against symbol-name.
---
 lisp/tab-bar.el | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index edec6543a82..f2034616b06 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1216,12 +1216,6 @@ tab-bar-auto-width-min
 It's not recommended to change this value since with larger values, the
 tab bar might wrap to the second line when it shouldn't.")
 
-(defvar tab-bar-auto-width-faces
-  '( tab-bar-tab tab-bar-tab-inactive
-     tab-bar-tab-ungrouped
-     tab-bar-tab-group-inactive)
-  "Resize tabs only with these faces.")
-
 (defvar tab-bar--auto-width-hash nil
   "Memoization table for `tab-bar-auto-width'.")
 
@@ -1250,8 +1244,8 @@ tab-bar-auto-width
         (width 0))    ;; resize tab names to this width
     (dolist (item items)
       (when (and (eq (nth 1 item) 'menu-item) (stringp (nth 2 item)))
-        (if (memq (get-text-property 0 'face (nth 2 item))
-                  tab-bar-auto-width-faces)
+        (if (string-match-p "\\`\\(?:current-tab\\|\\(?:group\\|tab\\)-\\)"
+                            (symbol-name (nth 0 item)))
             (push item tabs)
           (unless (eq (nth 0 item) 'align-right)
             (setq non-tabs (concat non-tabs (nth 2 item)))))))
-- 
2.41.0

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Juri Linkov]

> The function tab-bar-auto-width determines which tabs to automatically
> resize based on the face applied to each tab's text.  If the face is one
> of tab-bar-auto-width-faces, then the tab gets resized.  However, if
> either tab-bar-tab-face-function or tab-bar-tab-group-face-function is
> set to a function which does not apply one of tab-bar-auto-width-faces,
> then the tabs which have a different face are not auto resized.
>
> A real-world example of this issue is in activities.el:
>
> https://github.com/alphapapa/activities.el/issues/76

Thanks for the request.

Maybe activities.el could add its face to tab-bar-auto-width-faces?

If not, then what about allowing tab-bar-auto-width-faces to have
the value t that means that all tabs should be resized regardless of
what faces they have.

> In the proposed patch, instead of checking each tab's face, we check
> that the symbol at the start of each tab keymap matches
>
> (rx bos (or "current-tab" "tab-" "group-"))

> -(defvar tab-bar-auto-width-faces
> -  '( tab-bar-tab tab-bar-tab-inactive
> -     tab-bar-tab-ungrouped
> -     tab-bar-tab-group-inactive)
> -  "Resize tabs only with these faces.")

Sorry, we can't remove the existing variable to not break user configs.

> @@ -1250,8 +1244,8 @@ tab-bar-auto-width
> -        (if (memq (get-text-property 0 'face (nth 2 item))
> -                  tab-bar-auto-width-faces)
> +        (if (string-match-p "\\`\\(?:current-tab\\|\\(?:group\\|tab\\)-\\)"
> +                            (symbol-name (nth 0 item)))

Matching the symbol name with the hard-coded regexp doesn't look right.
Maybe better to add a new variable that contains a predicate function?
When it returns t then resize.

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Ship Mints]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

I like both ideas. Supporting t seems idiomatic. `activities' could add its
face to the list, or a user in her configuration via documentation.

On Tue, Jul 2, 2024 at 2:59 AM Juri Linkov <juri@linkov.net> wrote:

> > The function tab-bar-auto-width determines which tabs to automatically
> > resize based on the face applied to each tab's text.  If the face is one
> > of tab-bar-auto-width-faces, then the tab gets resized.  However, if
> > either tab-bar-tab-face-function or tab-bar-tab-group-face-function is
> > set to a function which does not apply one of tab-bar-auto-width-faces,
> > then the tabs which have a different face are not auto resized.
> >
> > A real-world example of this issue is in activities.el:
> >
> > https://github.com/alphapapa/activities.el/issues/76
>
> Thanks for the request.
>
> Maybe activities.el could add its face to tab-bar-auto-width-faces?
>
> If not, then what about allowing tab-bar-auto-width-faces to have
> the value t that means that all tabs should be resized regardless of
> what faces they have.
>
> > In the proposed patch, instead of checking each tab's face, we check
> > that the symbol at the start of each tab keymap matches
> >
> > (rx bos (or "current-tab" "tab-" "group-"))
>
> > -(defvar tab-bar-auto-width-faces
> > -  '( tab-bar-tab tab-bar-tab-inactive
> > -     tab-bar-tab-ungrouped
> > -     tab-bar-tab-group-inactive)
> > -  "Resize tabs only with these faces.")
>
> Sorry, we can't remove the existing variable to not break user configs.
>
> > @@ -1250,8 +1244,8 @@ tab-bar-auto-width
> > -        (if (memq (get-text-property 0 'face (nth 2 item))
> > -                  tab-bar-auto-width-faces)
> > +        (if (string-match-p
> "\\`\\(?:current-tab\\|\\(?:group\\|tab\\)-\\)"
> > +                            (symbol-name (nth 0 item)))
>
> Matching the symbol name with the hard-coded regexp doesn't look right.
> Maybe better to add a new variable that contains a predicate function?
> When it returns t then resize.
>

[-- Attachment #2: Type: text/html, Size: 2693 bytes --]

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Joseph Turner via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Juri Linkov <juri@linkov.net> writes:

>> The function tab-bar-auto-width determines which tabs to automatically
>> resize based on the face applied to each tab's text.  If the face is one
>> of tab-bar-auto-width-faces, then the tab gets resized.  However, if
>> either tab-bar-tab-face-function or tab-bar-tab-group-face-function is
>> set to a function which does not apply one of tab-bar-auto-width-faces,
>> then the tabs which have a different face are not auto resized.
>>
>> A real-world example of this issue is in activities.el:
>>
>> https://github.com/alphapapa/activities.el/issues/76
>
> Thanks for the request.
>
> Maybe activities.el could add its face to tab-bar-auto-width-faces?

Unfortunately, this isn't possible.  activities.el sets
tab-bar-tab-face-function to

(defun activities-tabs--tab-bar-tab-face-function (tab)
  "Return a face for TAB.
If TAB represents an activity, face `activities-tabs' is added as
inherited."
  ;; TODO: Propose a tab-bar equivalent of `tab-line-tab-face-functions'.
  (let ((face (funcall activities-tabs-tab-bar-tab-face-function-original tab)))
    (if (activities-tabs--tab-parameter 'activity tab)
        `(:inherit (activities-tabs ,face))
      face)))

so there's no face symbol to match against.

> If not, then what about allowing tab-bar-auto-width-faces to have
> the value t that means that all tabs should be resized regardless of
> what faces they have.

Would you be willing to send a patch with this idea?

>> In the proposed patch, instead of checking each tab's face, we check
>> that the symbol at the start of each tab keymap matches
>>
>> (rx bos (or "current-tab" "tab-" "group-"))
>
>> -(defvar tab-bar-auto-width-faces
>> -  '( tab-bar-tab tab-bar-tab-inactive
>> -     tab-bar-tab-ungrouped
>> -     tab-bar-tab-group-inactive)
>> -  "Resize tabs only with these faces.")
>
> Sorry, we can't remove the existing variable to not break user
> configs.

You're right.

>> @@ -1250,8 +1244,8 @@ tab-bar-auto-width
>> -        (if (memq (get-text-property 0 'face (nth 2 item))
>> -                  tab-bar-auto-width-faces)
>> +        (if (string-match-p "\\`\\(?:current-tab\\|\\(?:group\\|tab\\)-\\)"
>> +                            (symbol-name (nth 0 item)))
>
> Matching the symbol name with the hard-coded regexp doesn't look right.
> Maybe better to add a new variable that contains a predicate function?
> When it returns t then resize.

What would be passed to the predicate function?

Thanks for the review!

Joseph

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Juri Linkov]

>> If not, then what about allowing tab-bar-auto-width-faces to have
>> the value t that means that all tabs should be resized regardless of
>> what faces they have.
>
> Would you be willing to send a patch with this idea?

Probably this is not needed after implementing a variable with
a predicate function, since it could be set to 'always' to return t.

Then activities.el could set this to a function that checks for a symbol.

>>> In the proposed patch, instead of checking each tab's face, we check
>>> that the symbol at the start of each tab keymap matches
>>>
>>> (rx bos (or "current-tab" "tab-" "group-"))
>>
>>> -(defvar tab-bar-auto-width-faces
>>> -  '( tab-bar-tab tab-bar-tab-inactive
>>> -     tab-bar-tab-ungrouped
>>> -     tab-bar-tab-group-inactive)
>>> -  "Resize tabs only with these faces.")
>>
>> Sorry, we can't remove the existing variable to not break user
>> configs.
>
> You're right.

But we could deprecate tab-bar-auto-width-faces in Emacs 30,
and in Emacs 31 replace it with a function that matches a symbol name
like in your patch.  Then users will have time to get the function into use.

>>> @@ -1250,8 +1244,8 @@ tab-bar-auto-width
>>> -        (if (memq (get-text-property 0 'face (nth 2 item))
>>> -                  tab-bar-auto-width-faces)
>>> +        (if (string-match-p "\\`\\(?:current-tab\\|\\(?:group\\|tab\\)-\\)"
>>> +                            (symbol-name (nth 0 item)))
>>
>> Matching the symbol name with the hard-coded regexp doesn't look right.
>> Maybe better to add a new variable that contains a predicate function?
>> When it returns t then resize.
>
> What would be passed to the predicate function?

I think only 'item' should be passed to the function,
there is no other useful information here.

Then in Emacs 30 this function could check for a face name
in (nth 2 item), and in Emacs 31 a symbol name in (nth 0 item).

But only if a symbol name covers all cases currently supported
by the face name.  Let's see with the face->symbol mapping:

  tab-bar-tab -> current-tab
  tab-bar-tab-inactive -> tab-N
  tab-bar-tab-ungrouped -> tab-N, unfortunately there is no separate symbol
  tab-bar-tab-group-inactive -> group-N
  tab-bar-tab-group-current -> there is no current-group, but this could be added:

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index edec6543a82..66fb9490ce8 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -1044,7 +1044,7 @@ tab-bar--format-tab-group
 when the tab is current.  Return the result as a keymap."
   (append
    `((,(intern (format "sep-%i" i)) menu-item ,(tab-bar-separator) ignore))
-   `((,(intern (format "group-%i" i))
+   `((,(intern (if current-p "current-group" (format "group-%i" i)))
       menu-item
       ,(if current-p
            (condition-case nil

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Adam Porter]

Thanks to all for working on this.

On 7/2/24 12:34, Juri Linkov wrote:
> Probably this is not needed after implementing a variable with
> a predicate function, since it could be set to 'always' to return t.
> 
> Then activities.el could set this to a function that checks for a symbol.

If it seems appropriate, I'd suggest using a list of predicate 
functions, which could be used with `run-hook-with-args-until-success'. 
That way there wouldn't be any contention with other libraries which 
also wanted to set that function.

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Juri Linkov]

>> Probably this is not needed after implementing a variable with
>> a predicate function, since it could be set to 'always' to return t.
>> Then activities.el could set this to a function that checks for a symbol.
>
> If it seems appropriate, I'd suggest using a list of predicate functions,
> which could be used with `run-hook-with-args-until-success'. That way there
> wouldn't be any contention with other libraries which also wanted to set
> that function.

Would you agree to use add-function instead?  For example, in tab-bar.el:

  (defvar tab-bar-auto-width-predicate #'tab-bar-auto-width-faces)

Then in activities.el you could use:

  (add-function :after-while tab-bar-auto-width-predicate activities-predicate)

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Adam Porter]

On 7/3/24 01:27, Juri Linkov wrote:
>>> Probably this is not needed after implementing a variable with
>>> a predicate function, since it could be set to 'always' to return t.
>>> Then activities.el could set this to a function that checks for a symbol.
>>
>> If it seems appropriate, I'd suggest using a list of predicate functions,
>> which could be used with `run-hook-with-args-until-success'. That way there
>> wouldn't be any contention with other libraries which also wanted to set
>> that function.
> 
> Would you agree to use add-function instead?  For example, in tab-bar.el:
> 
>    (defvar tab-bar-auto-width-predicate #'tab-bar-auto-width-faces)
> 
> Then in activities.el you could use:
> 
>    (add-function :after-while tab-bar-auto-width-predicate activities-predicate)

Isn't advice generally intended for users to use in their configs, 
rather than for libraries to use?  If we have here an opportunity to 
design an API that is extensible by multiple libraries, wouldn't that be 
preferable to asking downstream libraries to apply multiple levels of 
advice and the problems that would raise?

IOW, what would the problem be with using 
`run-hook-with-args-until-success' on a list of functions?  If there is 
one, I must be missing something.  :)

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Juri Linkov]

>>>> Probably this is not needed after implementing a variable with
>>>> a predicate function, since it could be set to 'always' to return t.
>>>> Then activities.el could set this to a function that checks for a symbol.
>>>
>>> If it seems appropriate, I'd suggest using a list of predicate functions,
>>> which could be used with `run-hook-with-args-until-success'. That way there
>>> wouldn't be any contention with other libraries which also wanted to set
>>> that function.
>> Would you agree to use add-function instead?  For example, in tab-bar.el:
>>    (defvar tab-bar-auto-width-predicate #'tab-bar-auto-width-faces)
>> Then in activities.el you could use:
>>    (add-function :after-while tab-bar-auto-width-predicate
>> activities-predicate)
>
> Isn't advice generally intended for users to use in their configs, rather
> than for libraries to use?  If we have here an opportunity to design an API
> that is extensible by multiple libraries, wouldn't that be preferable to
> asking downstream libraries to apply multiple levels of advice and the
> problems that would raise?
>
> IOW, what would the problem be with using
> `run-hook-with-args-until-success' on a list of functions?  If there is
> one, I must be missing something.  :)

Advice is intended for users and external libraries.
Only in core it should be avoided.

But `run-hook-with-args-until-success' is fine with me too.

Let's see what Joseph and Stephane think.

bug#71883: [PATCH] Fix tab-bar-auto-width with customized tab-bar-tab-face-function

[Ship Mints]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Seems reasonable and thank you for soliciting my input. You're the expert
with tab-bar, I'm just a happy minor contributor. I do have
activities/tab-bar UI features I use and don't want to break. Relevant to
this particular discussion, I rely on activities-tabs face to visually
differentiate activities-controlled tabs (I put a one-pixel box around the
tab).

I do similar things for tabs under other control regimes and have
customized formatters for groups and tabs for both colors and content
(prefixes for group names, prefixes for tab names that have "project"
buffers). And lots of customization around general usage; e.g., you know
about the work I did on optionally collapsing tab group members (Emacs 31!).

-S

On Thu, Jul 4, 2024 at 2:04 PM Juri Linkov <juri@linkov.net> wrote:

> >>>> Probably this is not needed after implementing a variable with
> >>>> a predicate function, since it could be set to 'always' to return t.
> >>>> Then activities.el could set this to a function that checks for a
> symbol.
> >>>
> >>> If it seems appropriate, I'd suggest using a list of predicate
> functions,
> >>> which could be used with `run-hook-with-args-until-success'. That way
> there
> >>> wouldn't be any contention with other libraries which also wanted to
> set
> >>> that function.
> >> Would you agree to use add-function instead?  For example, in
> tab-bar.el:
> >>    (defvar tab-bar-auto-width-predicate #'tab-bar-auto-width-faces)
> >> Then in activities.el you could use:
> >>    (add-function :after-while tab-bar-auto-width-predicate
> >> activities-predicate)
> >
> > Isn't advice generally intended for users to use in their configs, rather
> > than for libraries to use?  If we have here an opportunity to design an
> API
> > that is extensible by multiple libraries, wouldn't that be preferable to
> > asking downstream libraries to apply multiple levels of advice and the
> > problems that would raise?
> >
> > IOW, what would the problem be with using
> > `run-hook-with-args-until-success' on a list of functions?  If there is
> > one, I must be missing something.  :)
>
> Advice is intended for users and external libraries.
> Only in core it should be avoided.
>
> But `run-hook-with-args-until-success' is fine with me too.
>
> Let's see what Joseph and Stephane think.
>

[-- Attachment #2: Type: text/html, Size: 2922 bytes --]