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

mpvScripts.mpv-image-viewer: init at 0-unstable-2023-03-03 #347323

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

uninsane
Copy link
Contributor

@uninsane uninsane commented Oct 8, 2024

packages mpv-image-viewer. these are all independent, managed under one umbrella project. example usage:

$ nix-shell -E 'mpv.override { scripts = [ mpvScripts.mpv-image-viewer.image-positioning ]; }'
[nix-shell]$ echo 'CTRL+MBTN_LEFT script-binding drag-to-pan' | mpv --input-conf=/dev/stdin --pause 'https://github.com/NixOS/nixos-artwork/blob/master/wallpapers/NixOS-Gradient-grey.png?raw=true'

# zoom the image with default mpv controls (control + mouse wheel)
# and observe that holding control + left mouse button now allows to pan when zoomed in,
# whereas stock `mpv` does not implement any panning function

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Oct 8, 2024
@uninsane uninsane marked this pull request as draft October 31, 2024 00:09
@uninsane uninsane force-pushed the pr-mpv-image-viewer branch from f67dd3f to 2754139 Compare October 31, 2024 00:11
@uninsane uninsane marked this pull request as ready for review October 31, 2024 00:11
@uninsane uninsane added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Oct 31, 2024
@uninsane uninsane added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 25, 2024
@nbraud nbraud added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 27, 2024
@nbraud nbraud added the 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed label Nov 27, 2024
@nbraud nbraud mentioned this pull request Nov 27, 2024
8 tasks
@nbraud
Copy link
Contributor

nbraud commented Nov 27, 2024

I merged #359625 into this PR branch, locally, and can confirm everything works (and the generic tests pass)

@FliegendeWurst, shall I push the merge here so the merge conflict with that other PR is already resolved, and this can be merged right after #359625 ?

PS: sorry, I meant to address this to the PR's author 😅

@FliegendeWurst
Copy link
Member

You can do that, or just rebase after that one is merged. It will not delay this PR much either way

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Nov 27, 2024
@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 28, 2024
@nbraud nbraud force-pushed the pr-mpv-image-viewer branch from 63e15cc to b0e6d91 Compare November 28, 2024 12:39
Copy link
Contributor

@nbraud nbraud left a comment

Choose a reason for hiding this comment

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

Rebased now that the blocker was merged.

@nbraud nbraud added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed 2.status: merge conflict This PR has merge conflicts with the target branch labels Nov 28, 2024
@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 28, 2024
@nbraud
Copy link
Contributor

nbraud commented Nov 28, 2024

Sorry for missing all of that on first review, I think I just stopped when I noticed the generic tests were missing 😅

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 29, 2024
@uninsane uninsane force-pushed the pr-mpv-image-viewer branch from b0e6d91 to 9934f46 Compare November 29, 2024 13:47
@uninsane
Copy link
Contributor Author

removed the scope, copied descriptions from upstream's readme (equalizer has no description): https://github.com/occivink/mpv-image-viewer/blob/master/README.md#scripts

feel free to suggest alternate descriptions if that's important to you. they're slightly long for typical nixpkgs descriptions, but not enough that it bothered me personally.

@nbraud
Copy link
Contributor

nbraud commented Nov 29, 2024

feel free to suggest alternate descriptions if that's important to you.

I'm not terribly picky here, I just felt having per-script descriptions would be important for discoverability (e.g. on search.nixos.org) and for users to quickly see what the script is supposed to do (when deciding whether to try it)

they're slightly long for typical nixpkgs descriptions, but not enough that it bothered me personally.

They seem fine to me as well; FYI, for cases where it is an issue I'd keep it to a single, short sentence, and plonk the long-form explanation in meta.longDescription.

@nbraud
Copy link
Contributor

nbraud commented Nov 29, 2024

Addressed merge conflict

@nbraud
Copy link
Contributor

nbraud commented Nov 29, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 347323


aarch64-darwin

✅ 7 packages built:
  • mpvScripts.mpv-image-viewer.detect-image
  • mpvScripts.mpv-image-viewer.equalizer
  • mpvScripts.mpv-image-viewer.freeze-window
  • mpvScripts.mpv-image-viewer.image-positioning
  • mpvScripts.mpv-image-viewer.minimap
  • mpvScripts.mpv-image-viewer.ruler
  • mpvScripts.mpv-image-viewer.status-line

can be used like:

```nix
mpv.override { scripts = [ mpvScripts.mpv-image-viewer.image-positioning ]; }
```

Co-authored-by: Arne Keller <2012gdwu+github@posteo.de>
@nbraud
Copy link
Contributor

nbraud commented Nov 29, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 347323


x86_64-linux

✅ 7 packages built:
  • mpvScripts.mpv-image-viewer.detect-image
  • mpvScripts.mpv-image-viewer.equalizer
  • mpvScripts.mpv-image-viewer.freeze-window
  • mpvScripts.mpv-image-viewer.image-positioning
  • mpvScripts.mpv-image-viewer.minimap
  • mpvScripts.mpv-image-viewer.ruler
  • mpvScripts.mpv-image-viewer.status-line

@uninsane uninsane force-pushed the pr-mpv-image-viewer branch from 49bf03a to 06cc9e7 Compare November 29, 2024 16:12
@uninsane
Copy link
Contributor Author

squashed the commits

@nbraud
Copy link
Contributor

nbraud commented Nov 29, 2024

Eval still passes after merge (on the new GH Action setup) so I'll go ahead without waiting for @ofborg

squashed the commits

FWIW, I find a squash-merge preferable, since it avoids re-triggering CI etc. :3

@nbraud nbraud merged commit 4a98c23 into NixOS:master Nov 29, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants