383044 - Drag-and-drop bookmark creation should support meta-data

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Bo Yang]

While the existing bookmark service itself if very flexible, by which extension can add microsummary, live bookmarks and etc, the existing drag-and-drop bookmark creation mechanism is ugly to some extend. It can't satisfy the various bookmark creation requirement. 
When the user drag some text in the content browser window into the bookmark menu, there is a new entry in the menu, but it is not a complete bookmark which uri is the same with the bookmark title. I think this is some kind of irrational thing.
More complete bookmark drag-and-drop creation mechanism is necessary for more flexible bookmark creation by extensions. The new mechanism should support other extension to create bookmark with particular meta-data (for example the microsummary extension will create a microsummary bookmark entry which may have its own meta-data GENERATED-TITLE and so on) by drag-and-drop method.

Comment 1

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

Bo and I talked about this earlier, so I'm updating the summary to be a bit more specific.

The issue is that code can't create a drag object that, when dropped on bookmarks UI, creates a bookmark pre-populated with specific meta-data.  For example, one couldn't create a microsummary bookmark via drag and drop, since a microsummary bookmark requires an annotation to be set.

Comment 2

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

Bo Yang wrote:
>   In bookmarksMenu drop handelr of Places, there is no code to process
> the meta-data and what is more important is that there is no mechanism
> in FF now support transfer meta-data of bookmark in DND.
>   So I think two things should be done for fixing the bug.
>
> 1.Create a new flavour format to support the meta-data transfering. The
> new flavour can be called "text/x-place-url" which has the format
> "title\nuri\nfield1=value1\nfield2=value2\nfield3=value3" .
>
> 2.Add new flavour processing code in the Places drop handler to support
> the new flavour.
>
> That is all. But I am now wandering between whether I create a new
> flavour or I change the now existing flavour "text/x-moz-url" . Could
> you please give me some advice here?

Hmm, I'm not quite sure what's the best thing to do.  My inclination would be to reuse the existing flavor if it's possible to do so without breaking backwards compatibility, but otherwise to create a new flavor, since I wouldn't want to make a change that breaks some usage of text/x-moz-url unnecessarily.

But I'm probably not the best person to make this determination. cc:ing Seth Spitzer (sspitzer) for his thoughts.

Seth: the question is how best to support drag-and-drop creation of bookmarks that have some associated meta-data (i.e. annotations).  Presumably we need to include that meta-data in the drag object, and we're wondering whether we should do that by extending the text/x-moz-url flavor or defining a new one.

Comment 3

[(not reading, please use [e-mail] instead)]

sorry for the delay here.  

Christine has a patch for this already, see bug #378558.

That patch is pending review, but you are more than welcome to apply her latest version to your local tree, test it, and comment in the bug if you have any problems or if it works for you.

I think this bug should be marked as a dup of bug #378558, or depending on it.

Comment 4

[(not reading, please use [e-mail] instead)]

cyen, can confirm that your patch works-as-expected with microsummary annotations?

Comment 5

[Christine Yen]

(In reply to comment #4)
> cyen, can confirm that your patch works-as-expected with microsummary
> annotations?
> 

I'm not entirely familiar with microsummaries, but swon sent me this microsummary generator link [http://dietrich.ganx4.com/mozilla/test-microsummary.xml], where if I drag the provided test content link [http://dietrich.ganx4.com/mozilla/test-microsummary-content.php] (either as text-unicode or as a moz-url, actually) into the bookmarks manager or the bookmark toolbar, live title information is available via "properties."

This actually works in not only a trunk build with my patch, but a trunk build without my patch, and a nightly from June 2nd.

Bo, do you mind giving me a test case where I can check this?

Comment 6

[Bo Yang]

(In reply to comment #5)
> (In reply to comment #4)
> > cyen, can confirm that your patch works-as-expected with microsummary
> > annotations?
> > 
> 
> I'm not entirely familiar with microsummaries, but swon sent me this
> microsummary generator link
> [http://dietrich.ganx4.com/mozilla/test-microsummary.xml], where if I drag the
> provided test content link
> [http://dietrich.ganx4.com/mozilla/test-microsummary-content.php] (either as
> text-unicode or as a moz-url, actually) into the bookmarks manager or the
> bookmark toolbar, live title information is available via "properties."
> 
> This actually works in not only a trunk build with my patch, but a trunk build
> without my patch, and a nightly from June 2nd.
> 

I guess you must click the "Install" link in the page http://dietrich.ganx4.com/mozilla/test-microsummary.xml and then drag "http://dietrich.ganx4.com/mozilla/test-microsummary-content.php" into somewhere. But this feature is one of the standard bookmark microsummary component features and has nothing to do with your patch. So you have seen that it works even without your patch. 

Seth, Christine and Myk: I think we have some misunderstandings here about the two bugs. I have take a look over the patch of Christine and found it could not provide what I want. What Christine is doing is to make the cut/copy and paste with annotation data. The patch process the situation of drag-drop between bookmarks. But what I am doing is to provide a mechanism to let Places to support drag-drop bookmark creation with any annotation data. We need this because we want to add drag-and-drop creation of microsummary bookmark. For details: when you drag some text in content area into the bookmark toolbar, it will create a microsummary bookmark with the drag text as the live title. To accomplish this feature, we need that Places drop handler to process the drop of any annotation data. And that is what my patch does. I will upload it, it only contains hundred of code, please take a few minutes to look over it before deciding this is a dup bug. 

> Bo, do you mind giving me a test case where I can check this?

Yes, for the reason of test my patch, I have create a litte extension. I will email it to you. 

Comment 7

[Bo Yang]

Attachment: Initial patch

Although there is only less than hundred of code, this patch provide exactly what I want. Please review it before we make any decision about the two bugs.
Thanks!

Comment 8

[Christine Yen]

The (In reply to comment #7)
> Created an attachment (id=271784) [details]
> Initial patch
> 
> Although there is only less than hundred of code, this patch provide exactly
> what I want. Please review it before we make any decision about the two bugs.
> Thanks!

Thanks for your patch, Bo - it actually raises a question I came across when working on the other patch/rewrite of unwrapNodes and wrapNode. The reason we haven't supported x-moz-url with an unlimited number of parameters, as you propose with something like "title/nuri/ntype1:name1:value1:flag1:expire1/ntype2:name2:value2:flag2:expire2..." is that we currently seem to want to support unwrapping more than one moz-url appended together in a transferable.

So far, though, I've only seen moz-urls used in dragging a url to the location bar and to create an item in the bookmarks manager. Since it's impossible to drag more than one link at a time within the browser, and since the location bar only accepts the first moz-url, I'd like to see examples of multiple moz-urls needing to be transferred.. and if there are no cases where that's necessary, allowing moz-urls to have an unlimited number of parameters could be possible.

Otherwise... we could try to find some other safe way of delimiting multiple moz-urls besides newlines, and simply use newlines within the moz-url to separate fields.

FYI, Bo: TYPE_UNICODE is dealt with in unwrapNodes() when a user copies a text url to his or her clipboard, then wants to paste it into the Bookmark Manager / drags a selected text url into the manager. :) so it isn't unnecessary.

Comment 9

[Bo Yang]

(In reply to comment #8)
> The (In reply to comment #7)
> > Created an attachment (id=271784) [details] [details]
> > Initial patch
> > 
> > Although there is only less than hundred of code, this patch provide exactly
> > what I want. Please review it before we make any decision about the two bugs.
> > Thanks!
> 
> Thanks for your patch, Bo - it actually raises a question I came across when
> working on the other patch/rewrite of unwrapNodes and wrapNode. The reason we
> haven't supported x-moz-url with an unlimited number of parameters, as you
> propose with something like
> "title/nuri/ntype1:name1:value1:flag1:expire1/ntype2:name2:value2:flag2:expire2..."
> is that we currently seem to want to support unwrapping more than one moz-url
> appended together in a transferable.

If we do it, we got the ability to create more than one bookmark one time. But we still can't create a bookmark with specific annotation data because moz-url didn't provide a way to transfer them. 

> 
> So far, though, I've only seen moz-urls used in dragging a url to the location
> bar and to create an item in the bookmarks manager. Since it's impossible to
> drag more than one link at a time within the browser, and since the location
> bar only accepts the first moz-url, I'd like to see examples of multiple
> moz-urls needing to be transferred.. and if there are no cases where that's
> necessary, allowing moz-urls to have an unlimited number of parameters could be
> possible.
> 
> Otherwise... we could try to find some other safe way of delimiting multiple
> moz-urls besides newlines, and simply use newlines within the moz-url to
> separate fields.

I think add another particular flavour such as "text/x-dnd-url" is helpful. 

> 
> FYI, Bo: TYPE_UNICODE is dealt with in unwrapNodes() when a user copies a text
> url to his or her clipboard, then wants to paste it into the Bookmark Manager /
> drags a selected text url into the manager. :) so it isn't unnecessary.

After I comment it yesterday, I found it is necessary. I will undo it later. Thanks!

Comment 10

[(not reading, please use [e-mail] instead)]

fixed by cyen's fix for bug #378558

Comment 11

[Christine Yen]

Sorry Seth -- as I mentioned in comment #8, it's not actually fixed by my patch. What Bo is asking for is for annotations to be supported when dragging moz-urls to the bookmarks manager, which they're not, even with my patch.

If we somehow used some other delimiter to separate moz-urls, we'd still be able to concatenate multiple urls and potentially be able to append a random number of annotations on at the end (though I was wondering why that's necessary - as Seth and I noticed while dealing with bug 378558, the only use cases for x-moz-url in Firefox are dnd creation by dragging a link, and dragging a link to the toolbar. In the former case, you can only drag one link, and in the latter case only one link is accepted...)

Question being, out of curiosity: is multiple-moz-url support used/required anywhere? And is it feasible to change the format of moz-url to support annotations, or are we out of luck?

Comment 12

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

Comment on attachment 271784 [details] [diff] [review]
Initial patch

Seth or one of the Places guys should review this (adding a review request for Seth), but here are a few notes.

>Index: content/utils.js

>+  // Add a new flavor to support the drag-and-drop creation bookmark with annotation data
>+  // This url has the format  title/nuri/ntype1:name1:value1:flag1:expire1/ntype2:name2:value2:flag2:expire2...
>+  // name is the annotation name and type is one of string,int32,int64,double,binary
>+  TYPE_X_DND_URL: "text/x-dnd-url",

Nit: I think this format would be better named X_ANNOTATED_URL, since the format isn't specific to drag-and-drop (it could be used outside of that), and the key difference between this and X_MOZ_URL is that it supports annotations.


>       case this.TYPE_UNICODE:
>-        // See above.
>+        // See above. I think we didn't need in this kind of drop at all
>+        /*

We've already discussed this, but just noting that it seems like we do need to support the UNICODE type.


>+      case this.TYPE_X_DND_URL:
>+	  // extract the data
>+	  if( parts.length < 2){
>+	  	LOG("Wrong arguments of x-dnd-url");
>+		throw Cr.NS_ERROR_INVALID_ARG;
>+	  }
>+	  if( i == 0 )
>+	  	item.title = parts[i];
>+	  else if ( i == 1 )
>+	  	item.uri = this._uri(parts[i]);
>+	  else{

Nit: here and elsewhere, per standard Mozilla/Firefox style, put spaces after "if" statements and before curly brackets but not inside parentheses, i.e.:

  if (parts.length < 2) {

  if (i == 0)

  else {


>+	  	var fields = parts[i].split(":");
>+	  	item.annons.push({ type: parseInt(fields[0]), name: fields[1], value: fields[2], flags: fields[3], expires: fields[4] });

Don't the flags and expires fields also need to be parsed as integers?

Also, the name and value fields can contain colons and newlines, so they'll need to be escaped in some way when the data is created and then unescaped when it's parsed here.

Comment 13

[Bo Yang]

(In reply to comment #12)
> (From update of attachment 271784 [details] [diff] [review])
> Seth or one of the Places guys should review this (adding a review request for
> Seth), but here are a few notes.
> 
> >Index: content/utils.js
> 
> >+  // Add a new flavor to support the drag-and-drop creation bookmark with annotation data
> >+  // This url has the format  title/nuri/ntype1:name1:value1:flag1:expire1/ntype2:name2:value2:flag2:expire2...
> >+  // name is the annotation name and type is one of string,int32,int64,double,binary
> >+  TYPE_X_DND_URL: "text/x-dnd-url",
> 
> Nit: I think this format would be better named X_ANNOTATED_URL, since the
> format isn't specific to drag-and-drop (it could be used outside of that), and
> the key difference between this and X_MOZ_URL is that it supports annotations.
> 

This is a good point, I will change it soon.

> 
> >       case this.TYPE_UNICODE:
> >-        // See above.
> >+        // See above. I think we didn't need in this kind of drop at all
> >+        /*
> 
> We've already discussed this, but just noting that it seems like we do need to
> support the UNICODE type.

Yes, I forget that. I will add it back later.

> 
> 
> >+      case this.TYPE_X_DND_URL:
> >+	  // extract the data
> >+	  if( parts.length < 2){
> >+	  	LOG("Wrong arguments of x-dnd-url");
> >+		throw Cr.NS_ERROR_INVALID_ARG;
> >+	  }
> >+	  if( i == 0 )
> >+	  	item.title = parts[i];
> >+	  else if ( i == 1 )
> >+	  	item.uri = this._uri(parts[i]);
> >+	  else{
> 
> Nit: here and elsewhere, per standard Mozilla/Firefox style, put spaces after
> "if" statements and before curly brackets but not inside parentheses, i.e.:
> 
>   if (parts.length < 2) {
> 
>   if (i == 0)
> 
>   else {
> 

I got it.

> 
> >+	  	var fields = parts[i].split(":");
> >+	  	item.annons.push({ type: parseInt(fields[0]), name: fields[1], value: fields[2], flags: fields[3], expires: fields[4] });
> 
> Don't the flags and expires fields also need to be parsed as integers?
> 
> Also, the name and value fields can contain colons and newlines, so they'll
> need to be escaped in some way when the data is created and then unescaped when
> it's parsed here.
> 

I am sorry I ignore this, I will try to add the support and send a new patch soon. 

Thanks, Myk!

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 271784 [details] [diff] [review]
Initial patch

Bo/Christine/Myk, sorry for the misunderstanding.

Comment 15

[Bo Yang]

Attachment: A new version of the Drag-and-drop creation

This is the new version of that patch:
uncomment the UNICODE processing code;
escape the special chars ":" and "\n"
coding style change

Comment 16

[Bo Yang]

Attachment: The test extension for the drag-and-drop operation

This is the test extension of the new drag-and-drop creation of bookmark. When you install the extension, there will be a "TestDND" in the "Tools" menu. After clicking it, you can try to drag some text into the bookmark toolbar, the extension will create a new bookmark with two annotations and with the link "developer.mozilla.org"

Comment 17

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

Comment on attachment 272653 [details] [diff] [review]
A new version of the Drag-and-drop creation

>+   * Unescape the colon and enter in the url

I just realized that you need to escape the slash character (/) as well, since the slash also a special character in the format you've devised.  

Also, as you're using ampersand-based character entities to do the escaping, you'll need to escape the ampersand itself first, just as in HTML/XML, and then unescape it last.

Comment 18

[Bo Yang]

(In reply to comment #17)
> (From update of attachment 272653 [details] [diff] [review])
> >+   * Unescape the colon and enter in the url
> 
> I just realized that you need to escape the slash character (/) as well, since
> the slash also a special character in the format you've devised.  
> 
Oh, I am sorry. I found I have wrote the "\n" to "/n", which may let you think the "/" is another special character. 

> Also, as you're using ampersand-based character entities to do the escaping,
> you'll need to escape the ampersand itself first, just as in HTML/XML, and then
> unescape it last.

Oh, I forget this and now I add it.
Thank you Myk, thanks!

Comment 19

[Bo Yang]

Attachment: The new patch agaist the cvs trunk

Seth, please review this patch. Thanks!

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 273792 [details] [diff] [review]
The new patch agaist the cvs trunk

putting in my review queue

Comment 21

[(not reading, please use [e-mail] instead)]

Bo, sorry for the delay.  I'm going to move this to M8.

Comment 22

[Bo Yang]

Could somebody please tell me how is the patch for this bug going now? Thanks!

Comment 23

[(not reading, please use [e-mail] instead)]

hey bo, sorry for the delay in getting to your patch.

this will probably not make m9, as we are limiting our efforts to blockers.

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 273792 [details] [diff] [review]
The new patch agaist the cvs trunk

my apologies, Bo.

re-assign review to dietrich.

this bug has been bit rotting in my review queue for a while.

Comment 25

[Dietrich Ayala (:dietrich)]

Comment on attachment 273792 [details] [diff] [review]
The new patch agaist the cvs trunk


>@@ -730,6 +734,25 @@
>   },
> 
>   /**
>+   * Unescape the colon and enter in the url
>+   * &cl;  =  :
>+   * &et;  =  \n
>+   */
>+  unEscape: function PU_unEscape(str) {
>+    str = str.replace("&cl;",":");
>+    str = str.replace("&et;","\n");
>+    str = str.replace("&am","&");
>+    return str;
>+  },
>+
>+  escape: function PU_escape(str) {
>+    str = str.replace("&","&am");
>+    str = str.replace("\n","&et");
>+    str = str.replace(":","&cl");
>+    return str;
>+  },
>+

why not use standard url encoding?

>@@ -741,6 +764,7 @@
>   unwrapNodes: function PU_unwrapNodes(blob, type) {
>     // We split on "\n"  because the transferable system converts "\r\n" to "\n"
>     var nodes = [];
>+    var item = { title: null, uri: null , annons: [], type: null };

s/annons/annos/

>     switch(type) {
>       case this.TYPE_X_MOZ_PLACE:
>       case this.TYPE_X_MOZ_PLACE_SEPARATOR:
>@@ -772,6 +796,27 @@
>             nodes.push({ uri: uriString, title: uriString });
>         }
>         break;
>+      case this.TYPE_X_ANNOTATED_URL:
>+        // extract the data
>+        item.type = this.TYPE_X_ANNOTATED_URL;
>+        var parts = blob.split("\n");
>+        if (parts.length < 2) {
>+          LOG("Wrong arguments of x-annotation-url");
>+          throw Cr.NS_ERROR_INVALID_ARG;
>+        }
>+        for (var i = 0; i < parts.length; i++) {
>+          if (i == 0)
>+            item.title = parts[i];
>+          else if (i == 1)
>+            item.uri = this._uri(parts[i]);
>+          else {
>+            var fields = parts[i].split(":");
>+            item.annons.push({ type: parseInt(fields[0]), name: this.unEscape(fields[1]), value: this.unEscape(fields[2]), flags: parseInt(fields[3]), expires: parseInt(fields[4]) });

s/annons/annos/

a couple of other things:

1. please add xpcshell tests

2. this will likely be rotted by bug 384370 when it lands

3. make a case for why this should be accepted by drivers this late in the cycle

Comment 26

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 27

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.