feat: improve command handling for _typer and _clipper#45
Conversation
|
Modified _typer and _clipper to better handle the
|
marty-oehme
left a comment
There was a problem hiding this comment.
Thanks for the PR!
When I first saw the 'magic' selection behind pickers and clippers I was not super enthusiastic (too much hidden functioning) but the more I think about it, I believe it makes some sense to implement it: a) we already do the same when no environment variable is given. b) someone only providing e.g. 'wl-copy' as a var makes no sense and c) we do the same for picker selection anyway.
I have some further thoughts on the implementation that could make it a little more stable, but I think getting this integrated could be a good thing for the program's ease of use overall.
| BEMOJI_PICKER_CMD="bemenu" # which picker tool to use | ||
| BEMOJI_CLIP_CMD="wl-copy" # which clipboard tool to use | ||
| BEMOJI_TYPE_CMD="wtype" # which typing tool to use (ydotool will NOT work) | ||
| BEMOJI_CLIP_CMD="wl-copy" # which clipboard tool to use // You can also type out full command xclip -selection clipboard |
There was a problem hiding this comment.
Good catch that this is not good documentation.
I am not sure how ready I am to introduce more 'magic' into the _CMD env vars, see below.
So for now, perhaps it makes most sense change the documentation to the latter half of your description, combined with the intention of the first half.
Additionally, I think we should indeed add the full command into the quotes here for "wl-copy" and "wtype" - though I think I would like to keep the default example as the wayland tools here and instead work on better X11 documentation in #44.
|
|
||
| if [ "${#cmd_array[@]}" -eq 1 ]; then | ||
| case "${cmd_array[0]}" in | ||
| wl-copy) |
There was a problem hiding this comment.
I like the intention behind giving precedence to 'wl-copy', 'xclip' and 'xsel' as default pickers 'selectable' through the "BEMOJI_CLIP_CMD" but I am not sure this is the right way to go about it.
A couple of issues come to mind:
- We are repeating ourselves for each 'magic' command (e.g. turning BEMOJI_CLIP_CMD=xclip ->
xclip selection clipboard) here and in the case of BEMOJI_CLIP_CMD="" below (i.e. L244 & L265 both independently define what to do). I would really like to avoid such repetition, so at least if we add this check we should save them to be accessible in a single variable somewhere. - If we go down that route, then it would be easier to keep them in an associative array with the full command as the value. This would then work similarly to our 'magic' picker selection, declared in
default_pickersso you could take inspiration there. - What happens in case we select a clip program (e.g. wl-copy) and then do not have it installed? Currently, this subroutine runs through all the checks and then does nothing. The user should be informed of an issue - and since they already explicitly choose to supply the cmd within this subroutine I think we should just execute it as-is and have them deal with the issue.
- What happens in case the user has a single-word command that they want to execute which is not one of these options? I.e. they have a 'mycustomclip' program on their machine and supply it as
BEMOJI_CLIP_CMD=mycustomclip. As far as I can tell this would currently result in their command being ignored completely.
There was a problem hiding this comment.
One last thought - but this is not necessary for the start - would be that when all is said and done, the two conditional routines are essentially the same. They get executed when an env var has exactly one argument, and compare the existence of their value to one in an array, then execute the array's value at that position.
So ultimately we could probably extract that into its own abstract function which just takes the array and the value. As I said - definitely not super necessary for the first iteration.
| wl-copy) | ||
| if command -v wl-copy >/dev/null 2>&1; then | ||
| wl-copy | ||
| return |
There was a problem hiding this comment.
Since each branch of this conditional returns on its own, we may as well just return at the end of the conditional below instead of having each branch have its own return.
| fi | ||
| fi | ||
|
|
||
| if [ -n "$WAYLAND_DISPLAY" ] && command -v wl-copy >/dev/null 2>&1; then |
There was a problem hiding this comment.
If we don't have a match in one of the above branches this still gets executed. This feels a little iffy to me,
as before there was an absolute guard from this being executed by running the [ -n "$BEMOJI_CLIP_CMD" ] conditional and instantly having a return at its branch end.
With the above code there are cases where we have a BEMOJI_CLIP_CMD set but still end up in this 'default-case' code below which seems muddy to me (e.g. when point 4 above occurs).
| return | ||
| read -r -a cmd_array <<< "$BEMOJI_TYPE_CMD" | ||
|
|
||
| if [ "${#cmd_array[@]}" -eq 1 ]; then |
There was a problem hiding this comment.
Same issues as above for "BEMOJI_CLIP_CMD" routine apply here.
There was a problem hiding this comment.
In general I am not totally against having some 'magic' commands which internally receive some options etc.
I would perhaps more closely copy the way we use it for 'default_pickers' with associative arrays however.
Then we can have all our default 'magic' options in one place for us and for the user to see.
Perhaps it could be an idea to split the PR into one which improves the documentation issue (which I think you point out very well) and one which adds 'selectable' default Clipper and Typer programs, similarly to pickers.
These two environment variables were misleading. As inside the code. It was looking for the whole command. Otherwise it was failing. The change below was made.