Closed
Bug 1072833
Opened 10 years ago
Closed 6 years ago
Livemark containers should be represent as query nodes rather than folder nodes
Categories
(Toolkit :: Places, defect, P3)
Toolkit
Places
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: asaf, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxsearch])
Livemarks are semantically queries: their contents are live, temporary and read-only to the user. However, they're internally treated as folders, having the read-only annotation. Read-only folders being roughly the same as queries, the current setup works generally fine. However, the read-only concept is about to be removed (bug 1068671), and that forces us to introduce some fresh hacks to both queries code and UI code. Those being (at least): (1) keeping excludeReadOnlyFolders having the semantics of something like excludeLivemarks); (2) Adding a synchronous getItemAnnotation call to isCommandEnabled for container nodes.
If we find a way to make livemark nodes show up as queries, matching their semantics, we can avoid these hacks. Moreover, some of the ui glitches we have with livemarks (e.g. that they initially show up with the folder icon).
Updated•9 years ago
|
Priority: -- → P3
Comment 1•7 years ago
|
||
While brainstorming over this, I had this insane idea of a query like place:feed=feeduri&uri=siteuri that would also completely remove the annotations need and the hacks we use around to understand if a node is a livemark.
Hard to tell how hard it would be to support such a change in Sync. Likely we could remove the old "folder" and insert a new "uri" record, though from that point on, no idea what happens :\
OS: Mac OS X → All
Hardware: x86 → All
Comment 2•7 years ago
|
||
Kit, any quick thoughts about how hard would it be such a change on the Sync side?
Flags: needinfo?(kit)
Comment 3•7 years ago
|
||
Fairly easy, I think! We sync livemarks as their own record type, so we would only need to change `PlacesSyncUtils`. The record format would remain the same: `{ type: "livemark", feedUri: "...", siteUri: "...", title: "..." }`.
Old versions of Firefox would store the livemark internally as a folder; new versions would store it as a query. Assuming the site and feed URIs stay the same, we wouldn't need to re-sync the livemark when we remove the folder and replace it with a query.
We'd need to change https://searchfox.org/mozilla-central/rev/c9214daa09e3eb8246b1448e469df1652a140825/toolkit/components/places/PlacesSyncUtils.jsm#1407 to distinguish between livemark queries and other queries (which we sync as `type: "query"`), but this is all backward-compatible.
Flags: needinfo?(kit)
Comment 4•7 years ago
|
||
I made a wip patch during the week end, and it sounds like it would be a good change. Though, there is a lot of code to touch before it could be ready, so I cannot give a strict ETA, but I'll bring it on as a side project and I did a good chunk of work already.
The advantages:
- not having to manage special read-only folders (the whole concept may disappear if we fix this and bug 1310295)
- it's trivial to identify a livemark node in views, currently we have to roundtrip to the livemarks service that is async
- it removes 2 annotations. Along with the other changes like bug 1402890, we'd be left with very few annotations cases, that may allow us to more easily move them to a new async API.
- it largely simplifies the annotations handling code around, reducing I/O and views overhead
Assignee: nobody → mak77
Blocks: 1418257, removeAnnotations
Status: NEW → ASSIGNED
Whiteboard: [fxsearch]
Comment 5•7 years ago
|
||
Unassigning until we know the future of livemarks.
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Updated•6 years ago
|
No longer blocks: removeAnnotations
Comment 6•6 years ago
|
||
Wontfixing because live bookmarks are going away - See Bug 1477667.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•