bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Juri Linkov-2
> I noticed that with the latest master branch setting tab-bar-show
> to "1" does not work work for new frames. On those the tabs are shown
> even if tab-bar-show is set to 1.

Thanks for finding a case that is still unhandled.

> I suppose a hook is needed which applies the correct setting
> to the new frame?

Generally, Emacs core packages should avoid adding own code
to hooks, because hooks are intended mostly for users, such as
for example, configuring to enable tab-bar selectively:

  (add-hook 'after-make-frame-functions 'toggle-frame-tab-bar)

Fortunately, frames provide a better way to set their default values
with default-frame-alist, that tab-bar-mode already modifies.
So doing something similar fixes the problem:


diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 6720d82b47..0cf74a7833 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -252,6 +252,12 @@ tab-bar-show
          (set-default sym val)
          ;; Preload button images
          (tab-bar-mode 1)
+         ;; New frames have only one tab, so hide it by default
+         (when (eq val 1)
+           (setq default-frame-alist
+              (cons (cons 'tab-bar-lines 0)
+                    (assq-delete-all 'tab-bar-lines
+                                     default-frame-alist))))
          ;; Then handle each frame individually
          (dolist (frame (frame-list))
            (set-frame-parameter
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hey Juri,

On Fri, Feb 5, 2021 at 10:22 AM Juri Linkov <[hidden email]> wrote:

>
> > I noticed that with the latest master branch setting tab-bar-show
> > to "1" does not work work for new frames. On those the tabs are shown
> > even if tab-bar-show is set to 1.
>
> Thanks for finding a case that is still unhandled.
>
> > I suppose a hook is needed which applies the correct setting
> > to the new frame?
>
> Generally, Emacs core packages should avoid adding own code
> to hooks, because hooks are intended mostly for users, such as
> for example, configuring to enable tab-bar selectively:
>
>   (add-hook 'after-make-frame-functions 'toggle-frame-tab-bar)
>
> Fortunately, frames provide a better way to set their default values
> with default-frame-alist, that tab-bar-mode already modifies.

Oh I see. Yes that would also work. Although I would say that in
general it should be more robust to have a dynamic function which
counts the numbers of tabs and adapts the number of tab-bar-lines
according to the value of tab-bar show. Is it guaranteed that new
frames only have one tab?

> So doing something similar fixes the problem:

That patch looks fine, except that my bug report translates equally to
the case when tab-bar-show is nil, so (eq val 1) should be adapted to
catch both "1" and "nil".

Thanks for your help!
Bastian



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Juri,

I now installed your patch and I don't think it is complete yet.

1) Is the :set function actually used the next time emacs starts after
customizations have been written to .emacs and variables are
initialized to customized values using (custom-set-variables ...)? I
don't think it is, right?

2) Switching tab-bar-mode on and off seems to overwrite the
tab-bar-lines information in default-frame-alist:

    ;; If the user has given `default-frame-alist' a `tab-bar-lines'
    ;; parameter, replace it.
    (if (assq 'tab-bar-lines default-frame-alist)
        (setq default-frame-alist
              (cons (cons 'tab-bar-lines val)
                    (assq-delete-all 'tab-bar-lines
                                     default-frame-alist)))))

This code should depend on the value of tab-bar-show, right?

Cheers
Bastian



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Juri,

I added code to make the frame setting of tab-bar-lines as well as the
default-frame-alist value dependent on tab-bar-show to the
tab-bar-mode function.
I think with this the part to which sets frame parameters in
tab-bar-show :set is not needed because (tab-bar-mode 1) is called
anyway, which already does everything.

What do you think about the attached patch?

Cheers
Bastian

On Fri, Feb 5, 2021 at 3:11 PM Bastian Beranek
<[hidden email]> wrote:

>
> Hello Juri,
>
> I now installed your patch and I don't think it is complete yet.
>
> 1) Is the :set function actually used the next time emacs starts after
> customizations have been written to .emacs and variables are
> initialized to customized values using (custom-set-variables ...)? I
> don't think it is, right?
>
> 2) Switching tab-bar-mode on and off seems to overwrite the
> tab-bar-lines information in default-frame-alist:
>
>     ;; If the user has given `default-frame-alist' a `tab-bar-lines'
>     ;; parameter, replace it.
>     (if (assq 'tab-bar-lines default-frame-alist)
>         (setq default-frame-alist
>               (cons (cons 'tab-bar-lines val)
>                     (assq-delete-all 'tab-bar-lines
>                                      default-frame-alist)))))
>
> This code should depend on the value of tab-bar-show, right?
>
> Cheers
> Bastian

tab-bar.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Juri Linkov-2
> I added code to make the frame setting of tab-bar-lines as well as the
> default-frame-alist value dependent on tab-bar-show to the
> tab-bar-mode function.
> I think with this the part to which sets frame parameters in
> tab-bar-show :set is not needed because (tab-bar-mode 1) is called
> anyway, which already does everything.
>
> What do you think about the attached patch?

I think your earlier idea was better - to have a dynamic function which
counts the numbers of tabs and adapts the number of tab-bar-lines
according to the value of tab-bar-show.

Then such a function could be called from many places:
- tab-bar-mode
- tab-bar-show :set
- from the end of tab-bar-new-tab-to
- from the end of tab-bar-close-tab
- from the end of tab-bar-close-other-tabs

to sync the actual tabs with the value of tab-bar-show.



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Juri,

On Sun, Feb 7, 2021 at 8:48 PM Juri Linkov <[hidden email]> wrote:

>
> > I added code to make the frame setting of tab-bar-lines as well as the
> > default-frame-alist value dependent on tab-bar-show to the
> > tab-bar-mode function.
> > I think with this the part to which sets frame parameters in
> > tab-bar-show :set is not needed because (tab-bar-mode 1) is called
> > anyway, which already does everything.
> >
> > What do you think about the attached patch?
>
> I think your earlier idea was better - to have a dynamic function which
> counts the numbers of tabs and adapts the number of tab-bar-lines
> according to the value of tab-bar-show.
>
> Then such a function could be called from many places:
> - tab-bar-mode
> - tab-bar-show :set
> - from the end of tab-bar-new-tab-to
> - from the end of tab-bar-close-tab
> - from the end of tab-bar-close-other-tabs
>
> to sync the actual tabs with the value of tab-bar-show.
Makes sense! How about the attached?

Cheers
Bastian

tab-bar_v2.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Juri,

I have tried to clean up my patch a bit more, please see the attached version 3.

Best
Bastian

tab-bar_v3.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Juri Linkov-2
Hello Bastian,

> I have tried to clean up my patch a bit more, please see the attached version 3.

Thanks, everything looks right, except one thing that you removed
the most important rule:

> -    (cond
> -     ((eq tab-bar-show t)
> -      (tab-bar-mode 1))

This is the core reason of existence of tab-bar-show separate from tab-bar-mode:
when the user creates a new tab, the tab-bar should be activated,
unless the user has customized tab-bar-show to nil.

Actually, a more proper condition should be:

  (when tab-bar-show
    (tab-bar-mode 1))

and then after that you can use the remaining code:

> +    ;; Recalculate tab-bar-lines and update frames
> +    (tab-bar--update-tab-bar-lines (selected-frame))
> +    (when tab-bar-mode
> +      (tab-bar--load-buttons)
> +      (tab-bar--define-keys))

In tab-bar-close-other-tabs:

> +      ;; Recalculate tab-bar-lines and update frames
> +      (tab-bar--update-tab-bar-lines)

It could affect only the selected frame too, i.e.:
(tab-bar--update-tab-bar-lines (selected-frame))



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Juri,

On Mon, Feb 8, 2021 at 7:23 PM Juri Linkov <[hidden email]> wrote:

>
> Hello Bastian,
>
> > I have tried to clean up my patch a bit more, please see the attached version 3.
>
> Thanks, everything looks right, except one thing that you removed
> the most important rule:
>
> > -    (cond
> > -     ((eq tab-bar-show t)
> > -      (tab-bar-mode 1))
>
> This is the core reason of existence of tab-bar-show separate from tab-bar-mode:
> when the user creates a new tab, the tab-bar should be activated,
> unless the user has customized tab-bar-show to nil.
>
> Actually, a more proper condition should be:
>
>   (when tab-bar-show
>     (tab-bar-mode 1))
>
> and then after that you can use the remaining code:
>
> > +    ;; Recalculate tab-bar-lines and update frames
> > +    (tab-bar--update-tab-bar-lines (selected-frame))
> > +    (when tab-bar-mode
> > +      (tab-bar--load-buttons)
> > +      (tab-bar--define-keys))
>
> In tab-bar-close-other-tabs:
>
> > +      ;; Recalculate tab-bar-lines and update frames
> > +      (tab-bar--update-tab-bar-lines)
>
> It could affect only the selected frame too, i.e.:
> (tab-bar--update-tab-bar-lines (selected-frame))
Thank you for your comments. I have updated the patch according to
your suggestions. If it looks fine to you now, could you install it
for me?

Best
Bastian

tab-bar_v4.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

martin rudalics
 > +  "Update frame parameter tab-bar-line.
 > +When the optional frame parameter is omitted all frames as well
 > +as the default for new frames are updated. Otherwise only the
 > +given frame is modified."

This is confusing wrt our parameter/argument terminology.  Please just
say "FRAME" wherever it stands for the function's argument and use
"parameter of FRAME" when you talk about the frame parameter.  Also
please clarify: Does the value of the 'tab-bar-lines' parameter now
stand for the number of (matrix) rows occupied by the tab bar (for
example, 2 if the tab bar wraps once) or the number of frame lines
calculated by dividing the pixel size of the tab bar by the frame's
default line height?

And finally: Why can't we wrap tab bars at tab boundaries so one tab
never spans two lines unless it is too long for the frame (in which case
it should be just truncated IMO).

martin



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hey Martin,

On Tue, Feb 9, 2021 at 9:00 AM martin rudalics <[hidden email]> wrote:
>
>  > +  "Update frame parameter tab-bar-line.
>  > +When the optional frame parameter is omitted all frames as well
>  > +as the default for new frames are updated. Otherwise only the
>  > +given frame is modified."
>
> This is confusing wrt our parameter/argument terminology.  Please just
> say "FRAME" wherever it stands for the function's argument and use
> "parameter of FRAME" when you talk about the frame parameter.

I'll do my best to update the docstring. Note that I do not contribute
to emacs often, nor do I regularly code in elisp, so I'm not familiar
with the conventions and you are encouraged to modify my changes as
you see fit.

I propose to change the docstring as follows:

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 9666fb2460..2c3d976f28 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -135,7 +135,7 @@ Possible modifier keys are `control', `meta',
`shift', `hyper', `super' and
                         tab-bar-close-button)))

(defun tab-bar--tab-bar-lines-for-frame (frame)
-  "Compute the correct value of tab-bar-lines for the given frame."
+  "Compute the correct value of tab-bar-lines for FRAME."
  (if (not tab-bar-mode)
      0
    (cond
@@ -145,10 +145,10 @@ Possible modifier keys are `control', `meta',
`shift', `hyper', `super' and
     (t 0))))

(defun tab-bar--update-tab-bar-lines (&optional frame)
-  "Update frame parameter tab-bar-line.
-When the optional frame parameter is omitted all frames as well
-as the default for new frames are updated. Otherwise only the
-given frame is modified."
+  "Update tab-bar-line parameter in frames.
+When the optional parameter FRAME is omitted all frames as well
+as the default for new frames are updated. Otherwise only FRAME
+is modified."
  (if frame
      ;; Set frame parameters for the given frame
      (set-frame-parameter frame 'tab-bar-lines
(tab-bar--tab-bar-lines-for-frame frame))

> Also
> please clarify: Does the value of the 'tab-bar-lines' parameter now
> stand for the number of (matrix) rows occupied by the tab bar (for
> example, 2 if the tab bar wraps once) or the number of frame lines
> calculated by dividing the pixel size of the tab bar by the frame's
> default line height?
>
> And finally: Why can't we wrap tab bars at tab boundaries so one tab
> never spans two lines unless it is too long for the frame (in which case
> it should be just truncated IMO).

As far as I understand, tab-bar-lines is always just 1 or 0, meaning
whether to show the tab-bar at all or not. Maybe it would be better to
just rename the parameter? I guess if that is done then it does not
necessarily need further explanation in docstrings.

>
> martin

Bastian



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
Hello Martin,

On Tue, Feb 9, 2021 at 9:58 AM martin rudalics <[hidden email]> wrote:
>
>  > I'll do my best to update the docstring. Note that I do not contribute
>  > to emacs often, nor do I regularly code in elisp, so I'm not familiar
>  > with the conventions and you are encouraged to modify my changes as
>  > you see fit.
>
> Don't worry.  These "-lines" frame parameters are a minefield - we just
> should try to clarify things the best possible way while you're working
> on it.

I see.

>
>  > (defun tab-bar--tab-bar-lines-for-frame (frame)
>  > -  "Compute the correct value of tab-bar-lines for the given frame."
>  > +  "Compute the correct value of tab-bar-lines for FRAME."
>
> But what "is" the correct value and why and how would we want to
> "compute" it?

How about this:

diff --git a/lisp/tab-bar.el b/lisp/tab-bar.el
index 9666fb2460..1fc8537ccf 100644
--- a/lisp/tab-bar.el
+++ b/lisp/tab-bar.el
@@ -135,7 +135,11 @@ Possible modifier keys are `control', `meta',
`shift', `hyper', `super' and
                         tab-bar-close-button)))

(defun tab-bar--tab-bar-lines-for-frame (frame)
-  "Compute the correct value of tab-bar-lines for the given frame."
+  "Determine the value of tab-bar-lines for FRAME.
+The returned value will either be 1 or 0, meaning whether to show
+the tab-bar or not. If tab-bar-mode is off, the returned value is
+0. Otherwise the result depends on the value of the customizable
+variable `tab-bar-show'."
  (if (not tab-bar-mode)
      0
    (cond
@@ -145,10 +149,10 @@ Possible modifier keys are `control', `meta',
`shift', `hyper', `super' and
     (t 0))))

(defun tab-bar--update-tab-bar-lines (&optional frame)
-  "Update frame parameter tab-bar-line.
-When the optional frame parameter is omitted all frames as well
-as the default for new frames are updated. Otherwise only the
-given frame is modified."
+  "Update the tab-bar-line parameter in frames.
+When the optional parameter FRAME is omitted all frames as well
+as the default for new frames are updated. Otherwise only FRAME
+is modified."
  (if frame
      ;; Set frame parameters for the given frame
      (set-frame-parameter frame 'tab-bar-lines
(tab-bar--tab-bar-lines-for-frame frame))

>
>  > As far as I understand, tab-bar-lines is always just 1 or 0, meaning
>  > whether to show the tab-bar at all or not. Maybe it would be better to
>  > just rename the parameter? I guess if that is done then it does not
>  > necessarily need further explanation in docstrings.
>
> That ship has sailed long ago.  Neither the 'menu-bar-lines' nor the
> 'tool-bar-lines' parameters convey useful information in this regard and
> 'tab-bar-lines' just follows suit.  Their only practical (and completely
> misguided IMHO) purpose is to show the corresponding bar by setting the
> parameter to a non-zero value and remove it by setting the parameter to
> zero.
>
> Sometimes, as with our native tool bars, their value gives the number of
> frame lines occupied by the bar.  And very occasionally setting the
> parameter to a non-zero value can have strange effects: On a non-toolkit
> build setting menu-bar-lines to 7 will show six blank lines below a
> one-line menu bar which does not wrap anyway.
>
> In either case, we can hardly change the names of these frame parameters
> because they probably appear in too many applications and init files out
> there.  We could state somewhere that these are, in fact, booleans and
> should be set and interpreted in that sense.  Even that is not entirely
> trivial.

Makes sense!

>
> martin
Have a nice day
Bastian



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

martin rudalics
 > +  "Determine the value of tab-bar-lines for FRAME.
 > +The returned value will either be 1 or 0, meaning whether to show
 > +the tab-bar or not. If tab-bar-mode is off, the returned value is
 > +0. Otherwise the result depends on the value of the customizable
 > +variable `tab-bar-show'."

Please use active voice and always make sure to leave two spaces in
front of a new sentence.  I would write it as

(defun tab-bar--tab-bar-lines-for-frame (frame)
   "Return new value of `tab-bar-lines' parameter for specified FRAME.
Return 0 if `tab-bar-mode' is not enabled on FRAME or ....
Return 1 if `tab-bar-mode' is enabled on FRAME and the variable
`tab-bar-show' ...

Call this function when ..."

where the last sentence would describe when and why this function should
be called, justifying the "new" in the doc-string.  Instead of "new" you
can also use "adjusted" if the value gets just adjusted or your earlier
"correct" provided an earlier calculation was "incorrect".

martin



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
On Tue, Feb 9, 2021 at 10:45 AM martin rudalics <[hidden email]> wrote:

>
>  > +  "Determine the value of tab-bar-lines for FRAME.
>  > +The returned value will either be 1 or 0, meaning whether to show
>  > +the tab-bar or not. If tab-bar-mode is off, the returned value is
>  > +0. Otherwise the result depends on the value of the customizable
>  > +variable `tab-bar-show'."
>
> Please use active voice and always make sure to leave two spaces in
> front of a new sentence.  I would write it as
>
> (defun tab-bar--tab-bar-lines-for-frame (frame)
>    "Return new value of `tab-bar-lines' parameter for specified FRAME.
> Return 0 if `tab-bar-mode' is not enabled on FRAME or ....
> Return 1 if `tab-bar-mode' is enabled on FRAME and the variable
> `tab-bar-show' ...
>
> Call this function when ..."
>
> where the last sentence would describe when and why this function should
> be called, justifying the "new" in the doc-string.  Instead of "new" you
> can also use "adjusted" if the value gets just adjusted or your earlier
> "correct" provided an earlier calculation was "incorrect".
Thanks for your comments. I went with

(defun tab-bar--tab-bar-lines-for-frame (frame)
  "Determine and return the value of `tab-bar-lines' for FRAME.
Return 0 if `tab-bar-mode' is not enabled.  Otherwise return
either 1 or 0 depending on the value of the customizable variable
`tab-bar-show', which see."

Please see the attached v5 of the complete patch.

I don't think tab-bar-mode can vary from frame to frame? I also did
not add the "Call this function" part, because this is an internal
helper function that is only used from
"tab-bar--update-tab-bar-lines", just below. It just returns 0 or 1
and does not modify anything, the actual frame parameter modification
happens in the other function.

Hope this is satisfactory, if not please feel free to adjust as you wish.

Do I need to do the contribution paperworks for this patch? So far I
haven't done that.

Best
Bastian

tab-bar_v5.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

martin rudalics
 > (defun tab-bar--tab-bar-lines-for-frame (frame)
 >    "Determine and return the value of `tab-bar-lines' for FRAME.
 > Return 0 if `tab-bar-mode' is not enabled.  Otherwise return
 > either 1 or 0 depending on the value of the customizable variable
 > `tab-bar-show', which see."

Fine with me.

 > I don't think tab-bar-mode can vary from frame to frame?

I think only the 'tab-bar-lines' parameter can vary.

 > I also did
 > not add the "Call this function" part, because this is an internal
 > helper function that is only used from
 > "tab-bar--update-tab-bar-lines", just below. It just returns 0 or 1
 > and does not modify anything, the actual frame parameter modification
 > happens in the other function.
 >
 > Hope this is satisfactory, if not please feel free to adjust as you wish.

I leave that to Juri.

 > Do I need to do the contribution paperworks for this patch? So far I
 > haven't done that.

I suppose you need to do that.

Thanks, martin



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Juri Linkov-2
In reply to this post by Bastian Beranek
> I don't think tab-bar-mode can vary from frame to frame?

Indeed, currently tab-bar-mode is global, but not frame-local.
It's just a convenient way to enable the tab-bar without creating
a new tab first (when tab-bar-show is t).  Another convenience of
tab-bar-mode is enabling the C-TAB keybindings for switching tabs.

> Hope this is satisfactory, if not please feel free to adjust as you wish.

Thanks, looks nice, but needs a little more testing with different settings.

> Do I need to do the contribution paperworks for this patch? So far I
> haven't done that.

I don't know.  Please ask Eli or Lars if your contributions
fit into the updated rule in https://debbugs.gnu.org/44834#73



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
On Tue, Feb 9, 2021 at 7:15 PM Juri Linkov <[hidden email]> wrote:

>
> > I don't think tab-bar-mode can vary from frame to frame?
>
> Indeed, currently tab-bar-mode is global, but not frame-local.
> It's just a convenient way to enable the tab-bar without creating
> a new tab first (when tab-bar-show is t).  Another convenience of
> tab-bar-mode is enabling the C-TAB keybindings for switching tabs.
>
> > Hope this is satisfactory, if not please feel free to adjust as you wish.
>
> Thanks, looks nice, but needs a little more testing with different settings.
>

Thanks. I have been running it myself for a while now and didn't
notice anything, but more testing is always good!

> > Do I need to do the contribution paperworks for this patch? So far I
> > haven't done that.
>
> I don't know.  Please ask Eli or Lars if your contributions
> fit into the updated rule in https://debbugs.gnu.org/44834#73

Eli, Lars, what do you think? Since I already contributed a few
patches I would guess I need to assign copyright. I am fine with that,
maybe it's time to do it even if my contribution could be accepted
without. Could you please send me the form?



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Eli Zaretskii
In reply to this post by Juri Linkov-2
> From: Juri Linkov <[hidden email]>
> Date: Tue, 09 Feb 2021 20:06:07 +0200
> Cc: [hidden email]
>
> > Do I need to do the contribution paperworks for this patch? So far I
> > haven't done that.
>
> I don't know.  Please ask Eli or Lars if your contributions
> fit into the updated rule in https://debbugs.gnu.org/44834#73

They definitely exceed the threshold.



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Juri Linkov-2
In reply to this post by Bastian Beranek
>>  > +  "Update frame parameter tab-bar-line.
>>  > +When the optional frame parameter is omitted all frames as well
>>  > +as the default for new frames are updated. Otherwise only the
>>  > +given frame is modified."
>>
>> This is confusing wrt our parameter/argument terminology.  Please just
>> say "FRAME" wherever it stands for the function's argument and use
>> "parameter of FRAME" when you talk about the frame parameter.
>
> I'll do my best to update the docstring. Note that I do not contribute
> to emacs often, nor do I regularly code in elisp, so I'm not familiar
> with the conventions and you are encouraged to modify my changes as
> you see fit.

I don't know if Martin will agree, but most frame functions
interpret their optional FRAME argument in such a way that
if it's nil or omitted, FRAME defaults to the selected frame.

For example, 'set-frame-font' documents this:

  If FRAMES is nil, apply the font to the selected frame only.
  If FRAMES is non-nil, it should be a list of frames to act upon,
  or t meaning all existing graphical frames.

and uses such implementation:

   (let ((frame-lst (cond ((null frames)
                           (list (selected-frame)))
                          ((eq frames t)
                           (frame-list))
                          (t frames))))
      (dolist (f frame-lst)



Reply | Threaded
Open this post in threaded view
|

bug#46299: 28.0.50; Value of tab-bar-show not respected in new frames.

Bastian Beranek
In reply to this post by Bastian Beranek
Hello Juri,

I have updated my patch (see v6 attached). Please find my comments inline.

I have also finished the copyright assignment procedure now.

On Wed, Feb 10, 2021 at 7:41 PM Juri Linkov <[hidden email]> wrote:

> I don't know if Martin will agree, but most frame functions
> interpret their optional FRAME argument in such a way that
> if it's nil or omitted, FRAME defaults to the selected frame.
>
> For example, 'set-frame-font' documents this:
>
>   If FRAMES is nil, apply the font to the selected frame only.
>   If FRAMES is non-nil, it should be a list of frames to act upon,
>   or t meaning all existing graphical frames.
>
> and uses such implementation:
>
>    (let ((frame-lst (cond ((null frames)
>                            (list (selected-frame)))
>                           ((eq frames t)
>                            (frame-list))
>                           (t frames))))
>       (dolist (f frame-lst)
That's true and I noticed this inconsistency as well. So thanks for
the suggestion, I have updated the patch accordingly.

On Wed, Feb 10, 2021 at 7:41 PM Juri Linkov <[hidden email]> wrote:

>
> > Hope this is satisfactory, if not please feel free to adjust as you wish.
>
> Thanks, please see more comments:
>
> > +(defun tab-bar--tab-bar-lines-for-frame (frame)
> > +  (if (not tab-bar-mode)
> > +      0
> > +    (cond
> > +     ((eq tab-bar-show t) 1)
> > +     ((natnump tab-bar-show)
> > +      (if (> (length (funcall tab-bar-tabs-function frame)) tab-bar-show) 1 0))
> > +     (t 0))))
>
> A small optimization:
>
>   ((not tab-bar-mode) 0)
>
> could be added as the first condition of the same 'cond'.
Thanks! Done.

>
> >    :set (lambda (sym val)
> >           (set-default sym val)
> >           ;; Preload button images
> > +         ;; Note: tab-bar-mode updates tab-bar-lines as well.
> > +         (tab-bar-mode 1))
>
> Not sure whether the users would want to enable tab-bar-mode
> unconditionally after customizing tab-bar-show.
>
> Maybe when customized tab-bar-show to nil, only call
> tab-bar--update-tab-bar-lines in all frames?
> Or maybe simply to disable the tab bar with (tab-bar-mode 0)
> when customized to nil?
I am not sure either. I think the best is to just leave tab-bar-mode
as it is to be honest. All this entanglement doesn't seem very clean
to me. Yes this would mean that the user needs to manually enable
tab-bar-mode after customizing the variable, but on the other hand
tab-bar-mode is on by default, so the user must have switched it off
in his .emacs by choice. So I just added the call the
tab-bar--update-tab-bar-lines for all frames, because this is
necessary for sure. On the other hand I don't fully understand the
comment about 'Preload button images'. I think the images and
keybindings are loaded when tab-bar-mode is switched on and afterwards
whenever a new tab is created in tab-bar-new-to, so it seems
independent of tab-bar-show. When tab-bar-show is customized they are
either already loaded because tab-bar-mode is on, or if it is not they
are not required and will be loaded when tab-bar-mode is activated.

>
> > @@ -852,16 +867,15 @@ After the tab is created, the hooks in
> > +    ;; Switch on tab-bar-mode, since a tab was created
> > +    (when tab-bar-show
> >        (tab-bar-mode 1))
> > +
> > +    ;; Recalculate tab-bar-lines and update frames
> > +    (tab-bar--update-tab-bar-lines (selected-frame))
> > +    (when tab-bar-mode
> > +      (tab-bar--load-buttons)
> > +      (tab-bar--define-keys))
>
> Would you agree that here in tab-bar-new-tab-to, the first call of
> tab-bar-mode should already do all these calls: tab-bar--update-tab-bar-lines,
> tab-bar--load-buttons, tab-bar--define-keys?  So maybe it should be
> sufficient just to leave these 2 lines here:
>
>     (when tab-bar-show
>        (tab-bar-mode 1))
Yes I agree that tab-bar--update-tab-bar-lines is not needed. It
happens in the line before when tab-bar-show is not nil and doesn't
matter otherwise. I have left these two lines, though:

    (when tab-bar-mode
      (tab-bar--load-buttons)
      (tab-bar--define-keys))

Because I think defining the keys is useful even if tab-bar-show is
nil, so you can switch to another tab using the key bindings even if
you can't see the tab-bar. As for the buttons, I think it makes sense
to load them so that in case tab-bar-show is customized to another
value afterwards they are available directly.

tab-bar_v6.patch (7K) Download Attachment
12