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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0f99ec6
Titles of horizontal separators are better visible consecutive lines.
macrogreg Jan 26, 2025
67d30bb
Create vertical separators on the Bookmarks Bar
macrogreg Jan 27, 2025
6f95334
Tests for vertical separators on the Bookmarks Bar
macrogreg Jan 27, 2025
785f2a7
Translate _locales/en/messages.json in zh_CN
transifex-integration[bot] Jan 27, 2025
a199cb7
Fix typos
macrogreg Jan 27, 2025
1341d31
For local development for Firefox.
peter-lyons-kehl Jan 27, 2025
9e3eacd
Merge pull request #1848 from peter-lyons-kehl/manifest-firefox-override
marcelklehr Jan 27, 2025
b9a2add
Fix indentation
macrogreg Jan 28, 2025
d81610c
Fix trailing spaces
macrogreg Jan 28, 2025
b64822a
Fix link issues in files unrelated to overall change
macrogreg Jan 28, 2025
fc13881
Add Win-specific npm build scripts
macrogreg Jan 28, 2025
460c9cc
Merge branch 'bookmarks-bar-separators' of github.com:macrogreg/flocc…
marcelklehr Jan 30, 2025
3dce2a9
fix(BrowserTree#getBookmarksTree): Pass parent node to next recursion
marcelklehr Jan 30, 2025
6d20625
fix(separator tests): Set correct separator ids and create artificial…
marcelklehr Jan 30, 2025
bab3214
Make sure that only the actual toolbar gets separator special casing
macrogreg Jan 31, 2025
ce0da59
Add a unit test to validate that separators are converted correctly
macrogreg Jan 31, 2025
f8fe422
Delete files that cause sync artifacts
macrogreg Jan 31, 2025
b01c149
Reset files that caused sync artifacts
macrogreg Jan 31, 2025
88389ef
Include separator improvements in changelog.
macrogreg Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@
"description": "Sync your bookmarks privately across browsers and devices",
"scripts": {
"build": "NODE_OPTIONS=--max-old-space-size=5000 gulp",
"build-win": "SET NODE_OPTIONS=--max-old-space-size=5000 & gulp",
"build-release": "NODE_OPTIONS=--max-old-space-size=5000 gulp release",
"build-release-win": "SET NODE_OPTIONS=--max-old-space-size=5000 & gulp release",
"watch": "NODE_OPTIONS=--max-old-space-size=5000 gulp watch",
"watch-win": "SET NODE_OPTIONS=--max-old-space-size=5000 & gulp watch",
"test": "node --unhandled-rejections=strict test/selenium-runner.js",
"lint": "eslint --ext .js,.vue src",
"lint:fix": "eslint --ext .js,.vue src --fix"
Expand Down
19 changes: 13 additions & 6 deletions src/lib/browser/BrowserTree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export default class BrowserTree implements IResource<typeof ItemLocation.LOCAL>
const allAccounts = await (await Account.getAccountClass()).getAllAccounts()

const recurse = (node, parentId?, rng?) => {
const TITLE_BOOKMARKS_BAR = 'Bookmarks Bar',
TITLE_OTHER_BOOKMARKS = 'Other Bookmarks',
TITLE_BOOKMARKS_MENU = 'Bookmarks Menu',
TITLE_MOBILE_BOOKMARKS = 'Mobile Bookmarks'
if (
allAccounts.some(
acc => acc.getData().localRoot === node.id && String(node.id) !== String(this.rootId) && !acc.getData().nestedSync
Expand All @@ -54,17 +58,17 @@ export default class BrowserTree implements IResource<typeof ItemLocation.LOCAL>
switch (node.id) {
case '1': // Chrome
case 'toolbar_____': // Firefox
overrideTitle = 'Bookmarks Bar'
overrideTitle = TITLE_BOOKMARKS_BAR
break
case '2': // Chrome
case 'unfiled_____': // Firefox
overrideTitle = 'Other Bookmarks'
overrideTitle = TITLE_OTHER_BOOKMARKS
break
case 'menu________': // Firefox
overrideTitle = 'Bookmarks Menu'
overrideTitle = TITLE_BOOKMARKS_MENU
break
case 'mobile______': // Firefox
overrideTitle = 'Mobile Bookmarks'
overrideTitle = TITLE_MOBILE_BOOKMARKS
}
if (overrideTitle) {
Logger.log(
Expand Down Expand Up @@ -99,14 +103,17 @@ export default class BrowserTree implements IResource<typeof ItemLocation.LOCAL>
return folder
} else if (self.location.protocol === 'moz-extension:' && node.type === 'separator') {
// Translate mozilla separators to floccus separators
const mockSeparator = (overrideTitle === TITLE_BOOKMARKS_BAR)
? {title: '', page: 'vertical.html'}
: {title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', page: 'index.html'}
return new Tree.Bookmark({
location: ItemLocation.LOCAL,
id: node.id,
parentId,
title: '-----',
title: mockSeparator.title,
// If you have more than a quarter million separators in one folder, call me
// Floccus breaks down much earlier atm
url: `https://separator.floccus.org/?id=${rng.int(0,1000000)}`,
url: `https://separator.floccus.org/${mockSeparator.page}?id=${rng.int(0,1000000)}`,
})
} else {
return new Tree.Bookmark({
Expand Down
57 changes: 43 additions & 14 deletions src/test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2337,6 +2337,15 @@ describe('Floccus', function() {
type: 'separator',
parentId: fooFolder.id
})
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.

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

await account.sync() // propagate to server
expect(account.getData().error).to.not.be.ok
Expand All @@ -2352,11 +2361,16 @@ describe('Floccus', function() {
new Folder({title: 'foo',
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=467366'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=731368'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=731368'})
]}),
]}),
new Folder({title: 'Bookmarks Bar',
children: [
new Bookmark({title: 'url3',url: 'http://ur3.l'}),
new Bookmark({title: '', url: 'https://separator.floccus.org/vertical.html?id=000000'})
marcelklehr marked this conversation as resolved.
Show resolved Hide resolved
]})
]}),
false
)
Expand All @@ -2369,11 +2383,16 @@ describe('Floccus', function() {
new Folder({title: 'foo',
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=467366'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=731368'})
]}),
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=731368'})
]})
]}),
new Folder({title: 'Bookmarks Bar',
children: [
new Bookmark({title: 'url3',url: 'http://ur3.l'}),
new Bookmark({title: '', url: 'https://separator.floccus.org/vertical.html?id=000000'})
marcelklehr marked this conversation as resolved.
Show resolved Hide resolved
]})
]}),
false
)
Expand Down Expand Up @@ -2401,10 +2420,15 @@ describe('Floccus', function() {
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=467366'})
]}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=379999'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/index.html?id=379999'})
]}),
new Folder({title: 'Bookmarks Bar',
children: [
new Bookmark({title: 'url3',url: 'http://ur3.l'}),
new Bookmark({title: '', url: 'https://separator.floccus.org/vertical.html?id=000000'})
marcelklehr marked this conversation as resolved.
Show resolved Hide resolved
]})
]}),
false
)
Expand All @@ -2419,10 +2443,15 @@ describe('Floccus', function() {
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=731368'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=731368'})
]}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=467366'})
]}),
new Folder({title: 'Bookmarks Bar',
children: [
new Bookmark({title: 'url3',url: 'http://ur3.l'}),
new Bookmark({title: '', url: 'https://separator.floccus.org/vertical.html?id=000000'})
marcelklehr marked this conversation as resolved.
Show resolved Hide resolved
]})
]}),
false
)
Expand Down Expand Up @@ -2482,9 +2511,9 @@ describe('Floccus', function() {
new Folder({title: 'foo',
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=731368'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=731368'})
]}),
]}),
]}),
Expand All @@ -2499,9 +2528,9 @@ describe('Floccus', function() {
new Folder({title: 'foo',
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=467366'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=731368'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=731368'})
]}),
]}),
]}),
Expand All @@ -2528,7 +2557,7 @@ describe('Floccus', function() {
children: [
new Bookmark({title: 'url', url: 'http://ur.l/'}),
new Bookmark({title: 'url2',url: 'http://ur2.l'}),
new Bookmark({title: '-----', url: 'https://separator.floccus.org/?id=467366'})
new Bookmark({title: '⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯', url: 'https://separator.floccus.org/?id=467366'})
]}),
]}),

Expand Down
8 changes: 4 additions & 4 deletions src/ui/components/AccountCard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@
</template>
<template v-if="status === 'scheduled'">
<v-btn
:color="statusType"
class="float-right"
x-small
@click="onForceSync">
:color="statusType"
class="float-right"
x-small
@click="onForceSync">
{{ t('LabelScheduledforcesync') }}
</v-btn>
</template>
Expand Down
4 changes: 2 additions & 2 deletions src/ui/components/OptionsGoogleDrive.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@
</v-card-title>
<v-card-text>
<OptionAutoSync
:value="enabled"
@input="$emit('update:enabled', $event)" />
:value="enabled"
@input="$emit('update:enabled', $event)" />
<OptionSyncInterval
:value="syncInterval"
@input="$emit('update:syncInterval', $event)" />
Expand Down
Loading