New features for clipdel #100
Conversation
|
This looks great, thanks! I'm out most of this weekend but I've made a note to go over this PR in the coming few days. |
Thank you! I just force pushed some fixes to my branch. From now on if any change is required they'll be pushed in new commits to be rebased before a merge. Have a nice weekend! |
|
Sorry, got slammed as soon as Monday came in. I do plan to look at this in the next couple of days. |
|
Don't worry, there's no hurry! |
|
Hey, @cdown. Sorry for the ping, but could this get reviewed this month? Otherwise I won't have time to do any requested change. Thank you. |
|
Yes, I'll review it today. Sorry for the delay, the last couple of months have really been hectic :-) |
| @@ -45,6 +45,9 @@ if [[ "$CM_LAUNCHER" == rofi-script ]]; then | |||
| # shellcheck disable=SC2124 | |||
| chosen_line="${@: -1}" | |||
| fi | |||
| elif [[ ! -t 1 ]]; then | |||
There was a problem hiding this comment.
Hmm, I think this seems good, but it should likely be implemented by a flag rather than by checking if stdout is connected to a terminal. Otherwise I worry this feature is too hard to turn off.
There was a problem hiding this comment.
You mean, like clipmenu --print?
Currently "all arguments are passed through to dmenu itself" (-h/--help being the exception), so it would have to be changed and maybe could get a little confusing.
There was a problem hiding this comment.
I think with the comment below, this can be done reasonably as it is in mpdmenu :-)
There was a problem hiding this comment.
So, are you suggesting we change the way arguments are handled to make it mpdmenu-like?
Pass clipmenu arguments first, followed by any dmenu arguments. They are separated by ::. For example:
clipmenu -p :: -sb '#000000'
Wouldn't it break current workflows?
There was a problem hiding this comment.
it should likely be implemented by a flag rather than by checking if stdout is connected to a terminal. Otherwise I worry this feature is too hard to turn off.
I'd argue that having selection passed through a pipe or redirected to a file would be the expected behavior when using *nix tools. Why would someone run something like clipmenu > test and expect it to print nothing to test (current behavior)?
The only flaw I see in this implementation is that what is printed is the content in line_cache, instead of the selection itself, so manipulating the output would need some extra work. Maybe this should be changed, but it'd require some thought on how to handle multiple selections.
| fi | ||
| ;; | ||
| --|[!-]*) | ||
| if ((CM_REQUIRE_PATTERN)) && ! ((${#CM_PATTERNS[@]})); then |
There was a problem hiding this comment.
I find this logic a bit hard to reason about. Can it be implemented with slicing and a for loop instead? :-)
There was a problem hiding this comment.
This is so everything after a -- is read as a pattern. E.g.:
clipdel --all: will delete all entries
clipdel -- --all: will delete all entries matching --all
clipdel pattern1 -- pattern2 will fail due to excessive arguments.
Sorry, I don't understand what you mean by "be implemented with slicing and a for loop instead". Could you give me a quick draft?
There was a problem hiding this comment.
Right, I'm talking about something like this: https://github.com/cdown/mpdmenu/blob/master/mpdmenu#L36-L40
|
Hey, what happened here? I think this feature looks great, I was just waiting on the feedback from the second comment :-) |
|
Hey, I'm sorry for closing this PR, but I won't be able to continue working on this. The semester has started, plus an internship, and my laptop stopped working (I'm stuck with a Windows machine until I fix it), so if you or someone else can help with this, feel free to work on top of my fork for a new PR, and reach me for any help needed understanding my code. Currently there are two pending changes. One regarding whether to expose pipe redirection as an option or not and the other requiring changes to the argument parsing logic. |
|
Sure, no problem. In which case I may take this and fix it up some time, of course keeping your commits in the history as credit. Sound good? :-) |
|
Thank you, that would be great! |
|
@brunelli This seems like great work! I know a lot of time has passed since this got closed, is there any chance you have some time to take another shot at it? If not, I totally understand and would like to see if I can find a way to get this merged while maintaining your commit history or add you as co-author in case of fixup commits. @cdown I just started using |
|
@indradhanush Hey, I'm not using this project anymore, since I moved to a Wayland desktop. You can feel free to continue the work I did in this PR as you'd like. No need to credit me for any piece of code I wrote and you may use. You can ping me if you have any question and I'll try to help you. |
|
Hey! This is still a desirable thing to have. Some of the backend storage has changed, for example the way cache lines are stored, so that will need to be changed to match the 6.0.0 format. I can't reopen this pull request because the repository backing it has been deleted, but I'd welcome another PR to this effect. :-) Thanks both! |
clipmenu | clipdeland other things;help.After some tests I can say the solution I used for line deletion in cache files looks safe. It needs some more testing by others before merging, though.
This PR fixes #57, fixes #97, fixes #98, closes #91 and closes #94.