459438 - Support bulk tagging for multiple history items

Status: VERIFIED FIXED

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

Description

[Nochum Sossonko [:Natch]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

Now that bug 412002 landed, this support should be added for bulk history tagging (note this involves adding the history item as a bookmark first, see bug 410196)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Nochum Sossonko [:Natch]]

Feel free to assign this to me, unless you have time and beat me to it... ;)

Comment 2

[Marco Bonardo [:mak]]

you're free to take this, i suppose target 3.1 since bulk taggin is targetted there

Comment 3

[Nochum Sossonko [:Natch]]

A little off subject, but I was wondering why not use PlacesUtils.tagging.tagURI for tagging instead of PlacesUIUtils.ptm.tagURI (and the same for untagURI) as the former checks the uri if it's tagged already or if doesn't have the tag while the latter places the burden on the caller?

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> A little off subject, but I was wondering why not use
> PlacesUtils.tagging.tagURI for tagging instead of PlacesUIUtils.ptm.tagURI (and
> the same for untagURI) as the former checks the uri if it's tagged already or
> if doesn't have the tag while the latter places the burden on the caller?

because ptm is the transaction manager, we need to use a transaction to support undo/redo

Comment 5

[Nochum Sossonko [:Natch]]

Attachment: First Pass

This should do it, it turns out it's a lot easier than I thought :-)

Comment 6

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

This is blurring the distinction between history and bookmarks a lot.  For the UI there needs to be some specific indication that bookmarks are being created for these history items if you add a tag.  Also, if we allow users to tag history items (turning them into bookmarks) why not allow them to move the history items to a different folder (also turning them into bookmarks)?

Comment 7

[Nochum Sossonko [:Natch]]

(In reply to comment #6)

This was introduced before this bug already, i.e. if you do it now to a single History item the same bookmarkin thing will happen, that's why I figured, for consistency, this should be implemented for bulk tagging as well.

Comment 8

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

Anyone know the bug number for that?  We need to get this UI problem solved, regardless of which bug we solve it in.  I'm not against bulk tagging or moving history items, but if the properties panel is going to allow for this functionality, we should add a note that a bookmark is being created.

Comment 9

[Nochum Sossonko [:Natch]]

Alex: methinks bug 419731 did this, some discussion about this is in bug 410196.

Comment 10

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

Natch: thanks, I'll file a follow up bug to modify the UI to be a little conceptually clearer that a bookmark is being created.

Comment 11

[Marco Bonardo [:mak]]

+      var tempId;
       for (var i = 0; i < aNodeList.length; i++) {
-        if (!PlacesUtils.nodeIsBookmark(aNodeList[i])) {
+        if (!PlacesUtils.nodeIsBookmark(aNodeList[i]) &&
+            !PlacesUtils.nodeIsURI(aNodeList[i])) {
           detailsDeck.selectedIndex = 0;
           var selectItemDesc = document.getElementById("selectItemDescription");
           var itemsCountLabel = document.getElementById("itemsCountText");
           selectItemDesc.hidden = false;
           itemsCountLabel.value =
             PlacesUIUtils.getFormattedString("detailsPane.multipleItems",
                                              [aNodeList.length]);
           return;
         }
-        itemIds[i] = PlacesUtils.getConcreteItemId(aNodeList[i]);
+        tempId = PlacesUtils.getConcreteItemId(aNodeList[i]);
+        itemIds[i] = tempId != -1 ? tempId : PlacesUtils._uri(aNodeList[i].uri);

i would rather check aNodeList[i].itemId != -1 ? PlacesUtils.getConcreteItemId(aNodeList[i]) : PlacesUtils._uri(aNodeList[i].uri);

then forward review to dietrich please

Are these new bookmarks actually added to unsorted folder?

Alex: for sure that would clarify things, IIRC we decided to do that (allow tagging an history item as a bookmark) time ago. Probably could be in bug
410196 that is still unsolved. If you think it should be fixed before final mark that as a blocker with your UI directions.

Comment 12

[Nochum Sossonko [:Natch]]

Marco: they're added to the unsorted folder, but I think there were some problems when the transaction included bookmarks and history items, however I got no errors in the console (obviously with js.options.showInConsole=true) so I can't really tell, also changes were made to the transaction so that now it uses aggregate transactions, I'll check the patch with that and post a new one with your comment addressed.

Comment 13

[Nochum Sossonko [:Natch]]

Attachment: This should do it

Not seeing any problems with this now, probably was a local problem. Made the changes from comment 11.

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 342883 [details] [diff] [review]
This should do it


>     else if (!aSelectedNode && aNodeList[0]) {
>       var itemIds = [];
>       for (var i = 0; i < aNodeList.length; i++) {
>-        if (!PlacesUtils.nodeIsBookmark(aNodeList[i])) {
>+        if (!PlacesUtils.nodeIsBookmark(aNodeList[i]) &&
>+            !PlacesUtils.nodeIsURI(aNodeList[i])) {
>           detailsDeck.selectedIndex = 0;
>           var selectItemDesc = document.getElementById("selectItemDescription");
>           var itemsCountLabel = document.getElementById("itemsCountText");
>           selectItemDesc.hidden = false;
>           itemsCountLabel.value =
>             PlacesUIUtils.getFormattedString("detailsPane.multipleItems",
>                                              [aNodeList.length]);
>           return;
>         }
>-        itemIds[i] = PlacesUtils.getConcreteItemId(aNodeList[i]);
>+        itemIds[i] = aNodeList[i].itemId != -1 ? aNodeList[i].itemId :
>+                     PlacesUtils._uri(aNodeList[i].uri);
>       }

why'd you drop the use of getConcreteItemId()?

Comment 15

[Nochum Sossonko [:Natch]]

(In reply to comment #14)
> why'd you drop the use of getConcreteItemId()?

thought it was ok if that's what the check is going to be for (comment 11).
ill put it back if you'd rather that (it's kinda pointless in this case, though reading the comment on top of getConcreteItemId).

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 342883 [details] [diff] [review]
This should do it

makes sense, r=me. thanks for the patch!

Comment 17

[Dietrich Ayala (:dietrich)]

checked in: http://hg.mozilla.org/mozilla-central/rev/5cc686a77232

Comment 18

[Dietrich Ayala (:dietrich)]

requesting in-litmus, need manual testing of this change.

Comment 19

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

I think an automated test would even be fine.

Comment 20

[Tracy Walker [:tracy]]

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

in-litmus : using Pair Wise technique I've Covered 32 possible paths with 8 test cases for adding and removing tags for bookmarks and history items

Comment 21

[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