Closed Bug 1193621 Opened 4 years ago Closed 4 years ago

Can't change tag name in library / bookmarks manager

Categories

(Firefox :: Bookmarks & History, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox40 --- affected
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: wagle, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150807085045

Steps to reproduce:

Bookmarks -> Show All Bookmarks (to popup global bookmark editor)


Actual results:

clicked on tag, tried to change name, it ignored me


Expected results:

should have been able to change name (worked earlier today before I installed 40.0)
Thank you for filing this!

Marco is away so I'll at least find a regression window if nobody else does.
Status: UNCONFIRMED → NEW
Component: Untriaged → Bookmarks & History
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
Summary: In "Show All Bookmarks", can't change tag name → Can't change tag name in library / bookmarks manager
Flags: needinfo?(gijskruitbosch+bugs)
Pushlog:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=8a151d59bdec&tochange=d84b62b367b4

Regressed by:
d84b62b367b4	Asaf Romano — Bug 951651 - Make bookmarkProperties, Star UI and Library info pane work with PlacesTransactions. r=mak
IIRC this was done by Mano on purpose cause editing folder shortcuts titles was adding quite a bit of fragile code. But must be verified.
(In reply to Marco Bonardo [::mak] (spotty available until 24 Aug) from comment #3)
> IIRC this was done by Mano on purpose cause editing folder shortcuts titles
> was adding quite a bit of fragile code. But must be verified.

Hm, but now there doesn't seem to be any way to edit tags at all anymore... except going to the individual bookmarks and updating all of them. That doesn't sound right... Mano, can you clarify what's going on here?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(asaf)
yes we need to fix this.

The wrokaround is to select all, add the new tag to all, then remove the old tag.
and likely this happens because we consider these read-only (they are QUERY_CONTAINING_QUERIES internally). I think we should be able to fix this, and write a test too.
> The wrokaround is to select all, add the new tag to all, then remove the old tag.

Wow..  And that now takes a lot of time, from 2 seconds for 100 bookmarks to 30 seconds or so.

Is that a separate bug?
I didn't say it's a solution, I said it's a workaround to obtain the same. This bug must be fixed.
Oh sorry, I meant that your workaround was faster in older versions of firefox (I used to do it normally when I needed to merge when tagnames collided).  This new slowdown might have something to do with the fix that created the main bug, hence my question in comment 7.
Actually, the slowdown seems to be general, I'm beachballing all over the place.  It's a separate bug.
Marco, do you have time to look at this soon?
Flags: needinfo?(mak77)
no, but I guess I should find it.
Assignee: nobody → mak77
Flags: needinfo?(mak77)
Attached patch patch v1Splinter Review
I still need to have a second look, but should do.
Flags: needinfo?(asaf) → in-testsuite+
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/editBookmarkOverlay.js
@@ +546,5 @@
> +        let guid = this._paneInfo.isTag
> +                    ? (yield PlacesUtils.promiseItemGuid(this._paneInfo.itemId))
> +                    : this._paneInfo.itemGuid;
> +        PlacesTransactions.EditTitle({ guid, title: newTitle })
> +                          .transact().catch(Components.utils.reportError);

driveby: if this is async, what happens if you type, and then you type again before the edit is complete? Or can't that happen here?
the function is called on blur
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

For now it should be fine, I should probably use Components.utils instead of Cu but I will fix that before pushing.
Attachment #8654213 - Flags: review?(ttaubert)
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Review of attachment 8654213 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/editBookmarkOverlay.js
@@ +95,5 @@
>      // This pane is read-only if:
>      //  * the panel is not initialized
>      //  * the node is a folder shortcut
>      //  * the node is not bookmarked
>      //  * the node is child of a read-only container and is not a bookmarked URI

Should extend the comment here to mention tags.
Attachment #8654213 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/6b84e7354d9b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Approval Request Comment
[Feature/regressing bug #]: bug 951651
[User impact if declined]: it's no more possible to edit a tag name in the Library
[Describe test coverage new/current, TreeHerder]: unit-test
[Risks and why]: nothing specific we can predict
[String/UUID change made/needed]: none
Attachment #8654213 - Flags: approval-mozilla-beta?
Attachment #8654213 - Flags: approval-mozilla-aurora?
Tracked for 41+ as it's a bad regression.
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Given that the risk assessment on this patch uplift request is not very reassuring, I would like to stabilize this on Aurora for a few days and then uplift to Beta41 if all goes well. Aurora42+ for now.
Attachment #8654213 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Perry, could you please verify this fix works as expected in 09/05 Nightly or later? Thanks in advance.
Flags: needinfo?(wagle)
pff, the usual Array.prototype.includes problem in Aurora and Beta... it just needs to use .contains there (luckily this is the last merge with this issue). I will push the fixed patch tomorrow.
ehr, indexOf... this array/string thing is so boring.
Ritu : all my use cases work..

Thanks all!
Flags: needinfo?(wagle)
Comment on attachment 8654213 [details] [diff] [review]
patch v1

Given that the fix was verified, it seems safe to uplift to Beta41.
Attachment #8654213 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
we need to reland to aurora a fixed patch first, not using .includes on arrays, the same should be landable on beta.
Marco, do you have an eta for this? The gtb of beta 8 is today and we would prefer take this patch in this beta instead of beta 9. Thanks
Flags: needinfo?(mak77)
I was planning to do that now, but I'm not sure what today means, is there a time?
Flags: needinfo?(mak77)
You need to log in before you can comment on or make changes to this bug.