-
-
Notifications
You must be signed in to change notification settings - Fork 253
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
Separators on Bookmarks-bar are Vertical rather than Horizontal #1846
Conversation
100% translated source file: '_locales/en/messages.json' on 'zh_CN'.
…irefox-override script to override manifest.json for Firefox and prevent accidental GIT commits
Tests ran through: Details
|
src/test/test.js
Outdated
await browser.bookmarks.create({ | ||
title: 'url3', | ||
url: 'http://ur3.l', | ||
parentId: 'toolbar_____' |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
And then, we use the result of that check to control behavior:
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.
There was a problem hiding this comment.
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.
…usaddon-floccus into macrogreg-bookmarks-bar-separators
Signed-off-by: Marcel Klehr <[email protected]>
… bookmarks bar Signed-off-by: Marcel Klehr <[email protected]>
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! Very elegant!
I added a unit test for it. |
@marcelklehr Could you please run the CI and take a quick look at the code. |
It looks like the remaining test failures are not related to the changes being made. 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:
|
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. |
Awesome stuff @macrogreg! Thank you for your contribution and the nice cooperation! 💙 |
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:
should convert vertical and horizontal separators
).Other changes:
npm
scripts to build under Windows.Note:
There appears to be a sync/diff issues with
_locales/zh_CN/messages.json
andmanifest-firefox-override.sh
. The files are identical to thedevelop
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.