#31 Fix copying url to the clipboard
Merged 4 years ago by ankursinha. Opened 4 years ago by farhaan.
farhaan/fpaste master  into  master

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

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)

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.

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?

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

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

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

1 new commit added

  • Update man page and help option
4 years ago

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

s/all the clipboard/all available clipboards/

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

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 :(

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

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

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

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.

agreed to this :)

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 ?

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

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

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.

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.

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

  • Fix documentation issue
4 years ago

2 new commits added

  • Update man page and help option
  • Fix copying url to the clipboard
4 years ago

Please remove this variable - it isn't needed any more, is it?

2 new commits added

  • Update man page and help option
  • Fix copying url to the clipboard
4 years ago

Ah, maybe mention in -i that --selection can be used?

Or, we could simply let -i take which clipboard is to be used as an argument and remove --selection completely.

What do you reckon?

I would say we use --selection because its there in the code and it makes the code more modular. what do you think ?

Yea, maybe modify it to --input-selection? Nice and clear that way?

1 new commit added

  • Replace selection with input-selection
4 years ago

Pull-Request has been merged by ankursinha

4 years ago