Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separators on Bookmarks-bar are Vertical rather than Horizontal #1846

Merged
merged 19 commits into from
Feb 1, 2025

Conversation

macrogreg
Copy link
Contributor

@macrogreg macrogreg commented Jan 27, 2025

This PR addresses #1841:

Firefox supports bookmark separator lines as a first-class concept, but other supported browsers do not. Fluccus mitigates that by creating dummy bookmarks looking like horizontal lines. This change ensures that when such dummy bookmarks are created in the Bookmark Toolbar, vertical rather than horizontal lines are used.

Validation:

  • Added a unit test that exercises the new logic (should convert vertical and horizontal separators).
  • Manual validation by creating bookmarks and separators in different folders in Firefox, observing them being correctly synced to Edge, then being modified in Edge, and the changed being correctly synched back to Firefox.

Other changes:

  • Add npm scripts to build under Windows.
  • Improve view of horizontal dummy separators by making them consecutive, slightly longer lines.
  • Fix a couple lint failures in random/unrelated files.

Note:

There appears to be a sync/diff issues with _locales/zh_CN/messages.json and manifest-firefox-override.sh. The files are identical to the develop branch, but a modification is being picked up. I think this was caused during cross-repo merges by multiple people somehow. A merge has not resolved that. To work around, let's merge this PR and subsequently manually validate that those files have the expected contents. If there are any issue, we will fix those contents in a separate PR.

@macrogreg macrogreg marked this pull request as draft January 27, 2025 00:47
src/test/test.js Outdated Show resolved Hide resolved
src/test/test.js Outdated Show resolved Hide resolved
src/test/test.js Outdated Show resolved Hide resolved
transifex-integration bot and others added 3 commits January 27, 2025 02:34
100% translated source file: '_locales/en/messages.json'
on 'zh_CN'.
@marcelklehr marcelklehr marked this pull request as ready for review January 27, 2025 07:22
src/test/test.js Outdated Show resolved Hide resolved
src/test/test.js Outdated Show resolved Hide resolved
src/test/test.js Outdated Show resolved Hide resolved
src/test/test.js Outdated Show resolved Hide resolved
@marcelklehr
Copy link
Member

Tests ran through:

Details
### should sync separators ###

initializing account 17381370413620.5719715913101022
Apparently not initialized, because: [Error: Incorrect argument types for bookmarks.getSubTree.]
Trees are not equal: (checkOrder: true, ignoreEmptyFolders: false) Tree 1:
+ #0chn5bGeJkTw[undefined] parentId: undefined, hash: undefined
  + #WFsCjkRqYQdy[bar] parentId: 0chn5bGeJkTw, hash: undefined
    + #1W-BaP8XYHw-[foo] parentId: WFsCjkRqYQdy, hash: undefined
      - #hAjoi-Xclgt9[url](http://ur.l/) parentId: 1W-BaP8XYHw-
      - #UUsvy_Kn_zqp[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: 1W-BaP8XYHw-
      - #JQLwfJZwe6Vk[url2](http://ur2.l/) parentId: 1W-BaP8XYHw-
      - #0NJ11s664XDt[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=731368) parentId: 1W-BaP8XYHw-
 Tree 2:
+ #undefined[undefined] parentId: undefined, hash: undefined
  + #undefined[bar] parentId: undefined, hash: undefined
    + #undefined[foo] parentId: undefined, hash: undefined
      - #undefined[url](http://ur.l/) parentId: undefined
      - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: undefined
      - #undefined[url2](http://ur2.l/) parentId: undefined
      - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=731368) parentId: undefined
  + #undefined[Bookmarks Bar] parentId: undefined, hash: undefined
    - #undefined[url3](http://ur3.l/) parentId: undefined
    - #undefined[](https://separator.floccus.org/vertical.html?id=000000) parentId: undefined
-> FAILED : should sync separators expected [ …(1) ] to have a length of 2 but got 1
n@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:1057
13896/e.exports/i.prototype.assert@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:8070
O@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:16780
m@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:2705
15553/e.exports/</</<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:5326
33863/e.exports/s.method@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:50471
n@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:45219
v@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4432
82585/</</</</<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:41054


### should sync separators 2 ###

initializing account 17381370496240.39390490132414424
Apparently not initialized, because: [Error: Incorrect argument types for bookmarks.getSubTree.]
Trees are not equal: (checkOrder: true, ignoreEmptyFolders: false) Tree 1:
- #pab2doPUAKM0[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: 13xLcw6pZoLy
 Tree 2:
- #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=467366) parentId: undefined
Trees are not equal: (checkOrder: true, ignoreEmptyFolders: false) Tree 1:
+ #13xLcw6pZoLy[foo] parentId: BOgSDQcxPwlt, hash: undefined
  - #-WUZ1wbuqylv[url](http://ur.l/) parentId: 13xLcw6pZoLy
  - #pab2doPUAKM0[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: 13xLcw6pZoLy
  - #-eM3Fc09JL8J[url2](http://ur2.l/) parentId: 13xLcw6pZoLy
  - #8TEzyXGaS7lC[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=731368) parentId: 13xLcw6pZoLy
 Tree 2:
+ #undefined[foo] parentId: undefined, hash: undefined
  - #undefined[url](http://ur.l/) parentId: undefined
  - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=467366) parentId: undefined
  - #undefined[url2](http://ur2.l/) parentId: undefined
  - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=731368) parentId: undefined
Trees are not equal: (checkOrder: true, ignoreEmptyFolders: false) Tree 1:
+ #BOgSDQcxPwlt[bar] parentId: EZ6yauu4c7hv, hash: undefined
  + #13xLcw6pZoLy[foo] parentId: BOgSDQcxPwlt, hash: undefined
    - #-WUZ1wbuqylv[url](http://ur.l/) parentId: 13xLcw6pZoLy
    - #pab2doPUAKM0[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: 13xLcw6pZoLy
    - #-eM3Fc09JL8J[url2](http://ur2.l/) parentId: 13xLcw6pZoLy
    - #8TEzyXGaS7lC[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=731368) parentId: 13xLcw6pZoLy
 Tree 2:
+ #undefined[bar] parentId: undefined, hash: undefined
  + #undefined[foo] parentId: undefined, hash: undefined
    - #undefined[url](http://ur.l/) parentId: undefined
    - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=467366) parentId: undefined
    - #undefined[url2](http://ur2.l/) parentId: undefined
    - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=731368) parentId: undefined
Trees are not equal: (checkOrder: true, ignoreEmptyFolders: false) Tree 1:
+ #EZ6yauu4c7hv[undefined] parentId: undefined, hash: undefined
  + #BOgSDQcxPwlt[bar] parentId: EZ6yauu4c7hv, hash: undefined
    + #13xLcw6pZoLy[foo] parentId: BOgSDQcxPwlt, hash: undefined
      - #-WUZ1wbuqylv[url](http://ur.l/) parentId: 13xLcw6pZoLy
      - #pab2doPUAKM0[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=467366) parentId: 13xLcw6pZoLy
      - #-eM3Fc09JL8J[url2](http://ur2.l/) parentId: 13xLcw6pZoLy
      - #8TEzyXGaS7lC[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/index.html?id=731368) parentId: 13xLcw6pZoLy
 Tree 2:
+ #undefined[undefined] parentId: undefined, hash: undefined
  + #undefined[bar] parentId: undefined, hash: undefined
    + #undefined[foo] parentId: undefined, hash: undefined
      - #undefined[url](http://ur.l/) parentId: undefined
      - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=467366) parentId: undefined
      - #undefined[url2](http://ur2.l/) parentId: undefined
      - #undefined[⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯](https://separator.floccus.org/?id=731368) parentId: undefined
-> FAILED : should sync separators 2 expected '[https://separator.floccus.org/index.h…](https://separator.floccus.org/index.h%E2%80%A6)' to equal '[https://separator.floccus.org/?id=467…](https://separator.floccus.org/?id=467%E2%80%A6)'
n@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:1057
13896/e.exports/i.prototype.assert@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:8070
f@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:10527
c@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:46182
m@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:2705
15553/e.exports/</</<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:5141
l@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/421.js:2:50867
v@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4167
82585/v/<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4469
v@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4451
82585/v/<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4469
v@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4451
82585/v/<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4469
v@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:4451
82585/</</</</<@moz-extension://f9f314b8-71b6-4a97-8c43-5f8aa5817488/dist/js/936.js:1:44459

src/test/test.js Outdated
await browser.bookmarks.create({
title: 'url3',
url: 'http://ur3.l',
parentId: 'toolbar_____'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work, sadly :)

You'll need to create stuff inside the localRoot Folder

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or change the localRoot field of the account being synced as is done in a few tests that test syncing the browser root folder, but just creating a folder called Bookmarks bar inside the localRoot should work, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I am not sure. In the prod code, we use that magic node id to test whether this is the bookmark bar:

image

And then, we use the result of that check to control behavior:

image

So for the test, we need to control the nodes in the folder with the magic node id - that's the "Bookmarks Toolbar" folder. This all only matters on Firefox, because it is the only browser that supports separators. And on Firefox, the Bookmarks Bar should always be there.

Here is some info:
https://bugzilla.mozilla.org/show_bug.cgi?id=1071505

So, I think that the initial approach

await browser.bookmarks.create({
    type: 'separator',
    parentId: bookmarksBar.id
})

should "just work".

I'll play around with the tests a little more and will see what comes out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Based on the latest test failure, it looks like your approach to do

const bookmarksBar = await browser.bookmarks.create({
    title: 'Bookmarks Bar',
    parentId: localRoot
})

does, actually work, and the only reason for the failure is the ordering of the folder branches.

However, based on the above reasoning, I am surprised. It should not actually work.
I think the explanation is this: You set the title to be 'Bookmarks Bar' and the prod code applies the special logic to any folder named like that. Even if it is not the actual bookmark bar. In fact, the name for that folder in the Englisch Firefox is "Bookmarks Toolbar".

To address this I will try changing the prod code to only perform the special casing if the actual toolbar is processed. I will also see if I can fix the test. I will let you know how far I get.

@macrogreg macrogreg changed the title [Draft] Separators on Bookmarks-bar are Vertical rather than Horizontal Separators on Bookmarks-bar are Vertical rather than Horizontal Jan 31, 2025
@marcelklehr
Copy link
Member

To address this I will try changing the prod code to only perform the special casing if the actual toolbar is processed.

I'm not sure if this is worth the effort as it also makes testing harder

if (node.parentId === this.absoluteRoot.id && !isVivaldiBrowser) {
switch (node.id) {
case '1': // Chrome
case 'toolbar_____': // Firefox
overrideTitle = TITLE_BOOKMARKS_BAR
isToolbar = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Very elegant!

@macrogreg
Copy link
Contributor Author

To address this I will try changing the prod code to only perform the special casing if the actual toolbar is processed.

I'm not sure if this is worth the effort as it also makes testing harder

I added a unit test for it.

@macrogreg
Copy link
Contributor Author

@marcelklehr
So, I think this is it. I added a unit test and the relevant tests pass for me locally.
I also manually tested that Firefox and Edge synched the separators as expected.

Could you please run the CI and take a quick look at the code.
Tomorrow I want to update the PR description and address any feedback you might have.
Then it should be ready to merge.

@macrogreg
Copy link
Contributor Author

macrogreg commented Jan 31, 2025

It looks like the remaining test failures are not related to the changes being made.
@marcelklehr, should they be ignored? If not, can you please advise?

I have updated the PR description. Unless you have any change requests, I'd like to merge this PR. Please let me know if we can go ahead and approve the PR.

Thank you!

Test issue details:

  1. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477234192?pr=1846
    E017: Network error: Check your network connection and your profile details

  2. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477238121?pr=1846
    E031: Could not authenticate with Google Drive.

  3. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477238401?pr=1846
    E031: Could not authenticate with Google Drive.

  4. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477238695?pr=1846
    E031: Could not authenticate with Google Drive.

  5. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477239012?pr=1846
    E031: Could not authenticate with Google Drive.

  6. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477239329?pr=1846
    E019: HTTP status 401. Failed GET request.

  7. https://github.com/floccusaddon/floccus/actions/runs/13071284285/job/36477239583?pr=1846
    E019: HTTP status 401. Failed GET request.

  8. https://github.com/floccusaddon/floccus/actions/runs/13071284284/job/36477232755?pr=1846
    iOS build: SecKeychainItemImport: One or more parameters passed to a function were not valid.

  9. Summary of earlier failures

  10. Summary of earlier failures

@marcelklehr
Copy link
Member

Yep, one chrome build failure on nextcloud bookmarks is expected, as well as the failing ios build. The Google Drive and linkwarden tests are failing because your branch is on a fork, so that's ok, too.

@marcelklehr marcelklehr merged commit 6683981 into floccusaddon:develop Feb 1, 2025
24 of 34 checks passed
@marcelklehr
Copy link
Member

Awesome stuff @macrogreg! Thank you for your contribution and the nice cooperation! 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants