PR#31: Fix copying url to the clipboard - fpaste

fpaste

#31 Fix copying url to the clipboard

Merged 8 years ago by ankursinha. Opened 8 years ago by farhaan.

farhaan/fpaste master into master

Replace selection with input-selection

Farhaan Bukhsh(fhackdroid) • 8 years ago

82240df

Update man page and help option

Farhaan Bukhsh(fhackdroid) • 8 years ago

1263591

Fix copying url to the clipboard

Farhaan Bukhsh(fhackdroid) • 8 years ago

d083780

docs/man/en/fpaste.1

file modified

+2 -2

		`@@ -40,9 +40,9 @@`
		`read paste text from current X clipboard selection`
		`.TP`
		`\fB\-o, \-\-clipout\fR`
		`- save returned paste URL to X clipboard`
		`+ save returned paste URL to all the clipboards i.e primary, secondary and clipboard`
		`.TP`
		`- \fB\-\-selection=CLIPBOARD\fR`
		`+ \fB\-\-input\-selection=CLIPBOARD\fR`
		`specify which X clipboard to use. valid options: "primary" (default; middle\-mouse\-button paste), "secondary" (uncommon), or "clipboard" (ctrl\-v paste)`
		`.TP`
		`\fB\-\-fullpath\fR`

fpaste

file modified

+19 -20

		`@@ -663,12 +663,12 @@`
		`'-o',`
		`'--clipout',`
		`dest='clipout',`
		`- help='save returned paste URL to X clipboard',`
		`+ help='save returned paste URL to all available clipboards',`
		`action="store_true",`
		`default=False)`
		`fpasteProg_group.add_option(`
		`'',`
		`- '--selection',`
		`+ '--input-selection',`
		`dest='selection',`
		`help='specify which X clipboard to use. valid options: "primary" (default; middle-mouse-button paste), "secondary" (uncommon), or "clipboard" (ctrl-v paste)',`
		`metavar='CLIP')`
		`@@ -846,30 +846,29 @@`

		`[url, short_url] = paste(text, options)`
		`if url:`
		`- # try to save URL in clipboard, and warn but don't error`
		`+ # Try to save URL in clipboard, and warn but don't throw error`
		`if options.clipout:`
		`if not os.access('/usr/bin/xsel', os.X_OK):`
		`print(`
		`'OOPS - the clipboard options currently depend on "/usr/bin/xsel", which does not appear to be installed',`
		`file=sys.stderr)`
		`else:`
		`- xselcmd = 'xsel -i --%s' % options.selection`
		`- # os.popen(xselcmd, 'wb').write(url)`
		`- p = subprocess.Popen(`
		`- xselcmd,`
		`- shell=True,`
		`- stdout=subprocess.PIPE,`
		`- stderr=subprocess.PIPE,`
		`- stdin=subprocess.PIPE)`
		`- (out, err) = p.communicate(input=url.encode('utf-8'))`
		`- if p.returncode != 0:`
		`- if options.debug:`
		`- print(err, file=sys.stderr)`
		`- print(`
		`- "WARNING: URL not saved to clipboard",`
		`- file=sys.stderr)`
		`- else:`
		`- print("URL copied to primary clipboard")`
		`+ # Copy the url in all the valid clipboard options`
		`+ for selection in validClipboardSelectionOpts:`
		`+ xselcmd = 'xsel -i --%s' % selection`
		`+ p = subprocess.Popen(`
		`+ xselcmd,`
		`+ shell=True,`
		`+ stdout=subprocess.PIPE,`
		`+ stderr=subprocess.PIPE,`
		`+ stdin=subprocess.PIPE)`
		`+ (out, err) = p.communicate(input=url.encode('utf-8'))`
		`+ if p.returncode != 0:`
		`+ if options.debug:`
		`+ print(err, file=sys.stderr)`
		`+ print(`
		`+ "WARNING: URL not saved to %s" % selection,`
		`+ file=sys.stderr)`

		`if ( not short_url and not options.rawurl ):`
		`print("WARNING: Could not shorten URL", file=sys.stderr)`

farhaan commented 8 years ago

Previously we were using primary clipboard as the only clipboard, now we just copy the url
in all the clipboards available to xsel which is primary, secondary and clipboard.

ankursinha commented 8 years ago

Thanks for the PR @farhaan . I don't know if we can using all option together works. I tried this out:

[asinha@ankur  fpaste(pr31=)]$ xsel -i --primary --clipboard --secondary
hahahahaha
[asinha@ankur  fpaste(pr31=)]$ xsel -o --primary --clipboard --secondary
hahahahaha
[asinha@ankur  fpaste(pr31=)]$ xsel -o --primary
adsfasdfasdf
[asinha@ankur  fpaste(pr31=)]$ xsel -o --secondary
lkjkjlj
[asinha@ankur  fpaste(pr31=)]$ xsel -o --clipboard
hahahahaha
[asinha@ankur  fpaste(pr31=)]$

Can you look into it more to confirm how xsel works?

farhaan commented 8 years ago

ohh i didn't think on those lines , i test the script and it was working but I guess its probably not the right way to do it , I am going to read more on xsel and let you know :)

rebased

8 years ago

farhaan commented 8 years ago

@ankursinha i have updated the PR look if it makes sense now , I didn't find a way in which i can update all the clipboards in a single command so I just updated every clipboard.

ankursinha commented 8 years ago

Works much better now! How do you suggest we change the documentation? Should we just say that we try to update all clipboards? I think once we use a config file, we'll be able to let them specify what clipboard they'd like (all/primary/secondary/clipboard)?

Can you update the man page and the --help output with another commit? :)

Edited 8 years ago by ankursinha

1 new commit added

Update man page and help option

8 years ago

farhaan commented 8 years ago

@ankursinha I have update the documentation let me know if anything has to be done. :smile:

ankursinha commented on line 6 of fpaste 8 years ago

s/all the clipboard/all available clipboards/

ankursinha commented on line 43 of fpaste 8 years ago

This comment seems unneeded, I think you can remove it.

ankursinha commented on line 56 of fpaste 8 years ago

Do you think it'd be better to also specify which clipboard the error occurred in? Otherwise the user would have to check all three manually :(

ankursinha commented on line 60 of fpaste 8 years ago

Do you think a flag is the best way of doing this? Why not have an else block that prints "Successfully copied to abc clipboard"? It'll be a lot more informative. The flag doesn't work here. Even if copying to one clipboard completes successfully, it's set to True. (Also, is the not correct here?)

ankursinha commented on line 9 of docs/man/en/fpaste.1 8 years ago

Hrm.. Do you think we still need the options for the various individual clipboards?

ankursinha commented 8 years ago

@ankursinha I have update the documentation let me know if anything has to be done. 😄

Left comments on the files. Thanks for working on this :)

farhaan commented on line 60 of fpaste 8 years ago

The flag works properly because default value of the flag is set to False and the flag is only changed when any error is raised , hence not in the flag. So, if the any error is encountered then only the flag will change to True and it wouldn't print in that case.

farhaan commented on line 56 of fpaste 8 years ago

agreed to this :)

farhaan commented on line 60 of fpaste 8 years ago

One of the reason I didn't use an else block is because it will print 3 times which looks a little odd. Do you want me to do that ?

farhaan commented 8 years ago

@ankursinha I have replied to the comment and fixed them as well I have not pushed the commit because then the comment will go off once you read the comment let me know I will push the corrected commit. :smile:

ankursinha commented on line 60 of fpaste 8 years ago

Hrm, maybe copied_to_clipboard_failed is a better variable name? Here's what I'm saying about the use of a single flag:

say the first primary clipboard works, the flag gets set to True
then whether or not the next two clipboards work, the flag remains True
so the user gets a message saying URL copied to clipboards which isn't accurate.

What do you think of getting rid of the flag and the final URL copied to clipboards part completely? So we only warn the user if copying to a clipboard fails?

ankursinha commented 8 years ago

@ankursinha I have replied to the comment and fixed them as well I have not pushed the commit because then the comment will go off once you read the comment let me know I will push the corrected commit. 😄

Really? The comments are removed? Should file a pagure issue for this, shouldn't we?

farhaan commented on line 60 of fpaste 8 years ago

Got your point with setting the flag :) and yes removing the success message makes it way more elegant I feel. Because for a normal person when we say that message is copied to clipboard it means its there and this will benefit us in keeping a layer of abstraction.

farhaan commented on line 9 of docs/man/en/fpaste.1 8 years ago

I think we should keep this option because this PR works on getting the output to all clipboards while user still has the choice to use -i option with the clipboard of his choice.

farhaan commented 8 years ago

But there is no way to retain comments I guess because if you update the file the comment are thought to be taken care of! You can file an issue if you want though :)

1 new commit added