Closed Bug 459438 Opened 11 years ago Closed 11 years ago

Support bulk tagging for multiple history items

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.1b2

People

(Reporter: Natch, Assigned: Natch)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Feel free to assign this to me, unless you have time and beat me to it... ;)
you're free to take this, i suppose target 3.1 since bulk taggin is targetted there
Assignee: nobody → highmind63
Status: UNCONFIRMED → ASSIGNED
Component: Bookmarks & History → Places
Ever confirmed: true
QA Contact: bookmarks → places
Target Milestone: --- → Firefox 3.1
Version: unspecified → Trunk
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?
(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
Attached patch First Pass (obsolete) — Splinter Review
This should do it, it turns out it's a lot easier than I thought :-)
Attachment #342797 - Flags: review?(mak77)
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)?
(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.
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.
Alex: methinks bug 419731 did this, some discussion about this is in bug 410196.
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.
+      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.
Attachment #342797 - Flags: review?(mak77)
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.
Not seeing any problems with this now, probably was a local problem. Made the changes from comment 11.
Attachment #342797 - Attachment is obsolete: true
Attachment #342883 - Flags: review?(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()?
(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 on attachment 342883 [details] [diff] [review]
This should do it

makes sense, r=me. thanks for the patch!
Attachment #342883 - Flags: review?(dietrich) → review+
Priority: -- → P3
Target Milestone: Firefox 3.1 → Firefox 3.1b2
Keywords: checkin-needed
checked in: http://hg.mozilla.org/mozilla-central/rev/5cc686a77232
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
requesting in-litmus, need manual testing of this change.
Flags: in-litmus?
I think an automated test would even be fine.
Flags: in-testsuite?
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
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Depends on: 410196
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.