Created attachment 318560 [details] [review]
[1] entryArea: Check if pasted text is not null

Created attachment 318561 [details] [review]
[2] Make paste functions generic

Created attachment 318562 [details] [review]
[3] Add imgur paste service

Created attachment 318563 [details] [review]
[4] handle paste service errors

Review of attachment 318560 [details] [review]:

beyond style nits, this patch looks good to me.

::: src/entryArea.js
@@ +36,3 @@
         clipboard.request_text(Lang.bind(this,
             function(clipboard, text) {
+                if(text==null) return;

style nit: space around operator
return should be on its own line.

Review of attachment 318561 [details] [review]:

::: src/entryArea.js
@@ +230,3 @@
                      "Paste %s lines of text to public paste service?",
                      nLines).format(nLines);
+        this._pasteButton.action_target = new GLib.Variant('(si)', [text, 0]);

perhaps it would be more clean if we make an enum instead of using an integer directly to represent paste content type? alternatively (and shorter) we could send a string around fx 'text'.

::: src/pasteManager.js
@@ +78,3 @@
+        let n;
+        if (type==0)
+            n = new UploadNotification("text");

..then we also wouldnt have to do this check but just parse the type directly to the upload notification.

Review of attachment 318562 [details] [review]:

::: src/entryArea.js
@@ +51,3 @@
+        clipboard.request_image(Lang.bind(this,
+            function(clipboard, pixbuf) {
+                if(pixbuf==null) return;

style nit: spacing around operator.
style nit: return should be on line below.

@@ +249,3 @@
+        if (!success)
+            return;
+        let encoded_buffer = GLib.base64_encode(buffer);

perhaps line 248-251 belongs better inside clipboard.request_image() ? (so clipboard.request_text and clipboard.request_image match in terms of what they do + _onTextPasted and _onImagePasted match in terms of what they do)

Review of attachment 318563 [details] [review]:

::: src/pasteManager.js
@@ +13,3 @@
+const Mainloop = imports.mainloop;
+
+const ERROR_NOTIFICATION_REVEAL_TIME = 5;

I suggest we use a longer reveal time (nautilus uses 8 seconds i think) but then make the in-app error notification user dismissable. (Provide an X next to the message to dismiss the message, see https://developer.gnome.org/hig/stable/in-app-notifications.html.en )

@@ +327,1 @@
+        label = new Gtk.Label({ label: _("Error in uploading %s").format(content) });

Hmm, maybe a better label is "Failed to upload %s."?

Thanks Bastian for review. Was waiting for patches on bug 760872 to land, and rebase the existing patches on them. Florian, want to add anything more to the reviews?

Review of attachment 318561 [details] [review]:

::: src/application.js
@@ +73,3 @@
+          { name: 'paste-content',
+            activate: Lang.bind(this, this._onPasteContent),
+            parameter_type: GLib.VariantType.new('(si)') },

We generally use "s" to mean "UTF-8 string". For arbitrary data, use "ay"

::: src/entryArea.js
@@ +230,3 @@
                      "Paste %s lines of text to public paste service?",
                      nLines).format(nLines);
+        this._pasteButton.action_target = new GLib.Variant('(si)', [text, 0]);

I agree with Bastian - no magic numbers. We already have DndTargetType in pasteManager

::: src/pasteManager.js
@@ +85,3 @@
     },
 
+    _pasteContent: function(data, datatype, notification) {

1) this obviously breaks the call to _pasteText()
    in _handleFileContent()
 2) the split looks wrong - the ellipsization shenanigans
    are workarounds for limitations of paste.gnome.org, so
    it's a bit silly to apply them to imgur.com as well

::: src/utils.js
@@ +138,3 @@
 }
 
+function gpaste(data, datatype, title, callback) {

"gpaste" == "gnome paste" (used to be "fpaste" when using fpaste.org)

So no, please leave this as a helper function to paste text to paste.gnome.org (read: don't touch utils.js in this patch) - just add imgurPaste(pixbuf, title, callback) or so in the follow-up patch

Review of attachment 318562 [details] [review]:

::: src/entryArea.js
@@ +20,3 @@
     Signals: { 'text-pasted': { param_types: [GObject.TYPE_STRING,
+                                              GObject.TYPE_INT] },
+               'image-pasted': { param_types: [GObject.TYPE_OBJECT] } },

Or more precise: GdkPixbuf.Pixbuf.$gtype

@@ +244,3 @@
 
+    _onImagePasted: function(entry, data) {
+        this._multiLinelabel.label = "Paste image to imgur paste service?";

Not sure we need the service name here. But we definitively want to mark the string for translation

::: src/utils.js
@@ +33,3 @@
 const GPASTE_TEXT_BASEURL = 'https://paste.gnome.org/';
 
+const IMGUR_CLIENT_ID = '4109e59177ec95e';

Is this a valid ID, or do we still need to request one?

Review of attachment 318563 [details] [review]:

Let's keep this on bug 756855?

Also please take some time to write proper commit messages.

Created attachment 320790 [details] [review]
entryArea: Check if pasted text is not null

We should check if text is null before pasting text,
and emitting corresponding signals.

Created attachment 320791 [details] [review]
Make paste functions generic

Change the name of the functions, signals, actions
to generic paste-content rather than paste-text, as
soon we will be adding support for pasting image, and
most of the code for pasting text and image is same,
so reuse the code.

Created attachment 320792 [details] [review]
Add support for pasting image to public image service

Support pasting images in polari by uploading them
to public image service (currently Imgur).

Review of attachment 320790 [details] [review]:

OK

Review of attachment 320791 [details] [review]:

::: src/application.js
@@ +70,3 @@
+          { name: 'paste-content',
+            activate: Lang.bind(this, this._onPasteContent),
+            parameter_type: GLib.VariantType.new('(si)') },

As mentioned previously, the convention is to use 's' for UTF8-encoded strings - assuming that all 'content' is in that format is a bit odd (see also the comment about not base64-encoding the image data immediately). Simply for semantic reasons, 'ay' (array of bytes) looks like a better choice (even though that means you'll have to call data.toString() to treat it like text)

::: src/pasteManager.js
@@ +77,1 @@
         let app = Gio.Application.get_default();

So this is the line that ends up being shared for the different types - not the biggest win ;-)

How about doing the type handling in _onPasteContent (in application.js) instead:

  let [data, type] = parameter.deep_unpack();
  switch (type) {
    case PasteManager.DndTargetType.TEXT:
      this._pasteManager.pasteText(data);
      break;
    default:
      log('Unhandled paste content of type %d'.format(type));
  }

Review of attachment 320792 [details] [review]:

::: src/entryArea.js
@@ +233,3 @@
+        if (!success)
+            return;
+        let encoded_buffer = GLib.base64_encode(buffer);

Style-nit: camel case in JS code

I'm also not quite sure this is the right place - "images are represented as base64 encoded strings" looks more like an implementation detail of the paste service, so maybe only convert the raw data in Utils.imgurPaste()?

Created attachment 320848 [details] [review]
Make paste functions generic

Change the name of the functions, signals, actions
to generic paste-content rather than paste-text, as
soon we will be adding support for pasting image, and
most of the code for pasting text and image is same,
so reuse the code.

Created attachment 320849 [details] [review]
Add support for pasting image to public image service

Support pasting images in polari by uploading them
to public image service (currently Imgur).

(In reply to Florian Müllner from comment #18)
> Review of attachment 320791 [details] [review] [review]:
> 
> ::: src/pasteManager.js
> @@ +77,1 @@
>          let app = Gio.Application.get_default();
> 
> So this is the line that ends up being shared for the different types - not
> the biggest win ;-)
> 
> How about doing the type handling in _onPasteContent (in application.js)
> instead:
> 
>   let [data, type] = parameter.deep_unpack();
>   switch (type) {
>     case PasteManager.DndTargetType.TEXT:
>       this._pasteManager.pasteText(data);
>       break;
>     default:
>       log('Unhandled paste content of type %d'.format(type));
>   }

I uploaded the new patch as you said, but didn't get why you asked so? The let app line still gets in both functions pasteText and pasteImage instead of pasteContent. What do you mean by line end up being shared ?

(In reply to Kunaal Jain from comment #22)
> I uploaded the new patch as you said, but didn't get why you asked so? The
> let app line still gets in both functions pasteText and pasteImage instead
> of pasteContent. What do you mean by line end up being shared ?

I mean pasteText(text) and pasteImage(image) are more readable than pasteContent(data, type). In the case of the action handling, the benefit of avoiding code duplication outweighs that, but there's little benefit in not duplicating a single line ...

(In reply to Florian Müllner from comment #23)
> (In reply to Kunaal Jain from comment #22)
> > I uploaded the new patch as you said, but didn't get why you asked so? The
> > let app line still gets in both functions pasteText and pasteImage instead
> > of pasteContent. What do you mean by line end up being shared ?
> 
> I mean pasteText(text) and pasteImage(image) are more readable than
> pasteContent(data, type). In the case of the action handling, the benefit of
> avoiding code duplication outweighs that, but there's little benefit in not
> duplicating a single line ...

Ah, now I get it. Well the current patch separates the function calls.

Review of attachment 320848 [details] [review]:

LGTM

Review of attachment 320849 [details] [review]:

Dto.

Attachment 320790 [details] pushed as 26e95fd - entryArea: Check if pasted text is not null
Attachment 320848 [details] pushed as 8a67ef0 - Make paste functions generic
Attachment 320849 [details] pushed as e5c4189 - Add support for pasting image to public image service

(In reply to Florian Müllner from comment #23)
> (In reply to Kunaal Jain from comment #22)
> > I uploaded the new patch as you said, but didn't get why you asked so? The
> > let app line still gets in both functions pasteText and pasteImage instead
> > of pasteContent. What do you mean by line end up being shared ?
> 
> I mean pasteText(text) and pasteImage(image) are more readable than
> pasteContent(data, type). In the case of the action handling, the benefit of
> avoiding code duplication outweighs that, but there's little benefit in not
> duplicating a single line ...

Florian, I ended up committing the current patches. But you told on IRC that you figured out a way to get me the abstraction of pasteContent :-) , no harm in another commit for abstracting it. What you figured out?

*** Bug 755594 has been marked as a duplicate of this bug. ***