tidal-hifi: improve quality of the desktop entry#419474
tidal-hifi: improve quality of the desktop entry#419474roberth merged 2 commits intoNixOS:masterfrom
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
Gerg-L
left a comment
Gerg-L
left a comment
There was a problem hiding this comment.
I was trying to mirror upstreams desktop entry... This is fine though.
It only needs to be one commit
nixpkgs-ci
bot
added
12.approvals: 1
spikespaz
force-pushed
the
u/jacob/tidal-hifi/improve-desktop-entry
branch
from
2594f48 to
46a2557
Compare
This reorders all of the attributes to follow the same order as the Freedesktop.org specification. <https://specifications.freedesktop.org/desktop-entry-spec/1.4/recognized-keys.html>
This patch improves the desktop entry for `tidal-hifi` because I noticed it looked a little funny. Summary: - Changed the file name to `tidal-hifi` (to remove the whitespace). - Change the `desktopName` (aesthetic name) to "TIDAL Hi-Fi". - Change the generic name to "Music Player" because "TIDAL Hi-Fi" is not generic. - Make `comment` and `exec` (and `name`) follow the values used for `meta` attributes to reduce maintenance overhead slightly. - Fix capitalization errors in `meta.description`. - Change the `categories` slightly to be more similar to other streaming service programs. - Re-ordered the attributes to make it easier to read along with <https://specifications.freedesktop.org/desktop-entry-spec/1.4/recognized-keys.html>. Each commit has a justification in its summary. tidal-hifi: remove type from desktop entry It already has a default value of `"Application"` if left unspecified. It's also not included in the example for this build helper. <https://github.com/NixOS/nixpkgs/blob/fd751b2cae7edc93d8f4bf11916169aae84964aa/doc/build-helpers/trivial-build-helpers.chapter.md?plain=1#L219-L221> tidal-hifi: change desktop entry filename to self.pname The `name` argument for this function corresponds to the filename in the Nix store, it is not the human-friendly display name. Some desktop entries use the display name as the filename, but I hate spaces in filenames if they aren't necessary. <https://github.com/NixOS/nixpkgs/blob/fd751b2cae7edc93d8f4bf11916169aae84964aa/doc/build-helpers/trivial-build-helpers.chapter.md?plain=1#L215-L217> tidal-hifi: change desktop entry display name to TIDAL Hi-Fi The `desktopName` argument to this function corresponds with the `Name` key in the generated `.desktop` file, which is the display name for the human. <https://github.com/NixOS/nixpkgs/blob/fd751b2cae7edc93d8f4bf11916169aae84964aa/doc/build-helpers/trivial-build-helpers.chapter.md?plain=1#L223-L225> tidal-hifi: change desktop entry generic name to Music Player Judging by the example given in the Nixpkgs documentation, this generic name should be a short description of what the program does. In this case, it is a "Music Player". <https://github.com/NixOS/nixpkgs/blob/fd751b2cae7edc93d8f4bf11916169aae84964aa/doc/build-helpers/trivial-build-helpers.chapter.md?plain=1#L247> tidal-hifi: desktop entry comment same as meta.description There's no reason to say the same thing twice. Also fixed the capitalization of "Electron" and "Widevine" since they are proper nouns. tidal-hifi: change desktop entry exec to meta.mainProgram The `meta.mainProgram` attribute is now considered the source of truth if the binary name changes upstream, it only has to be updated in one position for this package. tidal-hifi: change desktop entry categories to match other music players Removed `Video`, kept `Network`, now it matches Spotify's desktop entry more closely.
spikespaz
force-pushed
the
u/jacob/tidal-hifi/improve-desktop-entry
branch
from
46a2557 to
29b8657
Compare
nix-owners
bot
requested review from
GaetanLepage,
PerchunPak,
hsjobeki,
infinisil,
khaneliman,
mrcjkb and
roberth
PerchunPak
removed request for
GaetanLepage,
PerchunPak,
hsjobeki,
infinisil,
khaneliman,
mrcjkb and
roberth
| desktopName = "tidal-hifi"; | ||
| genericName = "TIDAL Hi-Fi"; | ||
| comment = "The web version of listen.tidal.com running in electron with hifi support thanks to widevine."; | ||
| name = self.pname; |
There was a problem hiding this comment.
If I'm not mistaken, it is discouraged to re-use pname in the derivation. Please, keep the hard-coded name here.
There was a problem hiding this comment.
I'm afraid I need to ask you to cite a source (or reason it out). I'm not sure I recall seeing that advice anywhere.
There was a problem hiding this comment.
That comment you linked covers a very specific circumstance. That is not this circumstance. In this case, that's the reason to use self.pname recursively, where it is not a footgun.
There was a problem hiding this comment.
name(String)The name of the desktop file in the Nix store.
Seems like a match.
It is not the Name field of the desktop entry, so it seems quite inconsequential.
LGTM.
| comment = self.meta.description; | ||
| icon = "tidal-hifi"; | ||
| startupNotify = true; | ||
| exec = self.meta.mainProgram; |
There was a problem hiding this comment.
Here I'm less sure... Maybe it's better to switch to meta.mainProgram indeed... Not sure though.
There was a problem hiding this comment.
Maybe the advice you recall is that using attributes via rec is bad, given the wide scope that is a package definition? Certainly, if someone overrides this package and changes the meta.mainProgram because the package's executable has been renamed, they'd appreciate the recursion.
Same goes for pname in fact, if this definition is used as the base for a fork, and pname is changed via overrideAttrs, you'd want the desktop entry's filename to not conflict with the upstream one. Doing the same thing with a let binding only accomplishes the second purpose, the single source of truth. For overrides, that benefit would be lost.
5 participants
This PR improves the desktop entry for
tidal-hifibecause I noticed it looked a little funny.Summary:
tidal-hifi(to remove the whitespace).desktopName(aesthetic name) to "TIDAL Hi-Fi".commentandexec(andname) follow the values used formetaattributes to reduce maintenance overhead slightly.meta.description.categoriesslightly to be more similar to other streaming service programs.Each commit has a justification in its summary.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.