[PATCH] notifications: Don't expand-file-name app-icon.

[PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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


Icon is not always a file name, but can be just an icon name.
https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

* lisp/notifications.el (notifications-notify): Don't expand-file-name
app-icon.
---
Otherwise it not possible to send a notification similiar to:
notify-send -i multimedia-player "I am playing music"

 lisp/notifications.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/notifications.el b/lisp/notifications.el
index 984ddbec5e9..c314858d0ae 100644
--- a/lisp/notifications.el
+++ b/lisp/notifications.el
@@ -304,7 +304,7 @@ notifications-notify
 					  notifications-application-name)
 			      :uint32 (or replaces-id 0)
 			      :string (if app-icon
-					  (expand-file-name app-icon)
+                                          app-icon
 					;; If app-icon is nil because user
 					;; requested it to be so, send the
 					;; empty string
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH v2] notifications: Allow to use Icon Naming Specification for app-icon

[Andrew Tropin]

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

Icon is not always a file name, but can be just an icon name.
https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

* lisp/notifications.el (notifications-notify): Allow to use Icon
Naming Specification for app-icon.
---
 lisp/notifications.el | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lisp/notifications.el b/lisp/notifications.el
index 984ddbec5e9..13353264de3 100644
--- a/lisp/notifications.el
+++ b/lisp/notifications.el
@@ -137,6 +137,9 @@ notifications-notify
  :app-icon       The notification icon.
                  Default is `notifications-application-icon'.
                  Set to nil if you do not want any icon displayed.
+                 If value is a string expand-file-name will be
+                 applied, if value is a symbol it will be used as
+                 is (helpful when using Icon Naming Specification).
  :actions        A list of actions in the form:
                    (KEY TITLE KEY TITLE ...)
                  where KEY and TITLE are both strings.
@@ -304,7 +307,10 @@ notifications-notify
 					  notifications-application-name)
 			      :uint32 (or replaces-id 0)
 			      :string (if app-icon
-					  (expand-file-name app-icon)
+					  (if (stringp app-icon)
+                                              (expand-file-name app-icon)
+                                            ;; Convert symbol to string
+                                            (symbol-name app-icon))
 					;; If app-icon is nil because user
 					;; requested it to be so, send the
 					;; empty string
-- 
2.41.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Andrew Tropin <andrew@trop.in>
> Date: Mon, 24 Jul 2023 12:39:06 +0400
> 
> Icon is not always a file name, but can be just an icon name.
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
> 
> * lisp/notifications.el (notifications-notify): Don't expand-file-name
> app-icon.
> ---
> Otherwise it not possible to send a notification similiar to:
> notify-send -i multimedia-player "I am playing music"
> 
>  lisp/notifications.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lisp/notifications.el b/lisp/notifications.el
> index 984ddbec5e9..c314858d0ae 100644
> --- a/lisp/notifications.el
> +++ b/lisp/notifications.el
> @@ -304,7 +304,7 @@ notifications-notify
>  					  notifications-application-name)
>  			      :uint32 (or replaces-id 0)
>  			      :string (if app-icon
> -					  (expand-file-name app-icon)
> +                                          app-icon

Thanks, but won't this break the cases where app-icon _is_ a file
name?

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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

On 2023-07-24 15:35, Eli Zaretskii wrote:

>> From: Andrew Tropin <andrew@trop.in>
>> Date: Mon, 24 Jul 2023 12:39:06 +0400
>> 
>> Icon is not always a file name, but can be just an icon name.
>> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>> 
>> * lisp/notifications.el (notifications-notify): Don't expand-file-name
>> app-icon.
>> ---
>> Otherwise it not possible to send a notification similiar to:
>> notify-send -i multimedia-player "I am playing music"
>> 
>>  lisp/notifications.el | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/lisp/notifications.el b/lisp/notifications.el
>> index 984ddbec5e9..c314858d0ae 100644
>> --- a/lisp/notifications.el
>> +++ b/lisp/notifications.el
>> @@ -304,7 +304,7 @@ notifications-notify
>>  					  notifications-application-name)
>>  			      :uint32 (or replaces-id 0)
>>  			      :string (if app-icon
>> -					  (expand-file-name app-icon)
>> +                                          app-icon
>
> Thanks, but won't this break the cases where app-icon _is_ a file
> name?

Tried it and it doesn't, at least both 

:app-icon (expand-file-name "emblem-default.png")
and
:app-icon "emblem-default.png"

work with new implementation, when file in the cwd.

I don't see other use cases that can break, so I guess it's backward
compatible change.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Andrew Tropin <andrew@trop.in>
> Cc: emacs-devel@gnu.org
> Date: Tue, 25 Jul 2023 08:39:31 +0400
> 
> >> diff --git a/lisp/notifications.el b/lisp/notifications.el
> >> index 984ddbec5e9..c314858d0ae 100644
> >> --- a/lisp/notifications.el
> >> +++ b/lisp/notifications.el
> >> @@ -304,7 +304,7 @@ notifications-notify
> >>  					  notifications-application-name)
> >>  			      :uint32 (or replaces-id 0)
> >>  			      :string (if app-icon
> >> -					  (expand-file-name app-icon)
> >> +                                          app-icon
> >
> > Thanks, but won't this break the cases where app-icon _is_ a file
> > name?
> 
> Tried it and it doesn't, at least both 
> 
> :app-icon (expand-file-name "emblem-default.png")
> and
> :app-icon "emblem-default.png"
> 
> work with new implementation, when file in the cwd.

But that's exactly the point: how can we make sure the file is "in
cwd"?

Moreover, in Emacs the concept of cwd makes little sense, because
Emacs behaves as if each buffer had its own cwd.  So when some Emacs
Lisp code runs with some buffer current, that buffer's
default-directory is the effective "cwd" for all file-related
operations.  The actual cwd of the Emacs process doesn't matter, and
AFAIR is never used, except at the very beginning of startup.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Dmitry Gutov]

On 25/07/2023 15:01, Eli Zaretskii wrote:
> But that's exactly the point: how can we make sure the file is "in
> cwd"?

When it's not in cwd, wouldn't (expand-file-name file) fail to work too?

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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

On 2023-07-25 15:59, Dmitry Gutov wrote:

> On 25/07/2023 15:01, Eli Zaretskii wrote:
>> But that's exactly the point: how can we make sure the file is "in
>> cwd"?
>
> When it's not in cwd, wouldn't (expand-file-name file) fail to work too?

Right.

Eli, by cwd I mean default-directory.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Andrew Tropin <andrew@trop.in>
> Cc: emacs-devel@gnu.org
> Date: Tue, 25 Jul 2023 17:41:27 +0400
> 
> > On 25/07/2023 15:01, Eli Zaretskii wrote:
> >> But that's exactly the point: how can we make sure the file is "in
> >> cwd"?
> >
> > When it's not in cwd, wouldn't (expand-file-name file) fail to work too?
> 
> Right.
> 
> Eli, by cwd I mean default-directory.

If that's what you meant, then my question should be rephrased: how do
we know that the code in question always runs when the buffer that is
the current buffer has the directory of the icon file as its
default-directory?

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Dmitry Gutov]

On 25/07/2023 17:05, Eli Zaretskii wrote:
>> From: Andrew Tropin<andrew@trop.in>
>> Cc:emacs-devel@gnu.org
>> Date: Tue, 25 Jul 2023 17:41:27 +0400
>>
>>> On 25/07/2023 15:01, Eli Zaretskii wrote:
>>>> But that's exactly the point: how can we make sure the file is "in
>>>> cwd"?
>>> When it's not in cwd, wouldn't (expand-file-name file) fail to work too?
>> Right.
>>
>> Eli, by cwd I mean default-directory.
> If that's what you meant, then my question should be rephrased: how do
> we know that the code in question always runs when the buffer that is
> the current buffer has the directory of the icon file as its
> default-directory?

If it does not, then the current code is already wrong, isn't it?

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> Date: Tue, 25 Jul 2023 19:48:53 +0300
> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 25/07/2023 17:05, Eli Zaretskii wrote:
> >> From: Andrew Tropin<andrew@trop.in>
> >> Cc:emacs-devel@gnu.org
> >> Date: Tue, 25 Jul 2023 17:41:27 +0400
> >>
> >>> On 25/07/2023 15:01, Eli Zaretskii wrote:
> >>>> But that's exactly the point: how can we make sure the file is "in
> >>>> cwd"?
> >>> When it's not in cwd, wouldn't (expand-file-name file) fail to work too?
> >> Right.
> >>
> >> Eli, by cwd I mean default-directory.
> > If that's what you meant, then my question should be rephrased: how do
> > we know that the code in question always runs when the buffer that is
> > the current buffer has the directory of the icon file as its
> > default-directory?
> 
> If it does not, then the current code is already wrong, isn't it?

No, it could be working by sheer luck.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> >>>> But that's exactly the point: how can we make sure the file is "in
>> >>>> cwd"?
>> >>> When it's not in cwd, wouldn't (expand-file-name file) fail to work too?
>> >> Right.
>> >>
>> >> Eli, by cwd I mean default-directory.
>> > If that's what you meant, then my question should be rephrased: how do
>> > we know that the code in question always runs when the buffer that is
>> > the current buffer has the directory of the icon file as its
>> > default-directory?
>>
>> If it does not, then the current code is already wrong, isn't it?
>
> No, it could be working by sheer luck.

As always with relative file names: the caller is responsible to use a
proper default-directory. What else?

Btw, the relative file name could even be something like
"hicolor/scalable/mimetypes/emacs-document.svg", when default directory
is Emacs' ".../etc/images/icons/". Another argument for expanding the
:app-icon string.

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Dmitry Gutov <dmitry@gutov.dev>,  andrew@trop.in,  emacs-devel@gnu.org
> Date: Tue, 25 Jul 2023 21:09:09 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> If it does not, then the current code is already wrong, isn't it?
> >
> > No, it could be working by sheer luck.
> 
> As always with relative file names: the caller is responsible to use a
> proper default-directory. What else?

Wait a minute.  The icon's file is not processed by Emacs, right?  We
pass it to an external program/library/service, right?

If the file is processed by Emacs itself, then Dmitry is right: the
call to expand-file-name is redundant.  But if the file is processed
by some software outside Emacs, then it is not redundant, because that
external software will handle non-absolute file names as relative to
the cwd of the Emacs process, not to the default-directory of the
buffer that is current when this code runs.

My assumption was that it's the latter: we pass the file name to some
external software.  If that is not the case, then I wonder why
expand-file-name was there to begin with.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> >> If it does not, then the current code is already wrong, isn't it?
>> >
>> > No, it could be working by sheer luck.
>>
>> As always with relative file names: the caller is responsible to use a
>> proper default-directory. What else?
>
> Wait a minute.  The icon's file is not processed by Emacs, right?  We
> pass it to an external program/library/service, right?

We pass it to an external service via a D-Bus call.

> If the file is processed by Emacs itself, then Dmitry is right: the
> call to expand-file-name is redundant.  But if the file is processed
> by some software outside Emacs, then it is not redundant, because that
> external software will handle non-absolute file names as relative to
> the cwd of the Emacs process, not to the default-directory of the
> buffer that is current when this code runs.

Processing D-Bus calls means, there is no knowledge by the daemon about
the cwd of the sender.

> My assumption was that it's the latter: we pass the file name to some
> external software.  If that is not the case, then I wonder why
> expand-file-name was there to begin with.

There was always a good reason to do so.

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: dmitry@gutov.dev,  andrew@trop.in,  emacs-devel@gnu.org
> Date: Wed, 26 Jul 2023 09:05:04 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> As always with relative file names: the caller is responsible to use a
> >> proper default-directory. What else?
> >
> > Wait a minute.  The icon's file is not processed by Emacs, right?  We
> > pass it to an external program/library/service, right?
> 
> We pass it to an external service via a D-Bus call.

In that case, we _definitely_ must call expand-file-name, and using
relative file names without that is asking for trouble.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Matthias Meulien]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: dmitry@gutov.dev,  andrew@trop.in,  emacs-devel@gnu.org
>> Date: Wed, 26 Jul 2023 09:05:04 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> As always with relative file names: the caller is responsible to use a
>> >> proper default-directory. What else?
>> >
>> > Wait a minute.  The icon's file is not processed by Emacs, right?  We
>> > pass it to an external program/library/service, right?
>> 
>> We pass it to an external service via a D-Bus call.
>
> In that case, we _definitely_ must call expand-file-name, and using
> relative file names without that is asking for trouble.

Why not try to be compliant with the called API?  According to the
Desktop Notification specification the Notify method support image path
being file URIs or in a freedesktop.org-compliant icon theme (see
https://specifications.freedesktop.org/notification-spec/notification-spec-latest.html#icons-and-images).

Being able to specify an icon with its name name according to Icon
Naming Specification (see
https://specifications.freedesktop.org/icon-naming-spec/latest/ar01s04.html)
is useful since file path vary depending on the OS!
-- 
Matthias

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Matthias Meulien <orontee@gmail.com> writes:

Hi Matthias,

>> In that case, we _definitely_ must call expand-file-name, and using
>> relative file names without that is asking for trouble.
>
> Why not try to be compliant with the called API?  According to the
> Desktop Notification specification the Notify method support image path
> being file URIs or in a freedesktop.org-compliant icon theme (see
> https://specifications.freedesktop.org/notification-spec/notification-spec-latest.html#icons-and-images).

Sending an image_path instead of app_icon as file:/// URL is just an
implementation detail. The `notifications-notify' API supports
the :image-path argument if users are willing to use it.

:app-icon can still be either a file name or an icon name. We must
distinguish relative file names from image names; that's what this
thread is about. Alternatively, we could allow only icon names with the
:app-icon argument, but this wouldn't be backward compatible.

> Being able to specify an icon with its name name according to Icon
> Naming Specification (see
> https://specifications.freedesktop.org/icon-naming-spec/latest/ar01s04.html)
> is useful since file path vary depending on the OS!

This is not true for relative file names.

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Matthias Meulien]

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

Thanks for the precision and sorry for confusion.

Matthias

Le mer. 26 juil. 2023, 15:14, Michael Albinus <michael.albinus@gmx.de> a
écrit :

> Matthias Meulien <orontee@gmail.com> writes:
>
> Hi Matthias,
>
> >> In that case, we _definitely_ must call expand-file-name, and using
> >> relative file names without that is asking for trouble.
> >
> > Why not try to be compliant with the called API?  According to the
> > Desktop Notification specification the Notify method support image path
> > being file URIs or in a freedesktop.org-compliant icon theme (see
> >
> https://specifications.freedesktop.org/notification-spec/notification-spec-latest.html#icons-and-images
> ).
>
> Sending an image_path instead of app_icon as file:/// URL is just an
> implementation detail. The `notifications-notify' API supports
> the :image-path argument if users are willing to use it.
>
> :app-icon can still be either a file name or an icon name. We must
> distinguish relative file names from image names; that's what this
> thread is about. Alternatively, we could allow only icon names with the
> :app-icon argument, but this wouldn't be backward compatible.
>
> > Being able to specify an icon with its name name according to Icon
> > Naming Specification (see
> >
> https://specifications.freedesktop.org/icon-naming-spec/latest/ar01s04.html
> )
> > is useful since file path vary depending on the OS!
>
> This is not true for relative file names.
>
> Best regards, Michael.
>

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

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> Date: Tue, 25 Jul 2023 15:59:53 +0300
> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 25/07/2023 15:01, Eli Zaretskii wrote:
> > But that's exactly the point: how can we make sure the file is "in
> > cwd"?
> 
> When it's not in cwd, wouldn't (expand-file-name file) fail to work too?

No, because expand-file-name doesn't use cwd.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi,

>> Icon is not always a file name, but can be just an icon name.
>> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>>
>> * lisp/notifications.el (notifications-notify): Don't expand-file-name
>> app-icon.
>> ---
>> Otherwise it not possible to send a notification similiar to:
>> notify-send -i multimedia-player "I am playing music"
>>
>>  lisp/notifications.el | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lisp/notifications.el b/lisp/notifications.el
>> index 984ddbec5e9..c314858d0ae 100644
>> --- a/lisp/notifications.el
>> +++ b/lisp/notifications.el
>> @@ -304,7 +304,7 @@ notifications-notify
>>  					  notifications-application-name)
>>  			      :uint32 (or replaces-id 0)
>>  			      :string (if app-icon
>> -					  (expand-file-name app-icon)
>> +                                          app-icon
>
> Thanks, but won't this break the cases where app-icon _is_ a file
> name?

Perhaps we extend the arguments of `notifications-notify'? :app-icon
should be either a string (a file name, which is expanded), or a symbol,
which denotes one of the Standard Action Icons of the Icon Naming Specification.

Your example would read in Lisp then

--8<---------------cut here---------------start------------->8---
(notifications-notify :title "I am playing music" :app-icon 'multimedia-player)
--8<---------------cut here---------------end--------------->8---

WDYT?

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Andrew Tropin <andrew@trop.in>,  emacs-devel@gnu.org
> Date: Tue, 25 Jul 2023 12:43:41 +0200
> 
> Perhaps we extend the arguments of `notifications-notify'? :app-icon
> should be either a string (a file name, which is expanded), or a symbol,
> which denotes one of the Standard Action Icons of the Icon Naming Specification.
> 
> Your example would read in Lisp then
> 
> --8<---------------cut here---------------start------------->8---
> (notifications-notify :title "I am playing music" :app-icon 'multimedia-player)
> --8<---------------cut here---------------end--------------->8---
> 
> WDYT?

SGTM, thanks.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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

On 2023-07-25 15:51, Eli Zaretskii wrote:

>> From: Michael Albinus <michael.albinus@gmx.de>
>> Cc: Andrew Tropin <andrew@trop.in>,  emacs-devel@gnu.org
>> Date: Tue, 25 Jul 2023 12:43:41 +0200
>> 
>> Perhaps we extend the arguments of `notifications-notify'? :app-icon
>> should be either a string (a file name, which is expanded), or a symbol,
>> which denotes one of the Standard Action Icons of the Icon Naming Specification.
>> 
>> Your example would read in Lisp then
>> 
>> --8<---------------cut here---------------start------------->8---
>> (notifications-notify :title "I am playing music" :app-icon 'multimedia-player)
>> --8<---------------cut here---------------end--------------->8---
>> 
>> WDYT?
>
> SGTM, thanks.

Does it make sense to introduce edge-case and differentiate between
strings and symbols?  Removing expand-file-name seems to be backward
compatible and simple solution, but if there is some good reason for
symbol/string solution - let me know, I will update the patch.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Andrew Tropin <andrew@trop.in> writes:

Hi Andrew,

>>> Perhaps we extend the arguments of `notifications-notify'? :app-icon
>>> should be either a string (a file name, which is expanded), or a symbol,
>>> which denotes one of the Standard Action Icons of the Icon Naming Specification.
>>>
>>> Your example would read in Lisp then
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> (notifications-notify :title "I am playing music" :app-icon 'multimedia-player)
>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> WDYT?
>>
>> SGTM, thanks.
>
> Does it make sense to introduce edge-case and differentiate between
> strings and symbols?  Removing expand-file-name seems to be backward
> compatible and simple solution, but if there is some good reason for
> symbol/string solution - let me know, I will update the patch.

The D-Bus interface doesn't support file names like
"~/src/emacs/etc/images/icons/hicolor/scalable/apps/emacs.svg". So we
must expand it.

The notify-send command in a shell supports this, but this is another
use case. I guess that the tilde expansion is performed by the shell.

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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

On 2023-07-25 16:47, Michael Albinus wrote:

> Andrew Tropin <andrew@trop.in> writes:
>
> Hi Andrew,
>
>>>> Perhaps we extend the arguments of `notifications-notify'? :app-icon
>>>> should be either a string (a file name, which is expanded), or a symbol,
>>>> which denotes one of the Standard Action Icons of the Icon Naming Specification.
>>>>
>>>> Your example would read in Lisp then
>>>>
>>>> --8<---------------cut here---------------start------------->8---
>>>> (notifications-notify :title "I am playing music" :app-icon 'multimedia-player)
>>>> --8<---------------cut here---------------end--------------->8---
>>>>
>>>> WDYT?
>>>
>>> SGTM, thanks.
>>
>> Does it make sense to introduce edge-case and differentiate between
>> strings and symbols?  Removing expand-file-name seems to be backward
>> compatible and simple solution, but if there is some good reason for
>> symbol/string solution - let me know, I will update the patch.
>
> The D-Bus interface doesn't support file names like
> "~/src/emacs/etc/images/icons/hicolor/scalable/apps/emacs.svg". So we
> must expand it.
>
> The notify-send command in a shell supports this, but this is another
> use case. I guess that the tilde expansion is performed by the shell.

Make sense, sent v2 with symbol value support to the root of the thread.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Michael Albinus]

Andrew Tropin <andrew@trop.in> writes:

Hi Andrew,

> Make sense, sent v2 with symbol value support to the root of the thread.

Thanks. Please adapt also etc/NEWS and lispref/os.texi.

Best regards, Michael.

Re: [PATCH] notifications: Don't expand-file-name app-icon.

[Andrew Tropin]

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

On 2023-07-26 09:13, Michael Albinus wrote:

> Andrew Tropin <andrew@trop.in> writes:
>
> Hi Andrew,
>
>> Make sense, sent v2 with symbol value support to the root of the thread.
>
> Thanks. Please adapt also etc/NEWS and lispref/os.texi.
>
> Best regards, Michael.

Oki, sent v3! :)

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH v3] notifications: Allow to use Icon Naming Specification for app-icon

[Andrew Tropin]

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

Icon is not always a file name, but can be just an icon name.
https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

* lisp/notifications.el (notifications-notify): Allow to use Icon
Naming Specification for app-icon.
---
Changes since v2: updated NEWS and os.texi.
 doc/lispref/os.texi   | 3 +++
 etc/NEWS              | 7 +++++++
 lisp/notifications.el | 8 +++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 4bcc9d5fea6..efa89fa387c 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -2885,6 +2885,9 @@ Desktop Notifications
 @item :app-icon @var{icon-file}
 The file name of the notification icon.  If set to @code{nil}, no icon
 is displayed.  The default is @code{notifications-application-icon}.
+If value is a string @code{expand-file-name} will be applied, if value
+is a symbol it will be used as is (helpful when using Icon Naming
+Specification).
 
 @item :actions (@var{key} @var{title} @var{key} @var{title} ...)
 A list of actions to be applied.  @var{key} and @var{title} are both
diff --git a/etc/NEWS b/etc/NEWS
index 5883b4df2a7..e1a5cbf0340 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -548,6 +548,13 @@ Similarly to buffer restoration by Desktop, 'recentf-mode' checking
 of the accessibility of remote files can now time out if
 'remote-file-name-access-timeout' is set to a positive number.
 
+** Notifications
+
++++
+*** Allow to use Icon Naming Specification for app-icon.
+Symbol can be used as a value for :app-icon to provide icon name
+without specifying a file.
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/notifications.el b/lisp/notifications.el
index 984ddbec5e9..13353264de3 100644
--- a/lisp/notifications.el
+++ b/lisp/notifications.el
@@ -137,6 +137,9 @@ notifications-notify
  :app-icon       The notification icon.
                  Default is `notifications-application-icon'.
                  Set to nil if you do not want any icon displayed.
+                 If value is a string expand-file-name will be
+                 applied, if value is a symbol it will be used as
+                 is (helpful when using Icon Naming Specification).
  :actions        A list of actions in the form:
                    (KEY TITLE KEY TITLE ...)
                  where KEY and TITLE are both strings.
@@ -304,7 +307,10 @@ notifications-notify
 					  notifications-application-name)
 			      :uint32 (or replaces-id 0)
 			      :string (if app-icon
-					  (expand-file-name app-icon)
+					  (if (stringp app-icon)
+                                              (expand-file-name app-icon)
+                                            ;; Convert symbol to string
+                                            (symbol-name app-icon))
 					;; If app-icon is nil because user
 					;; requested it to be so, send the
 					;; empty string
-- 
2.41.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v3] notifications: Allow to use Icon Naming Specification for app-icon

[Eli Zaretskii]

> From: Andrew Tropin <andrew@trop.in>
> Date: Wed, 26 Jul 2023 15:59:48 +0400
> 
> Icon is not always a file name, but can be just an icon name.
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

Thanks, but there's too much passive voice in the documentation parts,
please try to improve that.  For example:

> +If value is a string @code{expand-file-name} will be applied, if value
> +is a symbol it will be used as is (helpful when using Icon Naming
> +Specification).

I would rephrase (which also clarifies the stuff a bit):

  If the value is a string, the function interprets it as a file name
  and converts to absolute by using @code{expand-file-name}; if it is
  a symbol, the function will use its name (which is useful when using
  the Icon Naming Specification).

> +*** Allow to use Icon Naming Specification for app-icon.
> +Symbol can be used as a value for :app-icon to provide icon name
> +without specifying a file.

   "You can use a symbol as the value ..."

Re: [PATCH v3] notifications: Allow to use Icon Naming Specification for app-icon

[Michael Albinus]

Andrew Tropin <andrew@trop.in> writes:

Hi Andrew,

Eli did comment already (I agree with him). Just some few further nits.

> Icon is not always a file name, but can be just an icon name.
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>
> diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
> index 4bcc9d5fea6..efa89fa387c 100644
> --- a/doc/lispref/os.texi
> +++ b/doc/lispref/os.texi
> @@ -2885,6 +2885,9 @@ Desktop Notifications
>  @item :app-icon @var{icon-file}
>  The file name of the notification icon.  If set to @code{nil}, no icon
>  is displayed.  The default is @code{notifications-application-icon}.
> +If value is a string @code{expand-file-name} will be applied, if value
> +is a symbol it will be used as is (helpful when using Icon Naming
> +Specification).

Please add the URL of the spec above, perhaps as @footnote. Furthermore,
icon names are not restricted to the ones from the Icon Naming
Specification, you can use any icon name which is in the user's current icon
theme. The specification tells you what shall be offered on any system. For
example, you could say now

  (notifications-notify :title "The thunderbird icon" :app-icon 'thunderbird)

I have installed thunderbird, of course. And I don't need to care that
the corresponding icon files are installed at /usr/share/icons/**/apps/thunderbird.png

> diff --git a/etc/NEWS b/etc/NEWS
> index 5883b4df2a7..e1a5cbf0340 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -548,6 +548,13 @@ Similarly to buffer restoration by Desktop, 'recentf-mode' checking
>  of the accessibility of remote files can now time out if
>  'remote-file-name-access-timeout' is set to a positive number.
>
> +** Notifications
> +
> ++++
> +*** Allow to use Icon Naming Specification for app-icon.
> +Symbol can be used as a value for :app-icon to provide icon name
> +without specifying a file.

Say ':app-icon'.

I found your initial example funny, maybe add it here:

  (notifications-notify
    :title "I am playing music" :app-icon 'multimedia-player)

Best regards, Michael.

Re: [PATCH v3] notifications: Allow to use Icon Naming Specification for app-icon

[Andrew Tropin]

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

On 2023-07-26 19:46, Michael Albinus wrote:

> Andrew Tropin <andrew@trop.in> writes:
>
> Hi Andrew,
>
> Eli did comment already (I agree with him). Just some few further nits.
>
>> Icon is not always a file name, but can be just an icon name.
>> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>>
>> diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
>> index 4bcc9d5fea6..efa89fa387c 100644
>> --- a/doc/lispref/os.texi
>> +++ b/doc/lispref/os.texi
>> @@ -2885,6 +2885,9 @@ Desktop Notifications
>>  @item :app-icon @var{icon-file}
>>  The file name of the notification icon.  If set to @code{nil}, no icon
>>  is displayed.  The default is @code{notifications-application-icon}.
>> +If value is a string @code{expand-file-name} will be applied, if value
>> +is a symbol it will be used as is (helpful when using Icon Naming
>> +Specification).
>
> Please add the URL of the spec above, perhaps as @footnote. Furthermore,
> icon names are not restricted to the ones from the Icon Naming
> Specification, you can use any icon name which is in the user's current icon
> theme. The specification tells you what shall be offered on any system. For
> example, you could say now
>
>   (notifications-notify :title "The thunderbird icon" :app-icon 'thunderbird)
>
> I have installed thunderbird, of course. And I don't need to care that
> the corresponding icon files are installed at /usr/share/icons/**/apps/thunderbird.png
>
>> diff --git a/etc/NEWS b/etc/NEWS
>> index 5883b4df2a7..e1a5cbf0340 100644
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -548,6 +548,13 @@ Similarly to buffer restoration by Desktop, 'recentf-mode' checking
>>  of the accessibility of remote files can now time out if
>>  'remote-file-name-access-timeout' is set to a positive number.
>>
>> +** Notifications
>> +
>> ++++
>> +*** Allow to use Icon Naming Specification for app-icon.
>> +Symbol can be used as a value for :app-icon to provide icon name
>> +without specifying a file.
>
> Say ':app-icon'.
>
> I found your initial example funny, maybe add it here:
>
>   (notifications-notify
>     :title "I am playing music" :app-icon 'multimedia-player)
>
> Best regards, Michael.

Addressed points mentioned in both emails and sumbitted v4.

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Andrew Tropin]

Icon is not always a file name, but can be just an icon name.
https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

* lisp/notifications.el (notifications-notify): Allow to use Icon
Naming Specification for app-icon.
---
 doc/lispref/os.texi   |  7 +++++++
 etc/NEWS              | 10 ++++++++++
 lisp/notifications.el | 11 ++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index 4bcc9d5fea6..ebedfe82087 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -2885,6 +2885,13 @@ Desktop Notifications
 @item :app-icon @var{icon-file}
 The file name of the notification icon.  If set to @code{nil}, no icon
 is displayed.  The default is @code{notifications-application-icon}.
+If the value is a string, the function interprets it as a file name
+and converts to absolute by using @code{expand-file-name}; if it is a
+symbol, the function will use its name (which is useful when using the
+Icon Naming Specification @footnote{For more information about icon
+naming convention see
+@uref{https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html,
+Icon Naming Specification}}).
 
 @item :actions (@var{key} @var{title} @var{key} @var{title} ...)
 A list of actions to be applied.  @var{key} and @var{title} are both
diff --git a/etc/NEWS b/etc/NEWS
index 5883b4df2a7..51c7fe6a363 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -548,6 +548,16 @@ Similarly to buffer restoration by Desktop, 'recentf-mode' checking
 of the accessibility of remote files can now time out if
 'remote-file-name-access-timeout' is set to a positive number.
 
+** Notifications
+
++++
+*** Allow to use Icon Naming Specification for app-icon
+You can use a symbol as the value for ':app-icon' to provide icon name
+without specifying a file, like this:
+
+  (notifications-notify
+    :title "I am playing music" :app-icon 'multimedia-player)
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/notifications.el b/lisp/notifications.el
index 984ddbec5e9..a694b38e52e 100644
--- a/lisp/notifications.el
+++ b/lisp/notifications.el
@@ -137,6 +137,12 @@ notifications-notify
  :app-icon       The notification icon.
                  Default is `notifications-application-icon'.
                  Set to nil if you do not want any icon displayed.
+                 If the value is a string, the function
+                 interprets it as a file name and converts to
+                 absolute by using `expand-file-name'; if it is a
+                 symbol, the function will use its name (which is
+                 useful when using the Icon Naming
+                 Specification).
  :actions        A list of actions in the form:
                    (KEY TITLE KEY TITLE ...)
                  where KEY and TITLE are both strings.
@@ -304,7 +310,10 @@ notifications-notify
 					  notifications-application-name)
 			      :uint32 (or replaces-id 0)
 			      :string (if app-icon
-					  (expand-file-name app-icon)
+					  (if (stringp app-icon)
+                                              (expand-file-name app-icon)
+                                            ;; Convert symbol to string
+                                            (symbol-name app-icon))
 					;; If app-icon is nil because user
 					;; requested it to be so, send the
 					;; empty string
-- 
2.41.0

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Michael Albinus]

Andrew Tropin <andrew@trop.in> writes:

> Icon is not always a file name, but can be just an icon name.
> https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html

Thanks. Eli, if you don't have further comments, I would push this to master.

Best regards, Michael.

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: emacs-devel@gnu.org
> Date: Thu, 27 Jul 2023 13:49:15 +0200
> 
> Andrew Tropin <andrew@trop.in> writes:
> 
> > Icon is not always a file name, but can be just an icon name.
> > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
> 
> Thanks. Eli, if you don't have further comments, I would push this to master.

Please do, and thanks.

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Icon is not always a file name, but can be just an icon name.
>> > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>>
>> Thanks. Eli, if you don't have further comments, I would push this to master.
>
> Please do, and thanks.

Done as commit 42a911c61e67caa807750cd40887b729f09aaf52.

Best regards, Michael.

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Andrew Tropin]

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

On 2023-07-27 16:59, Michael Albinus wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> > Icon is not always a file name, but can be just an icon name.
>>> > https://specifications.freedesktop.org/icon-naming-spec/icon-naming-spec-latest.html
>>>
>>> Thanks. Eli, if you don't have further comments, I would push this to master.
>>
>> Please do, and thanks.
>
> Done as commit 42a911c61e67caa807750cd40887b729f09aaf52.
>
> Best regards, Michael.

Thank you!

-- 
Best regards,
Andrew Tropin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Michael Albinus]

Andrew Tropin <andrew@trop.in> writes:

Hi Andrew,

>>>> Thanks. Eli, if you don't have further comments, I would push this to master.
>>>
>>> Please do, and thanks.
>>
>> Done as commit 42a911c61e67caa807750cd40887b729f09aaf52.
>
> Thank you!

You're welcome.

I've digged the documentation further, and it seems like we've overlooked,
that XDG icon names are possible already, via the :image-path argument:

(notifications-notify :title "I am playing music" :image-path "multimedia-player")

So the patch wasn't necessary. I'm a little bit undecided, whether we
shall revert it. Eli?

Best regards, Michael.

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Fri, 28 Jul 2023 09:24:58 +0200
> 
> Andrew Tropin <andrew@trop.in> writes:
> 
> >> Done as commit 42a911c61e67caa807750cd40887b729f09aaf52.
> >
> > Thank you!
> 
> You're welcome.
> 
> I've digged the documentation further, and it seems like we've overlooked,
> that XDG icon names are possible already, via the :image-path argument:
> 
> (notifications-notify :title "I am playing music" :image-path "multimedia-player")
> 
> So the patch wasn't necessary. I'm a little bit undecided, whether we
> shall revert it. Eli?

If you are unhappy with leaving it, I don't object to reverting.  But
if it currently causes no harm, perhaps leave it for now, and let's
see if it's useful for some use cases.

Re: [PATCH v4] notifications: Allow to use Icon Naming Specification for app-icon

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> So the patch wasn't necessary. I'm a little bit undecided, whether we
>> shall revert it. Eli?
>
> If you are unhappy with leaving it, I don't object to reverting.  But
> if it currently causes no harm, perhaps leave it for now, and let's
> see if it's useful for some use cases.

I'm not unhappy with it, the patch does what it is intended. The only
worry I have is that the interface is now inconsistent. But this is
minor, and if you can live with it, so do I.

I'm just unhappy with myself, that I haven't seen it earlier. Nothing
anybody else needs to care,

Best regards, Michael.