Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plex-media-player: remove custom desktopItem #60444

Merged

Conversation

angristan
Copy link
Member

@angristan angristan commented Apr 29, 2019

Motivation for this change

Fixes #60437: plex-media-player comes with a working desktop file, so there is no need to add our own custom one unless we remove the original. Both seem to work fine, so let's keep it simple.

Things done
  • Removed makeDesktopItem dependency
  • Removed creation and copy of the custom desktop item desktopItem (plex-media-player.desktop)
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@alexarice
Copy link
Contributor

I have tested this and can confirm it builds and had two desktop items before and now has 1. I have not tested any binaries

@JohnAZoidberg
Copy link
Member

JohnAZoidberg commented Apr 30, 2019

Great! The upstream desktop file is more fully featured anyways :)
Btw. if you write Fixes #60444 instead of See #60444 GitHub will automatically close the issue once this PR is merged.

@jtojnar
Copy link
Contributor

jtojnar commented Apr 30, 2019

Is installing the icon necessary? I would expect them to install it on their own when they added a desktop file. Are the paths in the desktop file correct? Sometimes project hardcode /usr/share there.

@angristan
Copy link
Member Author

Indeed, the install part is not needed, the icon is fine even after removing that part.

There is no path in the desktop file:

stanislas@nixpsla ~/nixpkgs> cat result/share/applications/plexmediaplayer.desktop
[Desktop Entry]
Version=1.0
Name=Plex Media Player
GenericName=Media Player
Comment=View your media
Exec=plexmediaplayer --fullscreen --tv
Icon=plexmediaplayer
Terminal=false
Type=Application
Categories=AudioVideo;Video;Player;TV;

Actions=TV;DesktopF;DesktopW;

[Desktop Action TV]
Name=TV
Exec=plexmediaplayer --fullscreen --tv

[Desktop Action DesktopF]
Name=Desktop [Fullscreen]
Exec=plexmediaplayer --fullscreen --desktop

[Desktop Action DesktopW]
Name=Desktop [Windowed]
Exec=plexmediaplayer --windowed --desktop

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the desktop item and all icons and actions work.

Please squash your commits accordingly and we should be good to merge 😄

one to fix desktopItem, another to not copy icon (or whatever)

@angristan angristan force-pushed the plex-media-player/fix-desktop-icons branch from ed942fc to 5f1dc02 Compare April 30, 2019 21:17
@angristan
Copy link
Member Author

angristan commented Apr 30, 2019

It should be good now, I hope 🙂

@worldofpeace worldofpeace merged commit 8d212a5 into NixOS:master Apr 30, 2019
@worldofpeace
Copy link
Contributor

✨ Thank you @angristan for contributing

@angristan angristan deleted the plex-media-player/fix-desktop-icons branch April 30, 2019 21:40
@angristan
Copy link
Member Author

Thanks everyone for helping out! 🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plex-media-player has 2 .desktop files
5 participants