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

Jump to bottom

ranger: add video and pdf preview options #70432

Closed
wants to merge 1 commit into from
Closed

ranger: add video and pdf preview options #70432

wants to merge 1 commit into from

Conversation

mtrsk
Copy link
Member

@mtrsk mtrsk commented Oct 5, 2019

Motivation for this change

An attempt to add video & pdf previews to ranger via the nix derivation out of the box

Things done
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@ofborg ofborg bot requested a review from toonn October 5, 2019 01:32
@teto
Copy link
Member

teto commented Oct 5, 2019

I would love to see it installable out of the box. Have you checked the impact on closure size though ? It might be best to provide a module or a wrapper in charge of generating the scope.sh. Would prevent useless rebuilds/caching.

toonn
toonn requested changes Oct 5, 2019
View reviewed changes
Copy link
Contributor

@toonn toonn left a comment

Choose a reason for hiding this comment

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

One necessary change, the other more a request pending addition to scope.sh, all that'd have to change is to allow either poppler or mupdf as dependency.

pkgs/applications/misc/ranger/default.nix
@@ -1,9 +1,13 @@
{ stdenv, lib, fetchFromGitHub, python3Packages, file, less, highlight
, imagePreviewSupport ? true, w3m ? null}:
, imagePreviewSupport ? true, w3m ? null
, pdfPreviewSupport ? true, poppler_utils ? null
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be great if you could add support for mutool too, we already do this for text previews. I'll open an issue on this over at ranger/ranger.

pkgs/applications/misc/ranger/default.nix
@@ -52,6 +58,12 @@ python3Packages.buildPythonApplication rec {
# give image previews out of the box when building with w3m
substituteInPlace ranger/config/rc.conf \
--replace "set preview_images false" "set preview_images true"
'' + optionalString pdfPreviewSupport ''
# edits scope.sh and enables video previews via ffmpegthumbnailer
Copy link
Contributor

Choose a reason for hiding this comment

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

You swapped the comments around it seems.

Ma27
Ma27 reviewed Oct 5, 2019
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

Diff seems fine overall. If the closure size impact is too high, we could also add two packages (e.g. ranger and ranger-full).

Apart from that I agree with @teto, the optional features should be moved to a wrapper for less rebuilds :)

teto
teto requested changes Oct 5, 2019
View reviewed changes
Copy link
Member

@teto teto left a comment

Choose a reason for hiding this comment

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

Makes more sense to go for a module straightaway. Maybe even in home-manager if that's what you use ?

@mtrsk
Copy link
Member Author

mtrsk commented Oct 5, 2019

Makes more sense to go for a module straightaway. Maybe even in home-manager if that's what you use ?

A pure nix way to config ranger would be awesome, but how would a module for it look like?

Something like this?

programs.ranger = {
  enable = true;
  imagePreviewSupport = true;
  pdfPreviewSupport = true;
  videoPreviewSupport = true;
  extraConfig = {
    commands = <...>;
    rc = <...>;
    rifle = <...>;
    scope = <...>;
  };
};

@teto
Copy link
Member

teto commented Oct 6, 2019

You don't have to cover all the configurations items from the start. Just enable and maybe preview= { image .... video ... pdf }; would be enough for a start. Looking at ranger
s manpage, seems like the configuration could be implemented as a plugin (this way no need to patch the default files).

@mtrsk
Copy link
Member Author

mtrsk commented Oct 6, 2019

You don't have to cover all the configurations items from the start. Just enable and maybe preview= { image .... video ... pdf }; would be enough for a start. Looking at ranger
s manpage, seems like the configuration could be implemented as a plugin (this way no need to patch the default files).

I'll take a look at this and other reviews an try to write a wrapper as to improve my nix-fu.

@Ma27
Copy link
Member

Ma27 commented Oct 6, 2019

Tbh I'm not sure if that's a good idea: programs.x modules are perfectly fine for system-wide configuration but in this case we only use those options to configure the package itself. In this case it might be even harmful as this makes it hard for non-NixOS users (e.g. folks using Nix on Ubuntu) to actually use those features from ranger.

@teto
Copy link
Member

teto commented Oct 7, 2019

the changes modify the configuration, we shouldn't have to reinstall the package just to modify the config.
Maybe a wrapper would be better but they are not amazingly user friendly (compared to modules that appear in man configuration.nix for instance).

@mtrsk you can look at neovim for a wrapper example.
@Ma27 Regarding your remark, maybe it would be best to prepare a home-manager module then ? @mtrsk do you use home-manager (https://github.com/rycee/home-manager/) ? I so wish both projects could converge.

@mtrsk
Copy link
Member Author

mtrsk commented Oct 7, 2019

@mtrsk you can look at neovim for a wrapper example.

Thanks, I'll take a look at the neovim wrapper (for learning purposes).

the changes modify the configuration, we shouldn't have to reinstall the package just to modify the config.
Maybe a wrapper would be better but they are not amazingly user friendly (compared to modules that appear in man configuration.nix for instance).
@Ma27 Regarding your remark, maybe it would be best to prepare a home-manager module then ? @mtrsk do you use home-manager (https://github.com/rycee/home-manager/) ? I so wish both projects could converge.

It seems it's best to close this PR, as adding more options seems to bring more harm than benefits (once we take into account user friendliness). Making a home-manager module for ranger makes more sense.

@mtrsk mtrsk closed this Oct 7, 2019
@teto
Copy link
Member

teto commented Oct 8, 2019

You can ping me on irc or at home-manager should you have any question, I would really like such a module ;)

@mtrsk mtrsk deleted the ranger-add-options branch October 10, 2019 21:39
@JohnAZoidberg
Copy link
Member

Has any progress of the home-manager configuration been made?

@mtrsk
Copy link
Member Author

mtrsk commented Jan 3, 2020

@JohnAZoidberg I've never had the time to tackle this

@teto teto mentioned this pull request Jul 30, 2020
Closed
9 tasks
@teto teto mentioned this pull request Oct 14, 2021
Closed
12 tasks
@teto teto mentioned this pull request Oct 14, 2022
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 left review comments

@teto teto teto requested changes

@toonn toonn toonn requested changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mtrsk @teto @Ma27 @JohnAZoidberg @toonn