ELPA: New package: nano-theme

ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

Dear all,

I would like to submit a new package to ELPA which is a theme 
based on Material colors (https://material.io/) for the light 
version and Nord colors (https://www.nordtheme.com/) for the dark 
version. The theme itself is made of (only) 6 different faces and 
the rationale is here: https://arxiv.org/abs/2008.06030

The theme is hosted at https://github.com/rougier/nano-theme and 
the README displays what it looks like. Note that the theme 
exploits face inheritance a lot and Eli explained me that it might 
degrade performances. I don't know if this needs to be fixed.

Best,
Nicolas

Re: ELPA: New package: nano-theme

[Philip Kaludercic]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> Dear all,
>
> I would like to submit a new package to ELPA which is a theme based on
> Material colors (https://material.io/) for the light version and Nord
> colors (https://www.nordtheme.com/) for the dark version. The theme
> itself is made of (only) 6 different faces and the rationale is here:
> https://arxiv.org/abs/2008.06030
>
> The theme is hosted at https://github.com/rougier/nano-theme and the
> README displays what it looks like. Note that the theme exploits face
> inheritance a lot and Eli explained me that it might degrade
> performances. I don't know if this needs to be fixed.

I see that a lot of faces inherit from nano-default, nano-strong,
nano-faded, etc. Wouldn't it make more sense to inherit from built-in
faces and then let default, bold, shadow, etc. inherit from these faces.

Furthermore, I wonder why you define the commands nano-light and
nano-dark, instead of two themes, nano-light and nano-dark (along the
same lines of what modus-themes currently does). You could also turn
nano-setup into a custom theme, so that the user can easily enable and
disable it.

A few more points:

- Should the font really be part of the face? I think it makes sense to
  recommend a few fonts, but it is unconventional to add it as part of a
  theme.
- When you are only using one branch of a if expression, prefer
  when. Instead of (if (not ...) ...), prefer unless.
- Lines 363ff. seem to be indented unconventionally. Maybe add a
  .dir-locals.el to make sure everyone is using the same whitespace
  configuration.

Also, as you depend on at least 27.1, you probably don't have to check
if custom-theme-load-path is defined. The same applies to tooltip-mode,
scroll-bar-mode and tool-bar-mode. What you are doing looks more like
configuration code, where you want to make sure that a .emacs works on
older versions of Emacs.

> Best,
> Nicolas
>
>

-- 
	Philip Kaludercic

Re: ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

Many thanks for the review and the feedback.

Philip Kaludercic <philipk@posteo.net> writes:

> "Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> I see that a lot of faces inherit from nano-default, 
> nano-strong,
> nano-faded, etc. Wouldn't it make more sense to inherit from 
> built-in
> faces and then let default, bold, shadow, etc. inherit from 
> these faces.

Having such names helps a lot when you assign faces. For example, 
when I define font-lock-comment-face, I want to make it faded and 
it feels more natural and explicit to inherit from 
nano-faded. Also, is there an associated computational cost when 
defining a new face?

> Furthermore, I wonder why you define the commands nano-light and
> nano-dark, instead of two themes, nano-light and nano-dark 
> (along the
> same lines of what modus-themes currently does). You could also 
> turn
> nano-setup into a custom theme, so that the user can easily 
> enable and
> disable it.

Maybe I need to read the documentation on what a theme can 
set. For example, I set the face for the minibuffer (0 & 1) and 
echo area (0 & 1) and I wasn't sure how to specify this in a 
theme. Same for underline to be set at descent line, etc.

> A few more points:
>
> - Should the font really be part of the face? I think it makes 
> sense to
>   recommend a few fonts, but it is unconventional to add it as 
>   part of a
>   theme.

I agree this might be unconventional but since you can specify a 
font family for each face, why not use it? And user can still 
choose its own font stack.

> - When you are only using one branch of a if expression, prefer
>   when. Instead of (if (not ...) ...), prefer unless.

Thanks, I'll do that.


> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>   .dir-locals.el to make sure everyone is using the same 
>   whitespace
>   configuration.

Not sure what you mean by add a .dir-locals.el.
Can you give me a pointer to the documentation?

> Also, as you depend on at least 27.1, you probably don't have to 
> check
> if custom-theme-load-path is defined. The same applies to 
> tooltip-mode,
> scroll-bar-mode and tool-bar-mode. What you are doing looks more 
> like
> configuration code, where you want to make sure that a .emacs 
> works on
> older versions of Emacs.

Thansk, didn't know, I'll correct it.


Nicolas

Re: ELPA: New package: nano-theme

[Stefan Kangas]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> Maybe I need to read the documentation on what a theme can
> set. For example, I set the face for the minibuffer (0 & 1) and
> echo area (0 & 1) and I wasn't sure how to specify this in a
> theme. Same for underline to be set at descent line, etc.

You just put the theme in two different files, and set anything you like
from that file.  You could take a look at how modus-themes does this.

> I agree this might be unconventional but since you can specify a
> font family for each face, why not use it? And user can still
> choose its own font stack.

When I switch between themes, I don't want them to have different fonts,
and have to configure it separately for each one.

I have set a font I like in my configuration, and I would like themes to
respect that setting.

>> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>>   .dir-locals.el to make sure everyone is using the same
>>   whitespace
>>   configuration.
>
> Not sure what you mean by add a .dir-locals.el.
> Can you give me a pointer to the documentation?

Search for "directory local variables" in the manual and you will find
instructions there.  Or type `M-x add-dir-local-variable RET'.

Re: ELPA: New package: nano-theme

[Stefan Monnier]

> When I switch between themes, I don't want them to have different fonts,
> and have to configure it separately for each one.
>
> I have set a font I like in my configuration, and I would like themes to
> respect that setting.

Hmm... Custom's "user" theme should take precedence over those other
themes, so you should be able to specify your favorite font in your user
theme and then not be affected by those other theme's choice of font.

Of course, that depends if those choices are applied to the same faces
or not etc... but my point is that you might consider this as a bug that
can be fixed.


        Stefan

Re: ELPA: New package: nano-theme

[Stefan Kangas]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Hmm... Custom's "user" theme should take precedence over those other
> themes, so you should be able to specify your favorite font in your user
> theme and then not be affected by those other theme's choice of font.
>
> Of course, that depends if those choices are applied to the same faces
> or not etc... but my point is that you might consider this as a bug that
> can be fixed.

I'm not sure I understand what you mean by "user" theme here.  Do you
mean customizations of faces made with M-x customize-face?

For the default font, I simply do this, on recommendation from `(emacs)
Fonts':

    (add-to-list 'default-frame-alist '(font . "Ubuntu Mono-14"))

Re: ELPA: New package: nano-theme

[Stefan Monnier]

Stefan Kangas [2021-09-29 08:20:13] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Hmm... Custom's "user" theme should take precedence over those other
>> themes, so you should be able to specify your favorite font in your user
>> theme and then not be affected by those other theme's choice of font.
>> Of course, that depends if those choices are applied to the same faces
>> or not etc... but my point is that you might consider this as a bug that
>> can be fixed.
> I'm not sure I understand what you mean by "user" theme here.  Do you
> mean customizations of faces made with M-x customize-face?

Yes.  All modifications done via Custom but "outside of a theme" are
actually internally performed by tweaking a special "user" theme, which
is then combines with the other activated themes.

> For the default font, I simply do this, on recommendation from `(emacs)
> Fonts':
>
>     (add-to-list 'default-frame-alist '(font . "Ubuntu Mono-14"))

That indeed won't interact well with Custom themes.


        Stefan

Re: ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

Stefan Kangas <stefankangas@gmail.com> writes:

> "Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:
>
>> Maybe I need to read the documentation on what a theme can
>> set. For example, I set the face for the minibuffer (0 & 1) and
>> echo area (0 & 1) and I wasn't sure how to specify this in a
>> theme. Same for underline to be set at descent line, etc.
>
> You just put the theme in two different files, and set anything 
> you like
> from that file.  You could take a look at how modus-themes does 
> this.

One of the reason I defined the theme the way it is defined is to 
be able to have both the light and dark theme as for example in 
this screenshot: 
https://github.com/rougier/nano-sidebar/blob/master/images/nano-sidebar-ibuffer.png

I search for how to apply a theme to a single frame but did not 
find any obvious way. I ended up exploiting the dark/light frame 
background mode. As for splitting the theme in two files, I'm not 
sure I understand how this would help with settings some 
variables. 

>>> - Lines 363ff. seem to be indented unconventionally. Maybe add 
>>> a
>>>   .dir-locals.el to make sure everyone is using the same
>>>   whitespace
>>>   configuration.
>>
>> Not sure what you mean by add a .dir-locals.el.
>> Can you give me a pointer to the documentation?
>
> Search for "directory local variables" in the manual and you 
> will find
> instructions there.  Or type `M-x add-dir-local-variable RET'.

In the end, I fixed the indentation that was mixing tabs and 
space.

Nicolas

Re: ELPA: New package: nano-theme

[Stefan Monnier]

> I search for how to apply a theme to a single frame but did not find any
> obvious way.

Custom themes are sets of user settings (i.e. affecting typically
variables and faces), so they're naturally global.

So you can't have that "single frame" effect directly by specifying face
settings in the theme.

What you can do instead is:
- You can have a custom theme set a particular minor mode (provided as
  part of the theme, for example) and arrange for that minor mode to
  affect faces in a single frame.
- You can use the DISPLAY spec of face settings to restrict your face
  settings to specific classes of frames.  The DISPLAY specs could be
  extended to be more flexible if needed (file a bug report for that).


        Stefan

Re: ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

I think it is possible by first setting the frame-background-mode 
(to 'dark or 'light in my case), then create a new frame and call 
frame-set-background-mode on the newly created frame. It seems to 
work consistently on my system but maybe this is a side 
effect. Here is the code:

#+BEGIN_SRC elisp
(defun nano-new-frame (&optional mode)
  (interactive)
  (let ((mode (or mode (frame-parameter nil 'background-mode)))
        (background-mode frame-background-mode)
        (selected-frame (selected-frame))
        (new-frame nil))

    ;; Set mode
    (setq frame-background-mode mode)
    (setq new-frame (make-frame-command))
    (select-frame new-frame)

    ;; This forces recomputation of faces on the new frame
    (frame-set-background-mode (selected-frame))
           
    (when (eq mode 'light)
      (set-foreground-color nano-light-foreground)
      (set-background-color nano-light-background))

    (when (eq mode 'dark)
      (set-foreground-color nano-dark-foreground)
      (set-background-color nano-dark-background))

    ;; Restore background mode
    (setq frame-background-mode background-mode)
    (frame-set-background-mode selected-frame)
    
    new-frame))
#+END_SRC

Nicolas

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I search for how to apply a theme to a single frame but did not 
>> find any
>> obvious way.
>
> Custom themes are sets of user settings (i.e. affecting 
> typically
> variables and faces), so they're naturally global.
>
> So you can't have that "single frame" effect directly by 
> specifying face
> settings in the theme.
>
> What you can do instead is:
> - You can have a custom theme set a particular minor mode 
> (provided as
>   part of the theme, for example) and arrange for that minor 
>   mode to
>   affect faces in a single frame.
> - You can use the DISPLAY spec of face settings to restrict your 
> face
>   settings to specific classes of frames.  The DISPLAY specs 
>   could be
>   extended to be more flexible if needed (file a bug report for 
>   that).
>
>
>         Stefan

Re: ELPA: New package: nano-theme

[Philip Kaludercic]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> Many thanks for the review and the feedback.
>
> Philip Kaludercic <philipk@posteo.net> writes:
>
>> "Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:
>
>> I see that a lot of faces inherit from nano-default, nano-strong,
>> nano-faded, etc. Wouldn't it make more sense to inherit from
>> built-in
>> faces and then let default, bold, shadow, etc. inherit from these
>> faces.
>
> Having such names helps a lot when you assign faces. For example, when
> I define font-lock-comment-face, I want to make it faded and it feels
> more natural and explicit to inherit from nano-faded. Also, is there
> an associated computational cost when defining a new face?

There has to be *some* computation, but thinking again about this, it
probably shouldn't matter much if you define bold and let everything
inherit from bold or if you define nano-strong and inherit from
nano-strong.

>> Furthermore, I wonder why you define the commands nano-light and
>> nano-dark, instead of two themes, nano-light and nano-dark (along
>> the
>> same lines of what modus-themes currently does). You could also turn
>> nano-setup into a custom theme, so that the user can easily enable
>> and
>> disable it.
>
> Maybe I need to read the documentation on what a theme can set. For
> example, I set the face for the minibuffer (0 & 1) and echo area (0 &
> 1) and I wasn't sure how to specify this in a theme. Same for
> underline to be set at descent line, etc.

Not directly, but you could define a custom option/minor mode that is
activated as part of the theme to change these things too.

>> A few more points:
>>
>> - Should the font really be part of the face? I think it makes sense
>> to
>>   recommend a few fonts, but it is unconventional to add it as
>> part of a
>>   theme.
>
> I agree this might be unconventional but since you can specify a font
> family for each face, why not use it? And user can still choose its
> own font stack.

It was always my impression that faces are for users to set.

>> - When you are only using one branch of a if expression, prefer
>>   when. Instead of (if (not ...) ...), prefer unless.
>
> Thanks, I'll do that.
>
>
>> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>>   .dir-locals.el to make sure everyone is using the same
>> whitespace
>>   configuration.
>
> Not sure what you mean by add a .dir-locals.el.
> Can you give me a pointer to the documentation?

See (emacs) Directory Variables.

>> Also, as you depend on at least 27.1, you probably don't have to
>> check
>> if custom-theme-load-path is defined. The same applies to
>> tooltip-mode,
>> scroll-bar-mode and tool-bar-mode. What you are doing looks more
>> like
>> configuration code, where you want to make sure that a .emacs works
>> on
>> older versions of Emacs.
>
> Thansk, didn't know, I'll correct it.

-- 
	Philip Kaludercic

Re: ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

>>> Furthermore, I wonder why you define the commands nano-light 
>>> and
>>> nano-dark, instead of two themes, nano-light and nano-dark 
>>> (along
>>> the
>>> same lines of what modus-themes currently does). You could 
>>> also turn
>>> nano-setup into a custom theme, so that the user can easily 
>>> enable
>>> and
>>> disable it.
>>
>> Maybe I need to read the documentation on what a theme can 
>> set. For
>> example, I set the face for the minibuffer (0 & 1) and echo 
>> area (0 &
>> 1) and I wasn't sure how to specify this in a theme. Same for
>> underline to be set at descent line, etc.
>
> Not directly, but you could define a custom option/minor mode 
> that is
> activated as part of the theme to change these things too.

But then you need to load the theme and activate the minor mode, 
do you?. Having a (nano-light) command is more direct in my 
opinion and in any case, user is free to only load the theme the 
regular way.

>>> - Lines 363ff. seem to be indented unconventionally. Maybe add 
>>> a
>>>   .dir-locals.el to make sure everyone is using the same
>>> whitespace
>>>   configuration.

I had a mix of space and tabs for unknown reason. Hopefully it has 
been fixed.

Nicolas

Re: ELPA: New package: nano-theme

[Philip Kaludercic]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

>>>> Furthermore, I wonder why you define the commands nano-light and
>>>> nano-dark, instead of two themes, nano-light and nano-dark (along
>>>> the
>>>> same lines of what modus-themes currently does). You could also
>>>> turn
>>>> nano-setup into a custom theme, so that the user can easily enable
>>>> and
>>>> disable it.
>>>
>>> Maybe I need to read the documentation on what a theme can set. For
>>> example, I set the face for the minibuffer (0 & 1) and echo area (0
>>> &
>>> 1) and I wasn't sure how to specify this in a theme. Same for
>>> underline to be set at descent line, etc.
>>
>> Not directly, but you could define a custom option/minor mode that
>> is
>> activated as part of the theme to change these things too.
>
> But then you need to load the theme and activate the minor mode, do
> you?. Having a (nano-light) command is more direct in my opinion and
> in any case, user is free to only load the theme the regular way.

No, a custom theme can enable a custom option (via
custom-theme-set-variables), and since every minor mode is also a user
option, you can enable a minor mode as part of a theme.

>>>> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>>>>   .dir-locals.el to make sure everyone is using the same
>>>> whitespace
>>>>   configuration.
>
> I had a mix of space and tabs for unknown reason. Hopefully it has
> been fixed.
>
> Nicolas
>

-- 
	Philip Kaludercic

Re: ELPA: New package: nano-theme

[Philip Kaludercic]

Philip Kaludercic <philipk@posteo.net> writes:

> "Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:
>
>> Dear all,
>>
>> I would like to submit a new package to ELPA which is a theme based on
>> Material colors (https://material.io/) for the light version and Nord
>> colors (https://www.nordtheme.com/) for the dark version. The theme
>> itself is made of (only) 6 different faces and the rationale is here:
>> https://arxiv.org/abs/2008.06030
>>
>> The theme is hosted at https://github.com/rougier/nano-theme and the
>> README displays what it looks like. Note that the theme exploits face
>> inheritance a lot and Eli explained me that it might degrade
>> performances. I don't know if this needs to be fixed.
>
> I see that a lot of faces inherit from nano-default, nano-strong,
> nano-faded, etc. Wouldn't it make more sense to inherit from built-in
> faces and then let default, bold, shadow, etc. inherit from these faces.
>
> Furthermore, I wonder why you define the commands nano-light and
> nano-dark, instead of two themes, nano-light and nano-dark (along the
> same lines of what modus-themes currently does). You could also turn
> nano-setup into a custom theme, so that the user can easily enable and
> disable it.

I see nano-theme was added despite still providing functions for themes,
instead of using customize (and a few other points I raised). Have I
missed an explanation as to why this decision was made? Did it turn out
to not be possible? It seems regrettable to provide a theme on ELPA that
evades the conventional theme infrastructure, especially considering
that ELPA currently doesn't provide that many themes to begin with.

> A few more points:
>
> - Should the font really be part of the face? I think it makes sense to
>   recommend a few fonts, but it is unconventional to add it as part of a
>   theme.
> - When you are only using one branch of a if expression, prefer
>   when. Instead of (if (not ...) ...), prefer unless.
> - Lines 363ff. seem to be indented unconventionally. Maybe add a
>   .dir-locals.el to make sure everyone is using the same whitespace
>   configuration.
>
> Also, as you depend on at least 27.1, you probably don't have to check
> if custom-theme-load-path is defined. The same applies to tooltip-mode,
> scroll-bar-mode and tool-bar-mode. What you are doing looks more like
> configuration code, where you want to make sure that a .emacs works on
> older versions of Emacs.
>
>> Best,
>> Nicolas

-- 
	Philip Kaludercic

Re: ELPA: New package: nano-theme

[Nicolas P. Rougier (inria)]

>
> I see nano-theme was added despite still providing functions for 
> themes,
> instead of using customize (and a few other points I 
> raised). Have I
> missed an explanation as to why this decision was made? Did it 
> turn out
> to not be possible? It seems regrettable to provide a theme on 
> ELPA that
> evades the conventional theme infrastructure, especially 
> considering
> that ELPA currently doesn't provide that many themes to begin 
> with.

I cannot speak for decision but I intend to address all your 
comments for next version (next week most probably).

Nicolas

Re: ELPA: New package: nano-theme

[Philip Kaludercic]

"Nicolas P. Rougier (inria)" <nicolas.rougier@inria.fr> writes:

> I cannot speak for decision but I intend to address all your comments
> for next version (next week most probably).

Ok, that is good to hear. I was just surprised to see the package in
elpa.git all of a sudden.

-- 
	Philip Kaludercic