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

Suggested fixes for key-handler #1451

Open
RetroViking opened this issue Jan 27, 2025 · 5 comments
Open

Suggested fixes for key-handler #1451

RetroViking opened this issue Jan 27, 2025 · 5 comments

Comments

@RetroViking
Copy link

RetroViking commented Jan 27, 2025

One of the things that make nsxiv really cool is the ability to add whatever features you want to it, via the key-handler script. The repo has a key-handler with nice features, but some of them are currently broken. So I opened this issue to suggest some fixes.

File operations (copy and move) are broken.

~ $ destdir="$(sed "s/#.*$//;/^\s*$/d" ${XDG_CONFIG_HOME:-$HOME/.config}/shell/bm-dirs | awk '{print $2}' | dmenu -l 20 -i -p "Copy file(s) to where?" | sed "s|~|$HOME|g")"
~ $ echo "$destdir"
$HOME

As you can see, "$destdir" contains the literal string $HOME rather than the expanded version, e.g. /home/user. This triggers the "not a directory" notification even though valid paths are given.

One workaround for this is to expand the variable after the selection, using eval:

destdir=$(eval printf '%s\\n' \"$destdir\")

The way this is quoted is important, because 'eval' processes quotes differently.

echo may be used instead of printf '%s\\n'. The advantage of echo is that it will expand both ~ and $HOME, making the sed command unnecessary. However, using echo is not considered good practice.

The image rotation commands are broken
@aartoni recently created #1449 with the relevant fix.

The commands for copying the filename and file path are not properly implemented
In theory, the y option should copy just the file name (e.g. image.png), while the Y option should copy the entire file path (e.g. /home/user/Pictures/image.png). However, in practice both commands will copy the entire file path if nsxiv is opened with a full path, e.g.

nsxiv /home/user/Pictures/image.png

There is no option to copy the image itself to clipboard
The nsxiv repo's example key-handler script has a command for copying an image to the clipboard and pasting it somewhere else, e.g. in a forum post or an email. Although I don't frequently use this, it seems like a nice thing to have. This repo could implement it like this:

"x") xclip -selection clipboard -target image/png "$file" && notify-send "Image copied to clipboard" & ;;

EDIT:

Here is an example implementation that:

  • Fixes copying and moving files, in bulk or one at a time.
  • Overwrite protection
  • Does not force every single case to run in a loop (otherwise confirmation prompts may run multiple times, once per each loop, instead of once)
#!/bin/sh
# Some commands may only make sense with a single image as an argument.
# Others can be done in bulk (copying, moving, deleting etc.).
# Storing nsxiv's stdin into $file makes it possible to use loops selectively.

file=$(cat)

case "$1" in
w)	setbg "$file" & ;;
c)
	dest=$(sed -e 's/\s*#.*//' -e '/^$/d' -e 's/^\S*\s*//' "${XDG_CONFIG_HOME:-$HOME/.config}/shell/bm-dirs" | dmenu -l 20 -i -p "Copy" | sed "s|~|$HOME|g")
	[ -z "$dest" ] && exit || dest=$(eval printf '%s\\n' \"$dest\") && [ ! -d "$dest" ] && notify-send "$dest is not a directory" && exit ||
	for i in $file; do base="${i##*/}"; before_ext="${base%.*}"; ext="${base##*.}"
	[ -e "$dest/${base}" ] && cp "$i" "$dest/${before_ext}_$$.${ext}" || cp "$i" "$dest/${base}"
	done && notify-send "📋 Done" "Image(s) copied to $dest" & ;;
m)
	dest=$(sed -e 's/\s*#.*//' -e '/^$/d' -e 's/^\S*\s*//' "${XDG_CONFIG_HOME:-$HOME/.config}/shell/bm-dirs" | dmenu -l 20 -i -p "Move" | sed "s|~|$HOME|g")
	[ -z "$dest" ] && exit || dest=$(eval printf '%s\\n' \"$dest\") && [ ! -d "$dest" ] && notify-send "$dest is not a directory" && exit ||
	for i in $file; do base="${i##*/}"; before_ext="${base%.*}"; ext="${base##*.}"
	[ -e "$dest/${base}" ] && cp "$i" "$dest/${before_ext}_$$.${ext}" || cp "$i" "$dest/${base}"
	done && notify-send "🚚 Done" "Image(s) moved to $dest" & ;;
r)
	magick "$file" -rotate 90 "$file" ;;
R)
	magick "$file" -rotate -90 "$file" ;;
f)
	magick "$file" -flop "$file" ;;
s) 	
	# Copy image to clipboard.
	xclip -selection clipboard -target image/png "$file" && notify-send "Image copied to clipboard" & ;;
y)
	for i in $file; do printf '%s\n' "${i##*/}"; done | xclip -selection clipboard && notify-send "Filename(s) copied to clipboard" & ;;
Y)
	for i in $file; do readlink -f "$i"; done | xclip -selection clipboard && notify-send "File path(s) copied to clipboard" & ;;
d)
	[ "$(printf "No\nYes\n" | dmenu -i -p "Delete selection?")" = "Yes" ] && for i in $file; do rm "$i"; done && notify-send "Done" "File(s) deleted" ;;
g)
	ifinstalled gimp && setsid -f gimp "$file" ;;
i) 	
	# Timer increased to 10 seconds because otherwise notification is too fast to read.
	notify-send -t 10000 "File information" "$(mediainfo "$file" | sed "s/[ ]\+:/:/g;s/: /: <b>/;s/$/<\/b>/" | grep "<b>")" ;;
esac
@aartoni
Copy link
Contributor

aartoni commented Jan 28, 2025

My first consideration on this is that paths from ~cf/shell/bm-dirs might not be the best suggestions for a picture's destination, that is, most of the time I don't want to copy an image to ~cac, ~cf, ~rr and so on. Most of the time I want to copy an image to ~pp or anything from ~pp/*/, suggesting these directories would remove the need for sed and awk. For cases in which I don't want to copy to those directories I can type the path out or resort to lf.

Regarding the filename copy, the basename coreutil might be clearer than the printf + shell substitution.

Regarding the new feature, I really like it! Is your mind already set on the "x" key or are you still considering other letters?

I think that both the filename copy feature and image copy are ready to be made into PRs, nice contributions!

@aartoni
Copy link
Contributor

aartoni commented Jan 28, 2025

Quick note: the MIME type can be obtained using file. @RetroViking let me know if you'd prefer me to take on the PRs or if there's anything specific you'd like me to help with.

@RetroViking
Copy link
Author

RetroViking commented Jan 28, 2025

@aartoni, you can definitely do the PRs if you want. I use my personal fork of the repo, so I only follow the issues and PRs for ideas.

Typically, I use the file operations to copy pictures from /tmp into ~pp, but I also have custom "inbox"-like directories in which I might want to send pictures. So this approach works fine for me.

printf and parameter expansions may not be very legible, but they are "fashionable" in this repo because they are considered good practice (they're shell builtins, so they do not generate new processes like basename, awk etc. do).

I use the sed operations from the ~/.config/lf/lfrc version (cf. the copyto and moveto functions). The key-handler version uses a less efficient combo of sed and awk, while the lfrc version uses sed only. So I copied the lfrc approach to my copy of the key-handler.

Note: the key-handler version uses sed "s|~|$HOME|g" after a selection is made, and this works. In comparision, lfrc uses sed 's|~|$HOME|', which does not work.

Quick note: the MIME type can be obtained using file

I checked the script but don't see any commands to retrieve the MIME type. There is readlink, which is sometimes used in combination with file, e.g. file --mime-type "$(readlink -f $file)". But here readlink is used by itself to retrieve the full path in case nsxiv is called with a relative path.

Notes

  • When moving files. notify-send -i "$(readlink -f "$file")" should be changed to notify-send -i "$dest/${file##*/}" because once the file is moved to a different location, readlink would be pointed to a non-existent path.
  • The [ -z "$dest" ] check is due to dmenu sometimes sending an empty string when no selection is made, which is then potentially processed by other commands.

EDIT:
For those who want to use lf with dmenu with (instead of fzf) when moving or copying files, you can use this. The sed command that converts ~ to $HOME is also fixed, i.e. is the same as in the key-handler. And just like the latter, it warns if you choose a bookmark that points to a non-existent directory:

cmd moveto ${{
    set -f
    dest=$(sed -e 's/\s*#.*//' -e '/^$/d' -e 's/^\S*\s*//' "${XDG_CONFIG_HOME:-$HOME/.config}/shell/bm-dirs" | dmenu -l 20 -i -p "Move to where?" | sed "s|~|$HOME|g")
    [ -z "$dest" ] && exit || dest=$(eval printf '%s\\n' \"$dest\") && [ ! -d "$dest" ] && notify-send "$dest is not a directory" && exit ||
    mv -iv "$fx" "$dest" && notify-send "🚚 Done" "File(s) moved to $dest"
}}

cmd copyto ${{
    set -f
    dest=$(sed -e 's/\s*#.*//' -e '/^$/d' -e 's/^\S*\s*//' "${XDG_CONFIG_HOME:-$HOME/.config}/shell/bm-dirs" | dmenu -l 20 -i -p "Copy to where?" | sed "s|~|$HOME|g")
    [ -z "$dest" ] && exit || dest=$(eval printf '%s\\n' \"$dest\") && [ ! -d "$dest" ] && notify-send "$dest is not a directory" && exit ||
    cp -iv "$fx" "$dest" && notify-send "📋 Done" "File(s) copied to $dest"
}}

@aartoni
Copy link
Contributor

aartoni commented Jan 28, 2025

printf and parameter expansions may not be very legible, but they are "fashionable" in this repo because they are considered good practice (they're shell builtins.

Thanks for pointing that out, I've done a bit of research and was able to reproduce the results from this Stack Overflow question, showing that shell built-ins are also way faster than the commands.

@RetroViking
Copy link
Author

RetroViking commented Jan 30, 2025

A nice trick for organizing large picture libraries is opening the directory in lf, then pressing T. This will launch nsxiv's "thumbnail" mode from where you can mark pictures with m and then perform operations on them. However, due to the way the key-handler is written, if it contains dmenu prompts ("are you sure...") then they will run succesively for each and every marked file.

For an easy fix, you can check out this key-handler example and hack it as desired.

(Script was moved to the top post for visibility).

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

No branches or pull requests

2 participants