584063 - Clipboard API

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

The SDK needs a clipboard API. The prototype had one, detailed here: https://wiki.mozilla.org/Labs/Jetpack/JEP/10#JEP_10_-_Clipboard_Support.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: wip

Quick and dirty port of the Jetpack prototype's clipboard API to the SDK. Runs, and all tests pass.

TODO:
* get feedback on API, research HTML5 and other browser's APIs
* convert param checking to SDK style
* convert error handling to SDK style

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: wip

* added docs

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: v1

* more docs
* parameter validation

requesting review for api feedback and module implementation. will post to the forum for more api feedback and use-case satisfaction.

Comment 4

[Dietrich Ayala (:dietrich)]

Daniel pointed me at http://en.wikipedia.org/wiki/DOM_events#Microsoft-specific_events, which has a set of clipboard events.

Marcio Galli posted to the forum about a use-case for image data, for which I filed bug 586047.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

I don't know if it affects this bug at all, but note bug 583638 about a Gecko 2.0 regression in clipboard events that affects Bespin.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 464478 [details] [diff] [review]
v1

Drew's out on vacation, switching review to Myk.

Comment 7

[/\/\arcio Galli]

Get and set works nice. The way I tested was dropping the module in my own jetpack app dir instead from the main jetpack-core.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 464478 [details] [diff] [review]
v1

>diff --git a/packages/jetpack-core/lib/clipboard.js b/packages/jetpack-core/lib/clipboard.js

Nit: the file should have a license header at the top.


>+const kAllowableFlavors = [

Nit: it would be useful to note in a comment that flavors resemble Internet media types (and in some cases are identical to them) but are not the same thing (there is no text/unicode media type, "AOLMAIL" isn't even in the media type format, etc.).


>+const kFlavorMap = [
>+  { short: "plain", long: "text/unicode" },
>+  { short: "text", long: "text/unicode" },

Nit: this should define either "text" or "plain" but not both, which complicates the interface and gives developers an unnecessary choice.


>+  { short: "html", long: "text/html" }
>+  // Images are currently unsupported.
>+  //{ short: "image", long: "image/png" },
>+];

Nit: We should be conservative about adding to this list, since aliases complicate the interface, and because there are many duplicate words in the flavors we can potentially support (like "image" and "application").  In particular, "image" seems like a bad alias for "image/png".  It'd be worth adding a comment here that not all flavors will get their own alias and proposed new aliases must be run by an API design lead.


>+    datatype: {
>+      is: ["null", "undefined", "string"],
>+    },
>+  });

Nit: the last two commas in this snippet are trailing (generating a JS strict warning about trailing commas in ECMA-262 object initializers).


>+  // Handle the single argument case
>+  if (!options.datatype) {
>+    clipboardHelper.copyString(options.data);
>+    return true;
>+  }

Nit: it'd be better to simply set options.datatype to text/unicode in this case to reduce code duplication (since the code that handles that type does the exact same thing this code does).


>+  if (!xferable)
>+    throw new Error("Internal Error: Couldn't create Transferable");

Nit: we should try to make these errors as legible as possible for developers, so I would make this something like "Couldn't set the clipboard due to an internal error (couldn't create a Transferable object)."


>+  switch (flavor) {
>+    case "text/html":
>+      var str = Cc["@mozilla.org/supports-string;1"].
>+      createInstance(Ci.nsISupportsString);

Nit: for consistency and readability, indent the second line of this statement to the column in which the assigned value starts on the first line, i.e.:

      var str = Cc["@mozilla.org/supports-string;1"].
                createInstance(Ci.nsISupportsString);


>+      str.data = options.data;
>+      xferable.addDataFlavor(flavor);
>+      xferable.setTransferData(flavor, str, options.data.length * 2);
>+      break;
>+    // TODO: images!

Nit: I would also add "TODO: add a text/unicode flavor for HTML text that returns a plaintextified representation of the HTML."


>+    default:
>+      return false;

Nit: this should throw if it can't handle the flavor, to make sure developers know when they use an invalid one, and because this method throws if setting the clipboard fails for other reasons.


>+  } catch (e) {
>+    throw new Error("Internal Error: Could set clipboard data");
>+  }

Nit: Could -> Couldn't

Nit: I would make this something like: "Couldn't set the clipboard due to an internal error (" + e + ")" (that way, when developers report the exception to us, we get information about the exception that was originally thrown, which will help us diagnose the problem).


>+exports.get = function(aDataType) {
>+  let options = {
>+    datatype: aDataType
>+  };
>+  options = apiUtils.validateOptions(options, {
>+    datatype: {
>+      is: ["null", "undefined", "string"],
>+    },
>+  });

Nit: the last two commas in this snippet are trailing (generating a JS strict warning about trailing commas in ECMA-262 object initializers).


>+  var xferable = Cc["@mozilla.org/widget/transferable;1"].
>+                 createInstance(Ci.nsITransferable);
>+  if (!xferable)
>+    throw new Error("Internal Error: Couldn't create Transferable");

Nit: something like "Couldn't get the clipboard due to an internal error (couldn't create a Transferable object)."


>+  var flavor = aDataType ? fromJetpackFlavor(aDataType) : "text/unicode";
>+
>+  // Ensure that the user hasn't requested a flavor that we don't support.
>+  if (!flavor)
>+    throw new Error("Invalid flavor");

Nit: something like "Getting the clipboard with the flavor '" + flavor + "' is not supported.".


>+  var data = {};
>+  var dataLen = {};
>+  try {
>+    xferable.getTransferData(flavor, data, dataLen);
>+  } catch (e) {
>+    // Clipboard doesn't contain data in flavor, return null.
>+    return null;
>+  }
>+
>+  // TODO: Not sure data will ever be false or null at this point, but
>+  // doesn't hurt to check.
>+  if (!data)
>+    return null;

Nit: data will never be false or null at this point, because it is an object, and objects are always true when evaluated in a boolean context, so this is unnecessary.

data.value, on the other hand, might be null or false, but those seem like discrete cases that we should treat differently.  Presumably it makes sense to return null when data.value is null, since there is no value available, and we indicate that by returning null from this function.  If the value evaluates to false, on the other hand (f.e. because it is an empty string, or because it is some other kind of value we support in the future that evaluates to false), then while I don't understand what it would be doing on the clipboard, I would still defer to the implementation and return that value.


>+  // TODO: Add flavors here as we support more in kAllowableFlavors.
>+  switch (flavor) {
>+    case "text/plain":
>+      data = data.value.data;
>+      break;

Nit: the flavor should never be text/plain at this point, because that flavor isn't listed as one the API supports, so fromJetpackFlavor will never return it.  So this case is unnecessary and should be removed.


>+    case "text/unicode":
>+    case "text/html":
>+      data = data.value.QueryInterface(Ci.nsISupportsString).data;
>+      break;
>+    default:
>+      return null;
>+  }
>+
>+  return data;
>+};

Nit: rather than returning null in the default case, just set data to null in that case and let it get returned by the return statement at the end of the function.


>+function toJetpackFlavor(aFlavor) {
>+  for each (flavorMap in kFlavorMap)
>+    if (flavorMap.long == aFlavor || flavorMap.short == aFlavor)
>+      return flavorMap.short;
>+  // Return null in the case where we don't match
>+  return null;
>+}

Nit: this doesn't need to check flavorMap.short, because it is only ever called on values obtained from the clipboard service, which never uses the "short" aliases this API defines.


>diff --git a/packages/jetpack-core/tests/test-clipboard.js b/packages/jetpack-core/tests/test-clipboard.js

>+// Test the slightly less common case where we specify the flavor
>+exports.testWithFlavor = function(test) {
>+  var contents = "<b>hello there</b>";
>+  var flavor = "html";
>+  var fullFlavor = "text/html";
>+  var clip = require("clipboard");
>+  test.assert(clip.set(contents, flavor));
>+  test.assertEqual(clip.getCurrentFlavors()[0], flavor);
>+  // Confirm default flavor returns false
>+  test.assert(!clip.get());

Nit: test for null rather than false here, since that is what is actually being returned.

Of course, once we add a plaintext (text/unicode) flavor for HTML text, this will actually return the plaintext version.


>+exports.testNotInFlavor = function(test) {
>+  var contents = "hello there";
>+  var flavor = "html";
>+  var clip = require("clipboard");
>+  test.assert(clip.set(contents));
>+  // If there's nothing on the clipboard with this flavor, should return false
>+  test.assert(!clip.get(flavor));

Nit: test for null rather than false here, since that is what is actually being returned.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 464478 [details] [diff] [review]
v1

Oh!  One more thing, which isn't a nit and thus reverses my review: this needs docs.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> Oh!  One more thing, which isn't a nit and thus reverses my review: this needs
> docs.

BAH. I forgot to hg add the file.

Comment 11

[Dietrich Ayala (:dietrich)]

> Nit: the last two commas in this snippet are trailing (generating a JS strict
> warning about trailing commas in ECMA-262 object initializers).

Removed, since not intentionally left there. However, trailing commas shouldn't warn anymore, bug 508637.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 465322 [details] [diff] [review]
v2

>diff --git a/packages/jetpack-core/docs/clipboard.md b/packages/jetpack-core/docs/clipboard.md

>+The `clipboard` module allows callers to interact with the system clipboard,
>+setting and retrieving it's contents.

Nit: it's -> its


>+You can optionally specify the type of the data being set, and being requested.

Nit: being set, and being requested -> to set and retrieve (for consistency with previous paragraph)


>+The following types are supported, in both short and long form:
>+
>+plain (long form: text/unicode)
>+text  (long form: text/unicode)
>+html  (long form: text/html)

Remove the "plain" alias, since it is no longer supported.

Also, it would be useful to note that the "text" type represents plain text while the "html" type represents a string of HTML (as opposed to some kind of DOM object, which users might expect).

Finally, I wonder why we even need to mention the long forms here.  After all, there is no situation in which one needs to use a long form instead of a short one or in which doing so makes a difference.  As the whole purpose of the short forms is to simplify the interface, it'd be better to only mention the short forms here, even if we continue to support the long ones (and later support additional long ones that don't have a short form), i.e. something like this:

The following types are supported:

* `text` (plain text)
* `html` (a string of html)

Then, in the future, we might add:

* `image/png` (a PNG image)

(Ideally, this would be a definition list, but it seems like our Markdown processor doesn't support those.)


>+Functions
>+---------

Nit: this seems to be called Reference in other docs (which also future-proofs it against the addition of a property, as I actually suggest below).


>+<api name="set">
>+@function
>+  Add data to the user's clipboard.

Nit: "add" implies concatenation, whereas this does replacement, so it should say something like "replace the contents of the user's clipboard with the provided data."


>+@param [data] {string}
>+  The data to add to the clipboard.

data is not optional, so it should be specified as "data", not "[data]".

Nit: to add to -> to put on (or the like)


>+<api name="getCurrentFlavors">
>+@function
>+  Get a list of the types of data in the user's clipboard.
>+</api>

This function should be renamed to something like getCurrentTypes if we're going to use "type" rather than "flavor" in the docs.

Alternately, we could switch the docs to use the term "flavor", which is a more obscure term, but that may actually be a good thing in this case, given that programmers often think of types as discrete (i.e. a given piece of data only has one "type", even if it inherits from multiple others), whereas in this case the data potentially has multiple "variants" (just like food can have multiple flavors).

And that brings up another point, which is that these docs don't explain why getCurrentFlavors even exists in the first place.  They should say something like (replacing "type" with "flavor" if adopting that term):

  Data on the clipboard is sometimes available in multiple types.  For example,
  HTML data might be available as both a string of HTML (the `html` type) 
  and a string of plain text (the `text` type).  This function returns an array
  of all types in which the data currently on the clipboard is available.

(Even though at the moment our implementation doesn't provide a plaintextified variant for HTML data.)

Finally, this would be simpler as a getter property (currentFlavors/currentTypes) than a method.

Close!  Just need to fix the non-nits (and the nits at your discretion).

Comment 14

[Dietrich Ayala (:dietrich)]

/me facepalms. i didn't see this bugmail go by, was waiting for your review for the last 5 days. blargh. new patch coming up.

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: v3

comments addressed!

i stuck with flavors, which i think is less ambiguous and is more descriptive in this context, than types.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 466781 [details] [diff] [review]
v3

Looks good, r=myk!

Comment 17

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/labs/jetpack-sdk/rev/c41c408e6380

Comment 18

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".