Skip to content

Add support to ignore the clipboard in certain windows#89

Merged
cdown merged 7 commits into
cdown:developfrom
Sohalt:develop
Nov 8, 2018
Merged

Add support to ignore the clipboard in certain windows#89
cdown merged 7 commits into
cdown:developfrom
Sohalt:develop

Conversation

@Sohalt

@Sohalt Sohalt commented Oct 9, 2018

Copy link
Copy Markdown
Contributor

The clipboard does not get recorded when the title of the currenlty active
window matches the regular expression in CM_IGNORE_WINDOW. This allows copying
passwords from a password manager without the passwords ending up in clipmenu.

The matching is not 100% exact however, as there is a race condition bewteen the
time the clipboard is populated, clipmenu queries the clipboard, and the active
window gets queried. This race condition can be especially problematic when
using polling with large intervals instead of clipnotify.

This implements #86.

The clipboard does not get recorded when the title of the currenlty active
window matches the regular expression in CM_IGNORE_WINDOW. This allows copying
passwords from a password manager without the passwords ending up in clipmenu.

The matching is not 100% exact however, as there is a race condition bewteen the
time the clipboard is populated, clipmenu queries the clipboard, and the active
window gets queried. This race condition can be especially problematic when
using polling with large intervals instead of clipnotify.
@Sohalt

Sohalt commented Oct 9, 2018

Copy link
Copy Markdown
Contributor Author

Maybe the dependency on xdotool should be documented somewhere in the README or help text, as well as the race condition.
And maybe the implemented functionality should only work when clipnotify is installed, because without, the possibility for sensitive data ending up in the clipboard is too high and the feature working somewhat might give the user a false sense of security.

Comment thread clipmenud Outdated
fi

windowname="$(xdotool getactivewindow getwindowname)"
if [[ -n "${CM_IGNORE_WINDOW+x}" && "$windowname" =~ $CM_IGNORE_WINDOW ]]; then

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

You don't need +x or -n here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wanted to distinguish between CM_IGNORE_WINDOW being unset and CM_IGNORE_WINDOW being set to the empty string, so I used the approach from https://stackoverflow.com/questions/3601515/how-to-check-if-a-variable-is-set-in-bash (I thought the empty string would match the empty string and not be treated as a wildcard, so it would be helpful to include). I'll update the code

Comment thread clipmenud Outdated
fi
fi

windowname="$(xdotool getactivewindow getwindowname)"

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should only be executed if we actually have anything to check

@cdown

cdown commented Oct 26, 2018

Copy link
Copy Markdown
Owner

Looks generally sane, just a few things to change. Please do also add some code which disables this if it's set and xdotool is not present with a warning. Thanks!

Comment thread clipmenud Outdated
Comment thread clipmenud Outdated
Comment thread clipmenud Outdated
@cdown

cdown commented Nov 8, 2018

Copy link
Copy Markdown
Owner

With your hcanges it looks fine, although I see no way to accept your changes here and amend the commit, so you'll need to rebase your changes onto your existing commit locally probably.

Travis failure looks like it should be fixed by these changes.

@cdown

cdown commented Nov 8, 2018

Copy link
Copy Markdown
Owner

Thanks! Just waiting for CI.

@Sohalt

Sohalt commented Nov 8, 2018

Copy link
Copy Markdown
Contributor Author

Do you want me to squash all the commits into one?

@cdown

cdown commented Nov 8, 2018

Copy link
Copy Markdown
Owner

No worries, I can squash them on merge.

@cdown cdown merged commit 7de9c9e into cdown:develop Nov 8, 2018
@cdown

cdown commented Nov 8, 2018

Copy link
Copy Markdown
Owner

Thanks! Merged and will go into next release.

@Sohalt

Sohalt commented Nov 8, 2018

Copy link
Copy Markdown
Contributor Author

Thank you for the great review!

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

2 participants