492802 - library details pane is refreshed on focus even when contents haven't changed

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

should check concrete id

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: v1, cleaned

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 377220 [details] [diff] [review]
v1, cleaned

i think there are cases where we do edit the shortcut and not the concreteId... i'm thinking to shortcut to roots probably (could you please check?)

Maybe check for both itemId and concreteItemId?

What about history nodes, those have itemId = -1, but are still shown in the details deck, maybe we can check uris too?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 377220 [details] [diff] [review]
v1, cleaned

clearing request while waiting for answers to comment 2, feel free to reask for review when you're ready.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: v2

addressed comments

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 378115 [details] [diff] [review]
v2

>diff --git a/browser/components/places/content/editBookmarkOverlay.js b/browser/components/places/content/editBookmarkOverlay.js
>--- a/browser/components/places/content/editBookmarkOverlay.js
>+++ b/browser/components/places/content/editBookmarkOverlay.js
>@@ -56,16 +56,20 @@ var gEditItemOverlay = {
>   _initialized: false,
> 
>   // the first field which was edited after this panel was initialized for
>   // a certain item
>   _firstEditedField: "",
> 
>   get itemId() {
>     return this._itemId;
>+  },
>+
>+  get uri() {
>+    return this._uri;
>   },
> 
>   get multiEdit() {
>     return this._multiEdit;
>   },
> 
>   /**
>    * Determines the initial data for the item edited or added by this dialog
>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -653,19 +653,26 @@ var PlacesOrganizer = {
>       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 unless we're
>       // in multi-edit mode
>-      if (aSelectedNode && gEditItemOverlay.itemId == aSelectedNode.itemId &&
>-          detailsDeck.selectedIndex == 1 && !gEditItemOverlay.multiEdit)
>-        return;
>+      if (aSelectedNode) {
>+        var concreteId = PlacesUtils.getConcreteItemId(aSelectedNode);
>+        var nodeIsSame = gEditItemOverlay.itemId == aSelectedNode.itemId ||
>+                         gEditItemOverlay.itemId == concreteId ||
>+                         (gEditItemOverlay.uri &&
>+                          gEditItemOverlay.uri == aSelectedNode.uri);

2 different bookmarks could have the same uri but different titles or tags, this would catch them.
The uri check should include aSelectedNode.itemId == -1

r=me with that fixed

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v2.1 (for checkin)

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/652accbde683