412002 - should be able to edit tags for multiple bookmarks at the same time

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

A user should be able to select multiple items in a bookmark view, and be able to edit the tags field, affecting the change on all the selected items.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Here are the ways I think we should consider exposing the ability to tag multiple pages at the same time:

Adding tags:

-multiple selection, editing the tag field in the properties pane
-multiple selection, drag operation to the tag in the left pane
-multiple selection, copy and paste to a tag folder

Removing tags gets a little more complicated because it isn't clear if you are deleting the bookmark from the tag folder or totally deleting the bookmark.  Here is first pass:

-in a tag folder multiple selection, hit delete key to remove that tag (with a dialog "are you sure want to remove this tag?")
-in a tag folder multiple selection, right click and select "Remove", which is next to "Delete." Dialog says "are you sure you want to remove this tag?")

Comment 2

[Alex Faaborg [:faaborg] (Firefox UX)]

Should also be under removing:

-multiple selection, editing the tag field in the properties pane

Comment 3

[:Gijs (out for now; he/him)]

Reassigning to tjduavis at his request

Comment 4

[Timothy Joseph Duavis]

Attachment: Sample mockup1 of my first iteration fix ready for reviews and feedbacks

This my mockup of two bookmark folder items selected and the properties display on the bottom shows the ability to enter tag values. The tag values entered will apply to the items selected.

Comment 5

[Timothy Joseph Duavis]

Attachment: Sample mockup2 of my first iteration fix for review and feedback

This my second mockup of more than two bookmark items selected and the properties display on the bottom shows the ability to enter tag values. The “tag selection list” has also been expanded to show that this feature should be provided. Either he tag values entered or the selected list items will apply to the bookmark items selected.

I have not coded this mockup or the previous one yet. I just wanted to show my intentions so please let me know if there are any problems or additional comments.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

The properties pane will change quite a bit when you go from one item selected to multiple items selected.  Usually when you can change only some properties of multiple items, the fields you can't change just disable.  This might work better, especially if we later have something else that you will be able to change for multiple bookmarks at the same time.

Comment 7

[Grey Nicholson (they/them)]

(In reply to comment #1)
> -in a tag folder multiple selection, hit delete key to remove that tag (with a
> dialog "are you sure want to remove this tag?")
> -in a tag folder multiple selection, right click and select "Remove", which is
> next to "Delete." Dialog says "are you sure you want to remove this tag?")
> 

Are confirmation dialogues necessary? Would allowing the user to easily undo (Undo command in the context menu, usual keyboard shortcut) be simpler for most use cases (i.e. when you really do want to remove the tag)?

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

I agree that undo is far superior to asking first, but in this case we are using the confirmations to help disambiguate what is about to happen (remove tag vs. delete bookmark).  We could pull the dialog from the contextual action, since the user had a chance to choose between the two options, for hitting delete though, it really isn't clear which action is going to happen.

Comment 9

[Timothy Joseph Duavis]

Attachment: Prototype patch release 0.1 ready for review

Although remove feature was not part of the intended goal for this prototype patch, it was included as the feature became intuitive. In this version, the remove functionality happens inclusively with the items selected. Final note is that the bookmark folder that contains a places location value will work when multiple items are selected.

The following are two problems that I got stuck with:
Problem #1
- When two or more bookmark items are selected and a new tag value has been entered, if the user selects another bookmark item the previous selected bookmark items do not get updated. Currently this feature will only work when I include debug "alert" statements.
- I will plan to investigate this problem further. I suspect that this maybe a design issue between the onTagsFieldBlur function that calls _updateTags so please provide as much feedback.

Problem #2
- When multiple bookmark items are selected should the collection of existing tags be included?
- For example if I have the following bookmark items selected:
- item i), with tag basketball
- item ii), with tags basketball, sports,
If an update occurs should the results of the tags both item i) and ii) be basketball and sports? The update can occur when a user triggers the onTagsFieldBlur.

Comment 10

[Dietrich Ayala (:dietrich)]

Comment on attachment 301242 [details] [diff] [review]
Prototype patch release 0.1 ready for review

great work for a first-cut. first change: please clean up all your debugging code before submitting for review. makes it easier to see the actual changes that way :)

>@@ -90,76 +93,140 @@ var gEditItemOverlay = {
>   /**
>    * Initialize the panel
>    */
>   initPanel: function EIO_initPanel(aItemId, aInfo) {
>     const bms = PlacesUtils.bookmarks;
> 
>     this._folderMenuList = this._element("folderMenuList");
>     this._folderTree = this._element("folderTree");
>-    this._itemId = aItemId;
>-    this._itemType = bms.getItemType(this._itemId);
>-    this._determineInfo(aInfo);
>+    // logic to parse the list of items
>+    // potential solution (i) - traverse the length of items array
>+    var multiEditBool = false;
>+    var mixedItems = false; // bookmark folder items and bookmark items
>+    var container;
>+    if (aItemId.length > 0) {

i'd prefer making multi-edit more explicit in a few ways:

1. make |multiEdit| a bool property of the panel. use those instead of whether _itemId has a length property. (maybe give mixedItems this treatment as well)

2. i don't like that the _itemId might be non-scalar, yet retains a singular name. maybe rename _itemId to _itemIds, always treat it as an array, accessing _itemIds[0] when this._multiEdit is false. (same treatment for _uri).

mano spends much more time in this code, so you should also get an opinion from him once this patch is cleaned up a bit more.

>+      // must assign the 'this._itemId so that the Property that it uses wont be affected by an assigned array.
>+      //this._itemId = aItemId[0].itemId;
>+      this._itemId = aItemId;
>+      multiEditBool = true;
>+      var temp = "";
>+      for (var i = 0; i < this._itemId.length; i++) {
>+        this._itemType = bms.getItemType(this._itemId[i].itemId);
>+        temp = temp + this._itemId[i].itemId + ",";
>+        if (this._itemType != Ci.nsINavBookmarksService.TYPE_BOOKMARK) {
>+          mixedItems = true;
>+          i = this._itemId.length;

- hm, mixedItems isn't really the issue, since if all were folders, it's be true even though there's not mixed types. maybe rename hasFolders or something to better describe the condition.

- just use |break| to end the loop.

- you should move this determination down below where you're iterating over the items to populate the _uri property.

>+        }
>+      }
>+      alert(temp);
>+      container = bms.getFolderIdForItem(this._itemId[0].itemId);

not correct when viewing searches, smart folders, etc

also, container is only used for init'ing the folder menulist, which is disabled for multiEdit.

>+    } else {
>+      alert(aItemId);
>+      this._itemId = aItemId;
>+      this._itemType = bms.getItemType(this._itemId);
>+      container = bms.getFolderIdForItem(this._itemId);
>+    }
> 
>-    var container = bms.getFolderIdForItem(this._itemId);
>+    this._determineInfo(aInfo);
>     if (this._itemType == Ci.nsINavBookmarksService.TYPE_BOOKMARK) {
>-      this._uri = bms.getBookmarkURI(this._itemId);
>+      // store the uri as an array
>+      if (multiEditBool) {
>+        var uriArr = [];
>+        for (var i = 0; i < aItemId.length; i++) {
>+          uriArr.push(bms.getBookmarkURI(aItemId[i].itemId)); 

you can get the url via aItemId[i].url
         
>+        }
>+        this._uri = uriArr;
>+        //this._uri = bms.getBookmarkURI(this._itemId);
>+      } else {
>+        this._uri = bms.getBookmarkURI(this._itemId);        
>+      }

actually, this could be done when first iterating over the selection to get the , maybe need to re-arrange to just do all the multi-edit stuff in one block.

>       this._isLivemark = false;
>       if (PlacesUtils.livemarks.isLivemark(container))
>         this._readOnly = true;
>       else
>         this._readOnly = false;
> 
>-      this._initTextField("locationField", this._uri.spec);
>+      // *** check - goal is to append the tags for each uri - checked - must write my own to append the value
>+      // init First then set values iteratively
>+      if (multiEditBool == false) {

if (!multiEditBool)

>       this._initTextField("tagsField",
>                            PlacesUtils.tagging
>                                       .getTagsForURI(this._uri, {}).join(", "),
>                           false);
>+      } else {
>+        var fieldString = "";
>+        var tagsForUriArr = [];
>+        var _tagsForUriArr = [];
>+        for (var i = 0; i < this._uri.length; i++) {
>+          tagsForUriArr = _tagsForUriArr.concat(PlacesUtils.tagging.getTagsForURI(this._uri[i], {}), tagsForUriArr);
>+        } 

this._uri.forEach(function(aURI) {
  tagsForUriArr.concat(
    PlacesUtils.tagging.getTagsForURI(aURI, {}));
});

or something like that. don't need 2 arrays for this.

or could throw this inside the first loop, to get it unique, and skip iterating over the whole array as you are doing below.

tags.forEach(function(aTag) {
  if (!aTag in tagsForUriArr)
    tagsForUriArr.push(aTag);
});

>+        for (var i = 0; i < tagsForUriArr.length; i++) {
>+          if (!fieldString.match(tagsForUriArr[i])) {
>+            if (i == 0)
>+              fieldString = tagsForUriArr[i];
>+            else
>+              fieldString = fieldString + ", " + tagsForUriArr[i];
>+          }
>+        }
>+        this._initTextField("tagsField", fieldString,false);
>+      }
> 
>+      // *** check if there are dependencies with new arrays included - checked no dependencies
>       // tags selector
>       this._rebuildTagsSelectorList();
> 
>+      if (multiEditBool == false) {
>+      //alert("single selection");
>+      // *** do not include
>+      this._initTextField("locationField", this._uri.spec);

indent

>+
>+      // *** do not include
>       this._initTextField("keywordField",
>                           bms.getKeywordForBookmark(this._itemId));
> 
>+      // *** do not include
>       // Load In Sidebar checkbox
>       this._element("loadInSidebarCheckbox").checked =
>         PlacesUtils.annotations.itemHasAnnotation(this._itemId,
>                                                   LOAD_IN_SIDEBAR_ANNO);
>+      }
>+    } else {
>+      if (mixedItems == false) {

} else if (...) {


>       if (tagsToAdd.length > 0) {
>-        var tagTxn = PlacesUtils.ptm.tagURI(this._uri, tagsToAdd);
>-        PlacesUtils.ptm.doTransaction(tagTxn);
>+        if (multiedit) {
>+          //alert("test");
>+          for (var i = 0; i < this._uri.length; i++) {
>+            var tagTxn = PlacesUtils.tagging.tagURI(this._uri[i], tagsToAdd);            
>+          }
>+          PlacesUtils.ptm.doTransaction(tagTxn);
>+        } else {     
>+          PlacesUtils.tagging.tagURI(this._uri, tagsToAdd);

s/tagging/ptm/

if you don't go through the transaction manager, the changes are not undo-able.

>+          var tagTxn = PlacesUtils.ptm.doTransaction(tagTxn);
>+        }
>       }
>+//       alert("tags to remove = " + tagsToRemove.join(":"));
>       if (tagsToRemove.length > 0) {
>-        var untagTxn = PlacesUtils.ptm.untagURI(this._uri, tagsToRemove);
>-        PlacesUtils.ptm.doTransaction(untagTxn);
>+        if (multiedit) {
>+          //alert("test2");
>+          for (var i = 0; i < this._uri.length; i++) {
>+            var untagTxn = PlacesUtils.tagging.untagURI(this._uri[i], tagsToRemove);            
>+          }
>+          PlacesUtils.ptm.doTransaction(untagTxn);
>+        } else {     
>+          var untagTxn = PlacesUtils.tagging.untagURI(this._uri, tagsToRemove);
>+          PlacesUtils.ptm.doTransaction(untagTxn);
>+        }

ditto

>Index: browser/components/places/content/places.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/places.js,v
>retrieving revision 1.123
>diff -u -8 -p -r1.123 places.js
>--- browser/components/places/content/places.js	2 Feb 2008 00:39:05 -0000	1.123
>+++ browser/components/places/content/places.js	4 Feb 2008 07:19:47 -0000
>@@ -232,19 +232,19 @@ var PlacesOrganizer = {
>   onTreeClick: function PO_onTreeClick(aEvent) {
>     var currentView = aEvent.currentTarget;
>     var controller = currentView.controller;
> 
>     if (aEvent.target.localName != "treechildren")
>       return;
> 
>     if (currentView.hasSingleSelection && aEvent.button == 1) {
>-      if (PlacesUtils.nodeIsURI(currentView.selectedNode))
>+      if (PlacesUtils.nodeIsURI(currentView.selectedNode)) {
>         controller.openSelectedNodeWithEvent(aEvent);
>-      else if (PlacesUtils.nodeIsContainer(currentView.selectedNode)) {
>+      } else if (PlacesUtils.nodeIsContainer(currentView.selectedNode)) {
>         // The command execution function will take care of seeing the

the style in places js code is to only use brackets for multiline blocks.


>+      else { // hack
>+        //alert("hack-1");
>+        var selectedNode = contentTree.selectedNode;

unused?

Comment 11

[Giles Weaver]

Bug 412692 (Allow drag&drop for bookmarks to place them onto a tag under Tags) is related to this bug.

Comment 12

[Timothy Joseph Duavis]

Attachment: WIP - Prototype patch release 0.2 ready for review

Based on the comments you made on Prototype patch release 0.1 (bug-412002-patch-0.1.txt) the following comments below describes my current status. There are still problems that I'm currently looking into, but I think it would be best to provide an update on my progress. 

Changes Made:

1. Cleaned up most of my debug code and refactored.
2. Changed the _uri / _id scalar variables to be non scalar completely

Still Pending:

this._uri.forEach(function(aURI) {
  tagsForUriArr.concat(
    PlacesUtils.tagging.getTagsForURI(aURI, {}));
});

or something like that. don't need 2 arrays for this.

or could throw this inside the first loop, to get it unique, and skip iterating
over the whole array as you are doing below.

tags.forEach(function(aTag) {
  if (!aTag in tagsForUriArr)
    tagsForUriArr.push(aTag);
});

1. Still don't quite understand how to make this work. I'm still in the process of optimizing.

+  initPanel: function EIO_initPanel(aItemId, aInfo, mulipleItems) {  
+    // XXXtjduavis Hack: conflict with onselect event that calls
+    // onContentTreeSelect in places.js
+    // work in progress; from various function tests,
+    // there are some inconsistent results.      
+    if (this._multiEdit || this.itemId != -1)
+      this._updateTags();
2. onblur event conflicting with onselect from Places.xul. The above is my proposed solution but requires more testing.


+      // xxxtjduavis WIP: this._itemIds[0].uri does not return a nsIURI
+      // which is needed in _updateTags
+      // this._uris.push(this._itemIds[0].uri);
3. I could not get the url via aItemId[i].url, as I got an undefined property. Did some research from the Places API's and found that .uri is a defined property, however it does not return a nsIURI object.


Please feel free to let me know if there any ideas and suggestions or if there are anymore concerns that you need me to adjust.

Comment 13

[Timothy Joseph Duavis]

Attachment: Prototype patch release 1 ready for review

1. Resolved the onContentTreeSelect conflict in places.js with the _updateTags that is called from the onTagsFieldBlur event in editBookmarkOverlay.js.
	Example:
		1. Select multiple bookmarks.
		2. Enter a tag to update the selected bookmarks.
	`	3. Select another bookmark.
	Results:
		Bookmarks from step 1 are updated with tag value from step2.
2. Performed additional tests to ensure integrity (unit and functional).
3. During the tests I assumed the following functionality: multiple bookmarks will be updated based on combined tags. Please comment.
	Example:
		1. Select a bookmark (bmk1). This bookmark contains the following tags: aa, bb.
		2. Select another bookmark (bmk 2). This bookmark contains the following tags: a, b, c.
		3. Trigger the updateTags function by either selecting another bookmark/item, or cause the tags field to be unblurred.
	Results:
		Both bookmarks contain the following tags: a, aa, b, bb, c

Comment 14

[Recall]

Timothy, will crtl-a work for selecting multiple bookmarks with this patch?

Comment 15

[Timothy Joseph Duavis]

Karl, I believe it should work. I will run tests on this and provide an update in regards to feasibility.

Comment 16

[Recall]

Timothy, thanks I look forward to the patch.

Comment 18

[Timothy Joseph Duavis]

(In reply to comment #16)
> Timothy, thanks I look forward to the patch.
> 
Karl, based on my review the following is the status of the current patch. I will work to provide an updated patch that seeks to resolve the identified problem.

- ctrl+a within the tag folder, causes unexpected results: the bookmarks looses their tags.
- standard multiple select key stokes -- ctrl+click and ctrl+shift + click comibnations, also causes unexpected results.
- the key stroke, ctrl+a works with regular folders (essentially, non-tags subfolders).

STR:
- tag a set of bookmarks with the value of "a".
- click the "a" tag subfolder.
- enter ", b" to include a new tag.
- select the "search bookmarks" text box to update the tags (ie. trigger the onblur event)

Results: The bookmarks within the "a" tag subfolder disappears. The bookmark location within the regular folders remain the same.

Comment 19

[Timothy Joseph Duavis]

Attachment: WIP - Prototype patch release 1.1

Ok. I seem to have one problem with the current block. During a removal of a tag  in the tag folder when all bookmarks are selected, if the tag being removed is the same value of the given tags then the total bookmarks selected will not be completely removed. There will be remaining bookmark items left. Note: this applies to the total amount of bookmarks selected to be greater than 2.

All other scenarios seem to work including the ctrl+a key stroke.

STR:
- tag a set of bookmarks (greater than 2) with the value of "a".
- click the "a" tag subfolder.
- select all the bookmark item by using any multiple selection key stroke (including ctrl+a)
- delete the "a" in the tag textbox field
- select the "search bookmarks" text box to update the tags (ie. trigger the
onblur event)

Results:
Remaining bookmarks are left. The update was not complete.

Comment 20

[Timothy Joseph Duavis]

Furthermore, the only thing that I changed in the patch was to include within the _updateTags: function in the editBookmarksOverlay.js a tags folder identifier when there is a multiple edit during the collection of tagsToRemove and tagsToAdd array variable.

Comment 21

[Recall]

Timothy, thanks for the reply. I am of the opinion that getting this patch in for FF3 is very important for tagging and places intergration plus useability. People really are not going to tag 1000+ bookmarks one by one. Do you think you will be able to get a patch working for final?

Comment 22

[Timothy Joseph Duavis]

Kar(In reply to comment #21)
> Timothy, thanks for the reply. I am of the opinion that getting this patch in
> for FF3 is very important for tagging and places intergration plus useability.
> People really are not going to tag 1000+ bookmarks one by one. Do you think you
> will be able to get a patch working for final?
> 

Yes, I understand the concerns in regards to usability and I think this would provide value to the great improvements of Places.

I believe I can deliver a working patch for final. I am planning to work on it again this weekend. I am just busy with doing away with exams and all right now. By next Sat I should be confident to deliver.

Comment 23

[Recall]

Tim any chance of making this for final?

Comment 24

[Timothy Joseph Duavis]

Karl, sorry I haven't worked on it. Been busy planning a trip. I'm right now out of the country and will not be able to work on it till some time in the end of the  month (may 17/18).

In terms of the patch progress, I still have problems with selecting all the bookmark items in the tags subfolder and updating. The problem when I diagnosed it, was in the following component: 'PlacesUIUtils.ptm'. This seems to not handle multiple updates well. 

Unfortunately I don't think this patch is going to make final due to scheduling reasons. Last time I chatted with Dietrich he said that the post-release would have a better chance of making it into Fx3. 

Like I said, I would like to continue the work but when I get back to Toronto. For formality, I am going to remove the assign status to default.

Comment 25

[Dietrich Ayala (:dietrich)]

Hi Tim, we'd like to get this into Fx 3.1. Do you have time to work on this? If not, we'll re-assign. Thanks!

Comment 26

[Recall]

Dietrich, I think someone needs to get in contact with the maker of Tagging For Multiple Bookmarks https://addons.mozilla.org/en-US/firefox/addon/7463

I use the above add-on and it seems to work well. However the accessibility to the ui could be better, so when highlighting multiple ones, the tag box shows up below like when editing bookmark names.

Comment 28

[Nochum Sossonko [:Natch]]

Is anyone working on this, I have a patch updated to trunk in my tree (a little hacky but works great), should I attach it?

Comment 29

[cmtalbert]

(In reply to comment #28)
> Is anyone working on this, I have a patch updated to trunk in my tree (a little
> hacky but works great), should I attach it?
I think the answer to this question is almost always yes, yes, please attach your patch.  The other patch here is also a work-in-progress. So, another patch might help unclog the tubes a bit.

Comment 30

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 1

Alright here's my patch, now I added my own UI somewhat, didn't know exactly what was wanted but it can be worked on. This patch does not introduce the semi-colon separator, but if that's wanted I can definitely add that in later. (seems like it should be simple enough).

Comment 31

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 1.1

Some code clean-up after some NITs I saw, plus I split off some of the work to functions, also fixed-up selection and focus issues when switching between trees and multi/single tag editing...

Comment 32

[Nochum Sossonko [:Natch]]

Comment on attachment 339139 [details] [diff] [review]
Patch ver. 1.1

new patch coming...(dnd, uses built-in funcs etc..)

Comment 33

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2

Most of this bug is implemented already in places, i.e. the tag dnd and tag delete etc. all works already. This patch just adds the ability to multi-select and edit tags. This version fixes some problems with the updateMultipleTags funtion and uses getSelectionNodes of the tree itself, same otherwise.

If I'm tagging the wrong person for review (or if I need ui-review, sr, and what not) feel free to set it for me ;).

Comment 34

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2.1 (final, I hope!!)

This just updates _updateTags to be the deciding function on whether to use _updateSingleItem or _updateMultipleItems, also this patch addresses a bug in the current places implementation, when you add a (predefined) tag via the textbox and right after click the checkbox to add the same tag, the tag gets added twice.. just added a check in the form of tags.indexOf(label) just like when removing a tag.

Comment 35

[Dietrich Ayala (:dietrich)]

Hey Natch, thanks for working on this. I've been out of town for the last week, but will review this asap.

Comment 36

[Dietrich Ayala (:dietrich)]

Comment on attachment 339410 [details] [diff] [review]
Patch ver. 2.1 (final, I hope!!)

>   initPanel: function EIO_initPanel(aFor, aInfo) {
>+    var aForNodeList;
>+    if (aFor.length) {
>+      aForNodeList = aFor;
>+      aFor = aForNodeList[0];
>+    }

aForNodeList is misleading, as it's an array of ids passed in, not an array of nodes. please rename with more accuracy.

also, please update the js doc for aFor.

>-      this._initTextField("tagsField",
>-                           PlacesUtils.tagging
>-                                      .getTagsForURI(this._uri, {}).join(", "),
>-                          false);
>+      if (!aForNodeList) {
>+        this._initTextField("tagsField",
>+                             PlacesUtils.tagging
>+                                        .getTagsForURI(this._uri, {}).join(", "),
>+                            false);
>+      }

while you're there, could you please just put the tags in a local var to avoid this crazy indentation?

>-  _updateTags: function EIO__updateTags() {
>+  _updateTags: function EIO_updateTags() {

nit: please return the underscore

>+    if (this._multiEdit)
>+      this._updateMultipleItems();
>+    else
>+      this._updateSingleItem();
>+  },

these new functions should be named so that it's clear they're updating only tags.

actually, there's a lot of duplicated code in those two functions. maybe combine them, and create a local array of URIs at the top?

>+      if (tagsToRemove.length > 0) {
>+        PlacesUIUtils.ptm.beginBatch();
>+        var untagTxn;
>+        for (var i = 0; i < this._uris.length; i++) {
>+          untagTxn = PlacesUIUtils.ptm.untagURI(this._uris[i], tagsToRemove);
>+          PlacesUIUtils.ptm.doTransaction(untagTxn);
>+        }
>+        PlacesUIUtils.ptm.endBatch();
>+      }
>+      this._allTags = tags;
>+      this._tags = [];
>+      for (i = 0; i < this._uris.length; i++)
>+        this._tags[i] = PlacesUtils.tagging.getTagsForURI(this._uris[i], {});

should probably put the whole operation in the batch, for performance. note that you can create an array of transactions and execute them via an aggregate batch, instead of manually beginning and ending the batch. there's an example of doing so in that file.

>@@ -314,17 +314,17 @@ var PlacesOrganizer = {
>   /**
>    * Handle focus changes on the trees.
>    * When moving focus between panes we should update the details pane contents.
>    * @param   aEvent
>    *          The mouse event.
>    */
>   onTreeFocus: function PO_onTreeFocus(aEvent) {
>     var currentView = aEvent.currentTarget;
>-    var selectedNode = currentView.selectedNode;
>+    var selectedNode = currentView.selectedNode ? [currentView.selectedNode] : this._content.getSelectionNodes();
>     this._fillDetailsPane(selectedNode);
>   },

nit: keep line length at 80 chars max please

nit: for clarity, pluralize the local var name

>@@ -588,16 +588,23 @@ var PlacesOrganizer = {
>      * the wasminimal attribute here is used to persist the "more/less"
>      * state in a bookmark->folder->bookmark scenario.
>      */
>     var infoBox = document.getElementById("infoBox");
>     var infoBoxExpander = document.getElementById("infoBoxExpander");
> #ifdef XP_WIN
>     var infoBoxExpanderLabel = document.getElementById("infoBoxExpanderLabel");
> #endif
>+    if (!aNode) {
>+      infoBoxExpander.hidden = true;
>+#ifdef XP_WIN
>+      infoBoxExpanderLabel.hidden = true;
>+#endif
>+    return;
>+    }

nit: fix indent here

>   onContentTreeSelect: function PO_onContentTreeSelect() {
>-    if (this._content.treeBoxObject.focused)
>-      this._fillDetailsPane(this._content.selectedNode);
>+    if (this._content.treeBoxObject.focused) {
>+      var itemIds = this._content.getSelectionNodes();
>+      this._fillDetailsPane(itemIds);
>+    }
>   },

getSelectionNodes returns array of nodes, so please rename the local var to reflect that. or could even remove it altogether.

>-  _fillDetailsPane: function PO__fillDetailsPane(aSelectedNode) {
>+  _fillDetailsPane: function PO__fillDetailsPane(nodeList) {

please retain the "a" prefix style of indicating input parameters

>     var infoBox = document.getElementById("infoBox");
>     var detailsDeck = document.getElementById("detailsDeck");
>-
>+    var aSelectedNode = nodeList.length == 1 ? nodeList[0]: false;

nit: null would be more appropriate than false here.

>     // If a textbox within a panel is focused, force-blur it so its contents
>     // are saved
>     if (gEditItemOverlay.itemId != -1) {
>       var focusedElement = document.commandDispatcher.focusedElement;
>       if ((focusedElement instanceof HTMLInputElement ||
>            focusedElement instanceof HTMLTextAreaElement) &&
>           /^editBMPanel.*/.test(focusedElement.parentNode.parentNode.id))
>         focusedElement.blur();
> 
>       // don't update the panel if we are already editing this node
>       if (aSelectedNode && gEditItemOverlay.itemId == aSelectedNode.itemId &&
>-          detailsDeck.selectedIndex == 1)
>+          detailsDeck.selectedIndex == 1 && !gEditItemOverlay.multiEdit)
>         return;

edit the comment to reflect this change

r-, for the changes previously listed, but overall the patch looks good. i tested, and the interaction works fine. i'd also like to see the context-menu features from alice's extension, but will spin that off into a new bug i think.

Comment 37

[Nochum Sossonko [:Natch]]

new patch coming shortly... just wanted to verify that the ui was OK (at least for now) and if this is targeted for beta?

Comment 38

[Nochum Sossonko [:Natch]]

(In reply to comment #36)
>actually, there's a lot of duplicated code in those two functions. maybe
>combine them, and create a local array of URIs at the top?

I could do that, but it would bring up more checks in the form of if(multiedit)...
or if(array[i].length)... b/c the allTags array is used for the currentTags in multiEdit mode and b/c of all the modifications to this._tags and this._allTags in their respective places... should I still do this?

Comment 39

[Dietrich Ayala (:dietrich)]

(In reply to comment #37)
> new patch coming shortly... just wanted to verify that the ui was OK (at least
> for now) and if this is targeted for beta?

yes, ui seemed fine. we'll surely get more feedback once it's in the hand of nightly testers. yes, would like to get this into the next beta, so thanks for the quick turnaround!

(In reply to comment #38)
> (In reply to comment #36)
> >actually, there's a lot of duplicated code in those two functions. maybe
> >combine them, and create a local array of URIs at the top?
> 
> I could do that, but it would bring up more checks in the form of
> if(multiedit)...
> or if(array[i].length)... b/c the allTags array is used for the currentTags in
> multiEdit mode and b/c of all the modifications to this._tags and this._allTags
> in their respective places... should I still do this?

ack, right, no it's fine to keep them separate. please do update the function names though.

Comment 40

[Nochum Sossonko [:Natch]]

Attachment: Addresses Comments

Here's the patch, I fixed all of the above comments (and it uses aggregatedTransactions now), but my tree was burning and is now somewhat handicapped :(), so my testing was not entirely complete, but for what it's worth it works the same as before.

Thanks for the review!

Comment 41

[Nochum Sossonko [:Natch]]

Attachment: Whoops, left something over;)

Sorry bout that forgot to take out begin and end batch!

Comment 42

[Nochum Sossonko [:Natch]]

Comment on attachment 340496 [details] [diff] [review]
Whoops, left something over;)

Sorry bout all the corrections, left another patch in with this one... I'll post a new one ASAP, as soon as I'm done rebuilding. Also, this may be due to my broken build, it seemed to run slower now that I'm using aggregateTransactions... will update when I test for real! ;)

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+2]]

What about unit tests?

Comment 44

[Dietrich Ayala (:dietrich)]

(In reply to comment #43)
> What about unit tests?

there are no API-level changes in this patch. a chrome mochitest with synthesized events is possible to test the UI i guess, but it'd be laborious enough that i'd prefer the time be spent elsewhere... unless you're going to write it Henrik ;)

it's quite simple and fast to test this via litmus, so i recommend that route. switching flags.

Comment 45

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2.2

Here's the patch with comments addressed. I didn't use aggregateTransactions as it seems to work slower than just itterating and also has some weird effects when undoing and redoing a few times, editing 250 bookmarks with this patch hangs the library for a little (then again so does adding 250 bookmarks), but maybe this will be sped up by the move to the new sqlite? I have some optimizations in mind, but I won't be able to implement them till prob sunday, so this is what I have for now...

Comment 46

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #44)
> > What about unit tests?
> 
> there are no API-level changes in this patch. a chrome mochitest with
> synthesized events is possible to test the UI i guess, but it'd be laborious
> enough that i'd prefer the time be spent elsewhere... unless you're going to
> write it Henrik ;)

Sorry, that was my fault. I wanted to ask for a Mozmill test. After talking with Clint I'll re-add the in-testsuite? flag.

Comment 47

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2.2 alternative

I guess you can make the final decision, but this is a bit faster in that it only iterates over the selection once, and sets all the values at that time. I also think sqlite upgrade greatly speeds-up the bulk tagging, too bad it got backed out :(. (I had some other optimizations in mind like defining an ondeselect and an onaddselect but I'm not totally confident that they'll really speed things up so much).

Comment 48

[Nochum Sossonko [:Natch]]

Attachment: Patch ver. 2.2 alternative

Small change from the other alternative (which doesn't work ;)), since were setting multiEdit from places.js, we can safely check against it in initPanel...

Comment 49

[Dietrich Ayala (:dietrich)]

(In reply to comment #47)
> Created an attachment (id=340766) [details]
> Patch ver. 2.2 alternative
> 
> I guess you can make the final decision, but this is a bit faster in that it
> only iterates over the selection once, and sets all the values at that time. I
> also think sqlite upgrade greatly speeds-up the bulk tagging, too bad it got
> backed out :(. (I had some other optimizations in mind like defining an
> ondeselect and an onaddselect but I'm not totally confident that they'll really
> speed things up so much).

I don't think the perf concern here outweighs the increased burden on the caller, and the complication of the API to init the panel. Please revert back to passing the array of item ids in. I also prefer the panel itself making the determination of multi-edit. As it can determine the quantity of items being edited, it is redundant to also have to specify whether the panel is in multi-edit mode.

Also, please avoid ambiguous variable names such as "aObject", in favor of actually describing the contents of the object.

Comment 50

[Dietrich Ayala (:dietrich)]

Comment on attachment 340848 [details] [diff] [review]
Patch ver. 2.2 alternative

r-, see comment #49.

Comment 51

[Nochum Sossonko [:Natch]]

(In reply to comment #50)
> (From update of attachment 340848 [details] [diff] [review])
> r-, see comment #49.

In that case I'll just go with attachment (id=340567), that is the revised patched with itemIds as an Array and all other comments addressed.

Comment 52

[Dietrich Ayala (:dietrich)]

Comment on attachment 340567 [details] [diff] [review]
Patch ver. 2.2


>+  _getCommonTags: function(arrIndex) {

nit: please prefix parameters with "a" per the style in the rest of this file.

>   onContentTreeSelect: function PO_onContentTreeSelect() {
>-    if (this._content.treeBoxObject.focused)
>-      this._fillDetailsPane(this._content.selectedNode);
>+    if (this._content.treeBoxObject.focused) {
>+      this._fillDetailsPane(this._content.getSelectionNodes());
>+    }
>   },

nit: unnecessary brackets

> 
>-  _fillDetailsPane: function PO__fillDetailsPane(aSelectedNode) {
>+  _fillDetailsPane: function PO__fillDetailsPane(aNodeList) {
>     var infoBox = document.getElementById("infoBox");
>     var detailsDeck = document.getElementById("detailsDeck");
>-
>+    var aSelectedNode = aNodeList.length == 1 ? aNodeList[0]: null;

nit: space before colon

r=me otherwise

Comment 53

[Nochum Sossonko [:Natch]]

Attachment: [for-checkin]Nits Addressed

Please check-in whenever you're ready. If there's anything else, e.g. any small nits or what not, please if anyone can do it for me as I won't be available till after code freeze. Thank you.

Comment 54

[Dietrich Ayala (:dietrich)]

pushed: http://hg.mozilla.org/mozilla-central/rev/74413bbdd4db

will file a followups for bulk editing via the context menu, and the other scenarios in comment #1.

Comment 55

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b1pre) Gecko/20080930022236 Minefield/3.1b1pre

added test cases to litmus for add and removal of tags to multiple bookmarks via the library properties pane

Comment 56

[Nochum Sossonko [:Natch]]

Should bulk tag editing be allowed for HISTORY items as well? bug 410196 seems to imply that it's fine to tag HISTORY items, although it's a little more involved as all the items would have to be added as bookmarks... Should I file another bug about that?

Comment 57

[Dietrich Ayala (:dietrich)]

(In reply to comment #56)
> Should bulk tag editing be allowed for HISTORY items as well? bug 410196 seems
> to imply that it's fine to tag HISTORY items, although it's a little more
> involved as all the items would have to be added as bookmarks... Should I file
> another bug about that?

yes please file a new bug, thanks!

Comment 58

[Nochum Sossonko [:Natch]]

(In reply to comment #57)
> yes please file a new bug, thanks!

bug 459438.. I'll stop spamming this one now ;)

Comment 59

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #55)
> added test cases to litmus for add and removal of tags to multiple bookmarks
> via the library properties pane

Tracy, can you give us please the ids of the newly added tests?

Comment 60

[Tracy Walker [:tracy]]

(In reply to comment #59)
> (In reply to comment #55)
> > added test cases to litmus for add and removal of tags to multiple bookmarks
> > via the library properties pane
> 
> Tracy, can you give us please the ids of the newly added tests?

I added those test cases a couple of weeks ago. Now they aren't showing, wtf?  investigating....

Comment 61

[Tracy Walker [:tracy]]

They are in Litmus and show in the manage subgroups list, but the changes I make there aren't applying to the test run.

anyway: here are the test cases ID's
https://litmus.mozilla.org/show_test.cgi?id=7039
https://litmus.mozilla.org/show_test.cgi?id=7040
https://litmus.mozilla.org/show_test.cgi?id=7041
https://litmus.mozilla.org/show_test.cgi?id=7042

I'll ping coop when I see him so we can figure out what's going on with the subgroup management.

Comment 62

[Nochum Sossonko [:Natch]]

(In reply to comment #61)

You might want to add a test about adding a tag via the textbox _and_ the check-box simultaneously, this was a problem before, but should've been fixed by this bug (note this was a problem for both multi-editing and single-editing).

Comment 64

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Removing in-testsuite flag for now. Let's create MozMill tests based on the created Litmus tests later.

Comment 65

[Shawn Wilsher :sdwilsh]

in-testsuite is still valuable because mozmill isn't ran after every checkin...

Comment 66

[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