462765 - instant-apply muddies the MRU folder list in the bookmark properties dialog

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

1. add a bookmark, select a folder for it, then hit cancel
2. add another bookmark, open the folder list

Expected: the folder selected in step 1 should not be at the top of the list (or in it at all, if not recently used in a valid manner).

Actual: the folder selected in step 1 is at the top of the list.

Comment 1

[Marco Bonardo [:mak] (Away 10-21 August)]

also, if i read the code correctly that annotation is never removed, so actually you could end up with a thousand annotated folders and use only the top 5.
When setting a new last_used we should remove annotations on oldest ones...

Comment 2

[Marco Bonardo [:mak] (Away 10-21 August)]

actually this is also valid if i create a bookmark with the star panel, choose a folder and then Remove bookmark, the folder is remembered as recently used.

Comment 3

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: patch

uh, while looking at this i noticed we don't unit the panel before undoing the transaction, so we could generate new transactions during undo. Star panel is instead doing this correctly.

So this adds a transaction able to set/unset a generic annotation, I use this transaction to set LAST_USED_ANNO, so on Cancel I restore old LAST_USED values. To do this i've fixed the bogus dialog uninit behaviour and made PlacesUtils.setAnnotationsForItem/URI remove an anno if value is null (there was already an XXX requiring the change)

For now i've preferred not removing editItemDescription and LoadInSidebar transactions, they appear directly linked to bookmark properties, so i think it would be better having them defined apart from the new generic anno implementation.

Both the transaction service change than the setAnnotationsFor have unit tests.

Comment 4

[Marco Bonardo [:mak] (Away 10-21 August)]

also this cleans up old LAST_USED annos that we don't need anymore.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 346062 [details] [diff] [review]
patch

>   onDialogAccept: function BPP_onDialogAccept() {
>+    gEditItemOverlay.uninitPanel(true);
>+    gEditItemOverlay = null;
>     this._endBatch();
>     window.arguments[0].performed = true;
>   },
> 
>   onDialogCancel: function BPP_onDialogCancel() {
>+    // The order here is important! We have to uninit the panel first, otherwise
>+    // changes done as part of Undo may change the panel contents and by
>+    // that force it to commit more transactions.
>+    gEditItemOverlay.uninitPanel(true);
>+    gEditItemOverlay = null;

please add this comment to onDialogAccept as well.

>+  /**
>+   * Returns an object which could then be used to set/unset the
>+   * LAST_USED_ANNO annotation for a folder.
>+   *
>+   * @param aLastUsed
>+   *        Whether to set or unset the LAST_USED_ANNO annotation.
>+   * @returns an object representing the annotation which could then be used
>+   *          with the transaction manager.
>+   */
>+  _getLastUsedAnnotation:
>+  function EIO__getLastUsedAnnotation(aLastUsed) {
>+    var anno = { name: LAST_USED_ANNO,
>+                 type: Ci.nsIAnnotationService.TYPE_INT32,
>+                 flags: 0,
>+                 value: aLastUsed ? new Date().getTime() : null,
>+                 expires: Ci.nsIAnnotationService.EXPIRE_NEVER };
>+
>+    return anno;
>   },

please append "Object" on the method name

>   /**
>+   * Transaction for setting/unsetting an annotation
>+   *
>+   * @param aId
>+   *        id or URI where to set annotation

hrm, it should be clear when setting item vs uri annotations. please make separate apis for page and item annos.

>+   * @param aAnnotation
>+   *        Object representing an annotation, containing the following
>+   *        properties: name, flags, expires, type, mimeType (only used for
>+   *        binary annotations), value.
>+   *        If value is null the annotation will be removed
>+   * @returns nsITransaction object
>+   */
>+  nsITransaction setAnnotation(in nsIVariant aId,
>+                               in nsIVariant aAnnotation);
>+

please append "Data" or "Object" onto the second parameter name.

and add the keyword dev-doc-needed to the bug so MDC gets updated.

>+  setLoadInSidebar: function placesSetLdInSdbar(aItemId, aLoadInSidebar) {

expand the function name. we should fix the other methods in this file as well, when the opportunity is presented.

Comment 6

[Marco Bonardo [:mak] (Away 10-21 August)]

i'm asking blocking because this solves an issue with the new bookmarks dialog that could cause UNDO/REDO work with wrong assumption, that could cause dataloss.

Comment 7

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: patch

fixed comments, splitted method for Item/Page, cleaned up transaction manager a bit.

Notice changes to the idl are additive, so this is not late-compat because i did not change nor remove old methods, i only added 2 new, so add-ons compatibility is ensured.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, I don't think, but we'd take a tested patch.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 353464 [details] [diff] [review]
patch

r=me

Comment 10

[Marco Bonardo [:mak] (Away 10-21 August)]

http://hg.mozilla.org/mozilla-central/rev/99000e832da7

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 353464 [details] [diff] [review]
patch

This is wanted for 3.1 - Marco can comment on risk if needed.

Comment 12

[Marco Bonardo [:mak] (Away 10-21 August)]

risk is medium, but to have a correct undo/redo we need this fix

Comment 13

[Marco Bonardo [:mak] (Away 10-21 August)]

also this patch is needed to fix dataloss in bug 473120

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 353464 [details] [diff] [review]
patch

a191=beltzner

Comment 15

[Marco Bonardo [:mak] (Away 10-21 August)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6de0371245ec

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090118 Minefield/3.2a1pre ID:20090118020617

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090117 Minefield/3.2a1pre ID:20090117033728

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090117 Shiretoko/3.1b3pre ID:20090117020415

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090117 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090117035052

Marco, do we still need the litmus testcase for the fix here?

Comment 17

[Marco Bonardo [:mak] (Away 10-21 August)]

a litmus test covering the mru folder list would be probably useful.

i'm removing dev-doc-needed because we don't have documentation on the transaction manager (Apart the doxygen automatic one, and the idl). I'll add a note for that for the next meeting.

Comment 18

[Aakash Desai [:aakashd]]

A litmus test case is unnecessary for this bug as its more of an edge case than something that should be put into ffts.

Comment 19

[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