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

New features for clipdel #100

Closed
wants to merge 6 commits into from
Closed

New features for clipdel #100

wants to merge 6 commits into from

Conversation

brunelli
Copy link

@brunelli brunelli commented Jan 12, 2019

  • Implemented reading from standard input, allowing for a "clipdelmenu" with clipmenu | clipdel and other things;
  • Implemented deleting entries older than x time;
  • Implemented deleting current entry;
  • Changed 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.

@cdown
Copy link
Owner

cdown commented Jan 12, 2019

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.

@brunelli
Copy link
Author

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!

@cdown
Copy link
Owner

cdown commented Jan 17, 2019

Sorry, got slammed as soon as Monday came in. I do plan to look at this in the next couple of days.

@brunelli
Copy link
Author

Don't worry, there's no hurry!

@brunelli
Copy link
Author

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.

@cdown
Copy link
Owner

cdown commented Feb 13, 2019

Yes, I'll review it today. Sorry for the delay, the last couple of months have really been hectic :-)

Copy link
Owner

@cdown cdown left a comment

Choose a reason for hiding this comment

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

Conceptually looks great, thanks! Just a few user experience and implementation concerns.

@@ -45,6 +45,9 @@ if [[ "$CM_LAUNCHER" == rofi-script ]]; then
# shellcheck disable=SC2124
chosen_line="${@: -1}"
fi
elif [[ ! -t 1 ]]; then
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

I think with the comment below, this can be done reasonably as it is in mpdmenu :-)

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

clipmenu Outdated Show resolved Hide resolved
clipdel Outdated Show resolved Hide resolved
fi
;;
--|[!-]*)
if ((CM_REQUIRE_PATTERN)) && ! ((${#CM_PATTERNS[@]})); then
Copy link
Owner

Choose a reason for hiding this comment

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

I find this logic a bit hard to reason about. Can it be implemented with slicing and a for loop instead? :-)

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

Right, I'm talking about something like this: https://github.com/cdown/mpdmenu/blob/master/mpdmenu#L36-L40

clipdel Outdated Show resolved Hide resolved
clipdel Outdated Show resolved Hide resolved
@cdown
Copy link
Owner

cdown commented Apr 1, 2019

Hey, what happened here? I think this feature looks great, I was just waiting on the feedback from the second comment :-)

@brunelli
Copy link
Author

brunelli commented Apr 1, 2019

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.

@cdown
Copy link
Owner

cdown commented Apr 2, 2019

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? :-)

@brunelli
Copy link
Author

brunelli commented Apr 3, 2019

Thank you, that would be great!

@indradhanush
Copy link

@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 clipmenu and am absolutely loving it. In case @brunelli is unable to take this up, are the open issues the only things that need resolving for this to get merged? Has the project / expectations from this PR changed in the meantime?

@brunelli
Copy link
Author

@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.

@cdown
Copy link
Owner

cdown commented Mar 24, 2020

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!

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

Successfully merging this pull request may close these issues.

clipdel: Delete current clip clipdel: Delete older than x time Menu to delete clipboard entries
3 participants