Closed Bug 1378089 Opened 7 years ago Closed 6 years ago

Replace the Bookmark Manager with the Firefox Library in SeaMonkey

Categories

(SeaMonkey :: Bookmarks & History, enhancement)

enhancement
Not set
normal

Tracking

(seamonkey2.58 fixed, seamonkey2.53 fixed, seamonkey2.57esr fixed)

RESOLVED FIXED
seamonkey2.58
Tracking Status
seamonkey2.58 --- fixed
seamonkey2.53 --- fixed
seamonkey2.57esr --- fixed

People

(Reporter: mak, Assigned: frg)

References

(Blocks 3 open bugs)

Details

Attachments

(13 files, 24 obsolete files)

4.36 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
8.95 KB, patch
Details | Diff | Splinter Review
18.00 KB, patch
Details | Diff | Splinter Review
4.38 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
34.92 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
1.38 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
457 bytes, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
108.91 KB, patch
frg
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
768.70 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
245.87 KB, patch
iannbugzilla
: review+
iannbugzilla
: approval-comm-beta+
Details | Diff | Splinter Review
109.70 KB, patch
frg
: review+
Details | Diff | Splinter Review
798.84 KB, patch
iannbugzilla
: review+
Details | Diff | Splinter Review
3.23 KB, patch
Details | Diff | Splinter Review
In Bug 1071513 we'll try to enable the new async Transactions in Nightly.

Transactions are used by the UI to allow for undo/redo functionality in bookmarks, and so far they lived in PlacesUtils (see the various transaction objects).

Moving towards async bookmarking, the old transactions can't work, since they are async, thus we have a new PlacesTransactions.jsm module that implements async transactions and uses the async bookmarking API internally.

This requires fixing most of the ui code and tests to work asynchronously, and that's what we are doing in Bug 1071513.

For Firefox 56, you could keep the browser.places.useAsyncTransactions pref to false, it will just keep working, though from 57 it's likely old APIs will start being removed.
async transactions are enabled in Nightly. if things work correctly, they may be enabled by default in 57, and after that point the old transactions code may go away. Also the old synchronous bookmarking API (or a good chunk of it) may go away since transactions were the biggest consumer, along with Sync.
Thanks for the heads up. With all the changes in Fx 57 we will need some time for fixing up SeaMonkey anyway so go ahead.
Blocks: 1387409
Not blocking the feature, but depends on it.
No longer blocks: PlacesAsyncTransact
FYI, we plan to enable the feature in 58, and legacy code removal will likely happen in 59.
Summary: Figure out what to do with bookmarks transactions → Replace the Bookmark Manager with the Firefox Library in SeaMonkey
The patches are currently at 2.53 level and considered alpha quality.

Part 1 add recentwindow.jsm to make things easier if you are only in the browser part.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Part 2 move over the bookmarks files to places.
Part 3 The funny part. Replaces the Bookmarks Manager with the Fx library.

Only tested on Windows and 2.53 for now. Need to adapt to 2.57 and pull in the latest changes.

Things to check/do: 

- private browsing: does privatebrowsingutils work in the browser part or should gPrivate be used.
- Left in parts we don't have.
- css check
- Linux
- OSX
- Breaking up the patch into smaller parts. Ideas welcome.
Depends on: 1433962
Missed the new tag.png for osx
Attachment #8946354 - Attachment is obsolete: true
Minor cleanups
Attachment #8946358 - Attachment is obsolete: true
Some new fixes just to keep track. Lightly tested with async bookmarks transactions set in prefs. Works so far. Could probably be considered beta for Windows. I will check out Linux and osx over the weekend.

All patches need Bug 1433962 in the tree and a minor rebase because I have some other patches in my local tree.
Attachment #8946353 - Attachment is obsolete: true
Attachment #8946740 - Attachment is obsolete: true
Tested with 2.53 under Windows Linux and osx with both sync and async paths. For 2.57 only async will remain in the code. Might need some minor rebasing because of other patches in my local tree.
Attachment #8946742 - Attachment is obsolete: true
Attachment #8946747 - Attachment is obsolete: true
Tests likely broken.
Example required l10n changes for de. Not sure how we should handle access keys in the final 2.57 patch. Keep current ones or align with Fx if this doesn't clash with existing keys.
Depends on: 796994
Some small tweaks
Attachment #8948126 - Attachment is obsolete: true
Bleeding edge comm-central version part 1
Bleeding edge comm-central version part 2

I moved browser-places.js to browser. There is nothing in its final version suitable for mail or news.
Rebased patch from 2.53 / 56
Essentially the changes between 56 and 60 backported. I still miss 2 or 3 non essential ones but everything seems to work fine now. Without the sidebar and mail/news in 2.57 I was only able to test the browser part.
Blocks: 1439220
history icons fixed and css cleaned up. 2.53 version
Attachment #8950690 - Attachment is obsolete: true
"Final" versions for part 1 to 6. Lets discuss it as the meeting.
Attachment #8950702 - Attachment is obsolete: true
Flags: needinfo?(iann_bugzilla)
Attachment #8950703 - Attachment is obsolete: true
Attachment #8950705 - Attachment is obsolete: true
Attachment #8950709 - Attachment is obsolete: true
Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query".
Port Bug 1423896 "Make the All Bookmarks folder for the left pane a virtual query". Second part.
osx build fix. Missed filters.svg
Took out one location variable too many which broke smart bookmark recreation in nsSuiteGlue.js
Attachment #8951972 - Attachment is obsolete: true
Attachment #8951975 - Attachment is obsolete: true
Blocks: 942937
Rebased after Bug 1436605 landed.
Attachment #8951974 - Attachment is obsolete: true
Rebased after Bug 1436605 landed.
Attachment #8953461 - Attachment is obsolete: true
Attachment #8951976 - Attachment is obsolete: true
Comment on attachment 8948123 [details] [diff] [review]
1378089-part1-recentwindow-253.patch

LGTM r=me
Attachment #8948123 - Flags: review+
Comment on attachment 8951973 [details] [diff] [review]
1378089-part1-recentwindow-257.patch

LGTM r=me
Attachment #8951973 - Flags: review+
Comment on attachment 8955773 [details] [diff] [review]
1378089-part2-bookmarks-257.patch

LGTM r=me
Attachment #8955773 - Flags: review+
Comment on attachment 8948124 [details] [diff] [review]
1378089-part2-bookmarks-253.patch

LGTM r=me
Attachment #8948124 - Flags: review+
Comment on attachment 8953460 [details] [diff] [review]
1378089-part3-bookmarks-253.patch

f=me though we will need some fixes or follow-ups.
When I start the manager I get the following error:
Timestamp: 3/4/18, 8:07:16 PM GMT
Warning: ReferenceError: reference to undefined property 0
Source File: chrome://communicator/content/places/treeView.js
Line: 354
and when I start from Go -> History I get that and also:
Timestamp: 3/4/18, 8:09:21 PM GMT
Warning: ReferenceError: reference to undefined property 0
Source File: chrome://communicator/content/places/treeView.js
Line: 276

The UI is missing the column selection widget (yes, right click on any column does give you that menu but would be nice to keep the widget)
The column selection widget is missing keyword as an option but also the "Restore Column Order" option.

In the History part, if you select multiple pages and then try to Bookmark them, nothing happens, no error either (doesn't work in Firefox either).

The context menu for the History items, is missing some items so for a link from the site survey.example.com, I'd expect:
Delete
Delete History for survey.example.com
Delete History for *.example.com
Select All

Instead I get:
Delete Page
Forget About This Site

Do we want to add Delete History ones and Select All back in?
Flags: needinfo?(iann_bugzilla)
Attachment #8953460 - Flags: feedback+
Thanks for the feedback.

> When I start the manager I get the following error:

If using 2.53 could you add in suite\browser\browser-prefs.js:

// Whether useAsyncTransactions is enabled or not.
// Currently we only enable them for nightly.
#ifdef NIGHTLY_BUILD
pref("browser.places.useAsyncTransactions", true);
#else
pref("browser.places.useAsyncTransactions", false);
#endif

Missed that. Both codepaths are tested and should work. sync codepatch and the pref is removed in 2.57.

> The UI is missing the column selection widget (yes, right click on any column does give you that menu but would be nice to keep the widget)

Will check it.

> The context menu for the History items, is missing some items so for a link from the site survey.example.com, I'd expect:

No longer works. Already severely broken in 2.49.2. See also Bug 1246424.

> Select All back in?

I don't miss it. We have clear browsing history in the Preferences. Also in clear private data.

Will look at what can be done soon.
FWIW, the existing bookmarks manager in SeaMonkey is already a port of the Firefox library, but with a good amount of changes (and cutting out overlap with history). Of course that was a now pretty old version we based this on, from when places was still very young. That said, I'm all for reducing the diffs between SeaMonkey and Firefox (and Thunderbird), the amount of separate code that SeaMonkey has right now is unsustainable for a team as small as the SeaMonkey one.
(In reply to Robert Kaiser from comment #42)
> FWIW, the existing bookmarks manager in SeaMonkey is already a port of the
> Firefox library, but with a good amount of changes (and cutting out overlap
> with history). Of course that was a now pretty old version we based this on,
> from when places was still very young. That said, I'm all for reducing the
> diffs between SeaMonkey and Firefox (and Thunderbird), the amount of
> separate code that SeaMonkey has right now is unsustainable for a team as
> small as the SeaMonkey one.

As long as what you suggest does not result in losing functionality and reducing the user's ability to handle their own data as it used to be. FF seems to reduce as much choices from the user as they can, and SeaMonkey should not follow that path.
(In reply to Sanny from comment #43)
> As long as what you suggest does not result in losing functionality and
> reducing the user's ability to handle their own data as it used to be. FF
> seems to reduce as much choices from the user as they can, and SeaMonkey
> should not follow that path.

I'm pretty sure the team would appreciate your help in this developing and porting work. If you can't contribute, the very small team will have to do with the resources they have and users will need to live with the necessary decision to reduce the work load.
browser-prefs.js:
 Added the missing prefs

utilityOverlay.js:
 whereToOpenLink was duplicated and needed to be merged as per IRC
 openUILinkIn missed the private parameter so it was not possible to open a private browsing window from the Library.

I will look at the column selection widget during the weekend. If it can be re-added I will put it in a patch on top.

Reluctant to bring back the keyword column but will look at it too :)
Attachment #8953460 - Attachment is obsolete: true
2.57 version pref not needed.

Found a new problem here. Doubleclicking a folder from the all bookmarks selection does not open it. Need to check if it is a bug or feature. Already missing a few subsequent patches. 

Works fine with any other item from the left sidebar including bookmarks menu and folders.
Attachment #8955774 - Attachment is obsolete: true
Rebased 2.57 patch. Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed. r+ from iann_bugzilla retained.
Attachment #8955773 - Attachment is obsolete: true
Attachment #8967947 - Flags: review+
Rebased patch for latest changes in comm-central / comm-beta. This needs to go in because it blocks further development. 
Issues mentioned:
- The UI is missing the column selection widget
- keywords missing

should be addressed in followup patches. The patches already need followups because of api changes. I will file additional bugs later.
Attachment #8956238 - Attachment is obsolete: true
Attachment #8967948 - Flags: review?(iann_bugzilla)
rebased patch
Attachment #8955775 - Attachment is obsolete: true
Attachment #8967949 - Flags: review?(iann_bugzilla)
Attachment #8956240 - Attachment is obsolete: true
2.53 only. r+ from IanN retained in case we do an interim release.

Unused /suite/themes/classic/communicator/bookmarks/bookmarksWindow.css removed.
Attachment #8948124 - Attachment is obsolete: true
Attachment #8967950 - Flags: review+
2.53 only. Minor fix for the async path and rebased.
Attachment #8951977 - Flags: review?(iann_bugzilla)
Attachment #8951978 - Flags: review?(iann_bugzilla)
Attachment #8951985 - Flags: review?(iann_bugzilla)
Comment on attachment 8967948 [details] [diff] [review]
1378089-part3-bookmarks-257.patch

>+++ b/suite/browser/browser-places.js


>+      } catch (e) {
>+        Cu..reportError(e);
Double dot?

>   bookmarkCurrentPage: function PCH_bookmarkCurrentPage(aShowEditUI, aParent) {
>-    this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI);
>+    this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI)
>+        .catch(Cu..reportError);
Double dot?

>     PlacesUtils.bookmarks.fetch({url: this._uri}, b => aItemGuids.push(b.guid))
>-      .catch(Cu.reportError)
>+      .catch(Cu..reportError)
Double dot?

>-             Cu.reportError("BookmarkingUI failed adding a bookmarks observer: " + ex);
>+             Cu..reportError("BookmarkingUI failed adding a bookmarks observer: " + ex);
Double dot?

This is a difficult patch to review, due to the number of indirect changes needed for syncing e.g. Cc, Ci, Cu but from what I can tell LGTM r=me
Attachment #8967948 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8951977 [details] [diff] [review]
1378089-part5-bookmarks-257.patch

LGTM r=me
Attachment #8951977 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8951978 [details] [diff] [review]
1378089-part6-bookmarks-257.patch

LGTM r=me
Attachment #8951978 - Flags: review?(iann_bugzilla) → review+
Attachment #8951985 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8967949 [details] [diff] [review]
1378089-part4-bookmarks-257.patch

Looks like this patch does fix the previous Cu.. issues as a bonus.
LGTM r=me
Attachment #8967949 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8951973 [details] [diff] [review]
1378089-part1-recentwindow-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951973 - Flags: approval-comm-beta+
Comment on attachment 8951977 [details] [diff] [review]
1378089-part5-bookmarks-257.patch

[Triage Comment]

a=me for 2.57/c-b
Attachment #8951977 - Flags: approval-comm-beta+
Comment on attachment 8951978 [details] [diff] [review]
1378089-part6-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951978 - Flags: approval-comm-beta+
Comment on attachment 8951985 [details] [diff] [review]
1378089-part7-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8951985 - Flags: approval-comm-beta+
Comment on attachment 8967947 [details] [diff] [review]
1378089-part2-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967947 - Flags: approval-comm-beta+
Comment on attachment 8967948 [details] [diff] [review]
1378089-part3-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967948 - Flags: approval-comm-beta+
Comment on attachment 8967949 [details] [diff] [review]
1378089-part4-bookmarks-257.patch

[Triage Comment]
a=me for 2.57/c-b
Attachment #8967949 - Flags: approval-comm-beta+
Comment on attachment 8967951 [details] [diff] [review]
1378089-part3-bookmarks-253.patch

r=me just in case...
Attachment #8967951 - Flags: review+
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/b9a5db2726f3
Part 1. Add recentwindow.jsm to SeaMonkey. r=IanN
https://hg.mozilla.org/comm-central/rev/0c8c03f917f1
Part 2. Mass rename bookmarks files and dirs to corresponding Firefox ones. r=IanN
https://hg.mozilla.org/comm-central/rev/f000c3a844b1
Part 3. Replace the Bookmarks Manager and the History viewer with the Firefox library. r=IanN
https://hg.mozilla.org/comm-central/rev/fd9d6ed9952e
Part 4. Replace the Bookmarks Manager and the History viewer with the Firefox library. r=IanN
https://hg.mozilla.org/comm-central/rev/838853b3bbab
Part 5. Make the All Bookmarks folder for the left pane of the Library a virtual query. r=IanN
https://hg.mozilla.org/comm-central/rev/016955819613
Part 6. Make the new All Bookmarks folder query only update on the mobile folder status change for better performance. r=IanN
https://hg.mozilla.org/comm-central/rev/865b73490b82
Part 7. Move osx filters.svg from bookmarks to places. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Not really fixed because of later changes in 60 and 61. Stay tuned for the followup bugs.
Target Milestone: --- → Seamonkey2.58
Blocks: 1456589
Just a small patch for 2.53 already in the 2.57 patch. 

Fixes:
Timestamp: 4/28/2018, 11:23:15 AM
Error: TypeError: this._recentFolders is undefined
Source File: chrome://communicator/content/places/editBookmarkOverlay.js
Line: 1127
Depends on: 1474125
Blocks: 1476660
Blocks: 1476662
Blocks: 1489307
Blocks: 1524791
Depends on: 1597066
Regressions: 1597066
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/dc4ab600181b
Follow-Up. Fix bad search placeholder names in sidebar panel. r=me
You need to log in before you can comment on or make changes to this bug.