r-darwish / topgrade

Upgrade everything
GNU General Public License v3.0
3.36k stars 161 forks source link

Desktop notifications show step in summary #918

Closed Spaceface16518 closed 2 years ago

Spaceface16518 commented 2 years ago

What did you expect to happen?

Notification summary is "Topgrade".

The macos section of notify_desktop seems to imply this intent as well, listing "Topgrade" as the summary and the body as the message.

https://github.com/r-darwish/topgrade/blob/5d168bbf0d99040eed101cdc3a83f958781193d0/src/terminal.rs#L91-L99

What actually happened?

Topgrade is not mentioned, and step name is given as summary.

This is clear from the linux section of notify_desktop, where "Topgrade" is given as the app name (imo it should be "topgrade", just like on macos) and the message is given as the first positional argument to notify-send.

https://github.com/r-darwish/topgrade/blob/5d168bbf0d99040eed101cdc3a83f958781193d0/src/terminal.rs#L101-L110

From notify-send --help

Usage:
  notify-send [OPTION…] <SUMMARY> [BODY] - create a notification
Application Options:
  -a, --app-name=APP_NAME           Specifies the app name for the icon

As is shown here, the summary is the first positional argument and the body is the second positional argument.

Also: Why aren't we using notify-rust for linux notifications? The library has linux support, right?

Additional details

r-darwish commented 2 years ago

The reason for not using notify-rust is to avoid having a dynamic link to a library which isn't present in every Linux distribution.