search

brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.84k stars 2.33k forks source link

Other bookmarks is missing, breaking bookmarks API #7639

Closed Johann999 closed 4 years ago

Johann999 commented 4 years ago

Dear Brave,

isn't it frustrating if after the update to version 1.2.41 instead of "Other bookmarks"only ">>" sees?!

The bookmark changes are unnecessary, bring no benefits and only annoy the users. The structure and layout of the bookmarks has proven itself for many years. There was no reason to change this. Brave is now no longer compatible with other Chromium based browser.

Please undo changes to bookmarks.

Thank you

bsclifton commented 4 years ago

This was intentionally removed when solving https://github.com/brave/brave-browser/issues/5158

Removing this was needed to prevent problems with syncing Desktop w/ iOS/Android

Closing as wontfix for now

cc: @rebron @darkdh @jhreis @kjozwiak for additional comments

Hardtarget24 commented 4 years ago

Unfortunately with this new patch the Other Bookmarks folder is no longer anchored to it's own area on the right hand side of the bookmarks bar. This is an issue for me personally and not expected behaviour from a chromium based browser. (and honestly although seemingly minor this change, if intended, will most likely force me to stop using Brave and it's really disruptive to my workflow)

I'm hoping this can be fixed somehow.

bsclifton commented 4 years ago

per @Rob4226 via https://github.com/brave/brave-browser/issues/5158#issuecomment-571471219

This update on Windows 10 just broke a bookmark extension xBrowserSync and I had no idea why all day until I finally looked here. Is there any way to change it back (with syncing off), unnesting the Other Bookmarks folder? Thank you.

bsclifton commented 4 years ago

Going to re-open this; @rebron and @darkdh are looking into this, after comment by @nero120 via https://github.com/brave/brave-browser/issues/5158#issuecomment-571818432

As this change potentially breaks compatibility with any extension that uses the bookmarks api, what's the plan for dealing with that? Will you be launching your own extensions store shortly since extension authors that use the bookmarks api will now need to provide a separate build for brave...?

I suppose you'll need to update this page from:

Brave offers support for nearly all extensions that are compatible with chromium.

to something like

Brave offers support for some extensions that are compatible with chromium (up to you to work out which though)

Any guidance would be appreciated!

bridiver commented 4 years ago

I wasn't personally a big fan of the change to remove "Other Bookmarks", but (unfortunately ;) I don't always get my way. In any case I can tell you with a high degree of certainty that this change is here to stay. We have had extensive (and heated) discussions about this and it's not likely to change. What could change here is how things are presented in the bookmarks api so that Brave looks like it has the same bookmark structure as Chrome. Basically a kind of "shadow" node that is actually a reference to the bookmarks bar, but appears to be "other bookmarks" in the api. However, breakage in the known extensions could easily be fixed without any changes to Brave and at least one of them relies on undocumented node ids which are not part of the API and hence not even guaranteed to be compatible across chromium versions

antonycourtney commented 4 years ago

( Moving discussion here from #5158 )

@nero120 I was surprised they didn't have constants for the bookmarks bar and other bookmarks node. I expected to find them in the api docs and use it as a potential fix inside the api

Agree this is disappointing.

For what it's worth, Tabli doesn't depend on a fixed node ID. It does depend on tree.children[1] being the "Other Bookmarks" folder, where tree is the root returned from chrome.bookmarks.getTree. Not great, although I also think that looking for "Other Bookmarks" by name off the root also would have broken with this change.

The only mention I've found about the contents of the root folder in the API docs is this (on the chrome.bookmarks API page):

image

After this change in Brave, are there any guarantees about what will be in the root folder in Brave? Is "Other Bookmarks" replaced by a folder named "Bookmarks", a folder that exists at the same level as "Bookmarks", a sub-folder of "Bookmarks", or something else? It's really not clear from all the screenshots above.

@bridiver suggested either leaving the parent id blank or using children.length - 1 as the index when creating the folder used by my extension.

I'd like to do the right thing for Brave users. My hunch is that my extension creating a "Tabli Saved Windows" folder on the Bookmarks Bar in Brave isn't going to be an ideal user experience. "Other Bookmarks" is really a pretty reasonable place for this, but it's not entirely obvious to me if there is an approved replacement for that in Brave.

nero120 commented 4 years ago

For what it's worth, Tabli doesn't depend on a fixed node ID. It does depend on tree.children[1] being the "Other Bookmarks" folder, where tree is the root returned from chrome.bookmarks.getTree. Not great, although I also think that looking for "Other Bookmarks" by name off the root also would have broken with this change.

FYI, you can't really query by name (title) since they are different for each language!

antonycourtney commented 4 years ago

I wasn't personally a big fan of the change to remove "Other Bookmarks", but (unfortunately ;) I don't always get my way. In any case I can tell you with a high degree of certainty that this change is here to stay. We have had extensive (and heated) discussions about this and it's not likely to change. What could change here is how things are presented in the bookmarks api so that Brave looks like it has the same bookmark structure as Chrome. Basically a kind of "shadow" node that is actually a reference to the bookmarks bar, but appears to be "other bookmarks" in the api. However, breakage in the known extensions could easily be fixed without any changes to Brave and at least one of them relies on undocumented node ids which are not part of the API and hence not even guaranteed to be compatible across chromium versions

If Brave provided a root node with a replacement folder for "Other Bookmarks" at position 1 in the children list, I'm pretty sure this would instantly fix the Tabli extension.

edsimpsons83 commented 4 years ago

This is really disappointing and very user hostile. Moved my entire family over to Brave during the holidays for improved security and woke up to txts this morning asking why their bookmarks have moved around. With Chrome controlling ~70% of the browser market, the vast majority of people switching to Brave expect the "Other Bookmarks" folder to be on the right, not mixed in with their normal bookmarks.

georapbox commented 4 years ago

This was intentionally removed when solving #5158

Removing this was needed to prevent problems with syncing Desktop w/ iOS/Android

Closing as wontfix for now

cc: @rebron @darkdh @jhreis @kjozwiak for additional comments

Even after solving #5158 the sync issues still remain though. Trying to sync my Mac with android ended up with duplicate folders or missing bookmarks.

bridiver commented 4 years ago

You can always create a folder inside the bookmarks bar, but I seriously doubt we'll change the node structure because we wanted to standardize it across platforms (unlike Chrome which has a separate "mobile bookmarks" folder)

On Jan 8, 2020, at 3:45 PM, Antony Courtney notifications@github.com wrote:

 ( Moving discussion here from #5158 )

@nero120 I was surprised they didn't have constants for the bookmarks bar and other bookmarks node. I expected to find them in the api docs and use it as a potential fix inside the api

Agree this is disappointing.

For what it's worth, Tabli doesn't depend on a fixed node ID. It does depend on tree.children[1] being the "Other Bookmarks" folder, where tree is the root returned from chrome.bookmarks.getTree. Not great, although I also think that looking for "Other Bookmarks" by name off the root also would have broken with this change.

The only mention I've found about the contents of the root folder in the API docs is this (on the chrome.bookmarks API page):

After this change in Brave, are there any guarantees about what will be in the root folder in Brave? Is "Other Bookmarks" replaced by a folder named "Bookmarks", a folder that exists at the same level as "Bookmarks", a sub-folder of "Bookmarks", or something else? It's really not clear from all the screenshots above.

@bridiver suggested either leaving the parent id blank or using children.length - 1 as the index when creating the folder used by my extension.

I'd like to do the right thing for Brave users. My hunch is that my extension creating a "Tabli Saved Windows" folder on the Bookmarks Bar in Brave isn't going to be an ideal user experience. "Other Bookmarks" is really a pretty reasonable place for this, but it's not entirely obvious to me if there is an approved replacement for that in Brave.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

bridiver commented 4 years ago

There won't be a replacement per se, but we may decide to fake it to avoid other potential breakage. In that case I think it's likely that "Other Bookmarks" would just be an alias for the bookmarks bar

On Jan 8, 2020, at 4:01 PM, Antony Courtney notifications@github.com wrote:

 I wasn't personally a big fan of the change to remove "Other Bookmarks", but (unfortunately ;) I don't always get my way. In any case I can tell you with a high degree of certainty that this change is here to stay. We have had extensive (and heated) discussions about this and it's not likely to change. What could change here is how things are presented in the bookmarks api so that Brave looks like it has the same bookmark structure as Chrome. Basically a kind of "shadow" node that is actually a reference to the bookmarks bar, but appears to be "other bookmarks" in the api. However, breakage in the known extensions could easily be fixed without any changes to Brave and at least one of them relies on undocumented node ids which are not part of the API and hence not even guaranteed to be compatible across chromium versions

If Brave provided a root node with a replacement folder for "Other Bookmarks" at position 1 in the children list, I'm pretty sure this would instantly fix the Tabli extension.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

bridiver commented 4 years ago

This was intentionally removed when solving #5158 Removing this was needed to prevent problems with syncing Desktop w/ iOS/Android Closing as wontfix for now cc: @rebron @darkdh @jhreis @kjozwiak for additional comments

Even after solving #5158 the sync issues still remain though. Trying to sync my Mac with android ended up with duplicate folders or missing bookmarks.

This is something we have discussed and it seems silly to me that we don't match up bookmarks by name/url/folder. I can't see any reason why anyone would intentionally create a duplicate of a bookmark in the same folder cc @davidtemkin, however that is a separate issue that should have its own ticket. Also there are other outstanding issues with the sync implementation that are being fixed for missing bookmarks, etc...

BrendanEich commented 4 years ago

@antonycourtney @edsimpsons83 This was a mistake, we will fix as best we can. Sorry about it and please stay tuned.

nsy16 commented 4 years ago

It would be nice to get my xBrowserSync extension working again. Only started using Brave since November last year after considering many alternative browsers.

z7r1k3 commented 4 years ago

Yes, due to lack of 1st-party implementations for every platform (talking about things like Arch Linux here) I have a need for other browsers sometimes, so xBrowserSync functionality is a must. It sounds like based off of what @BrendanEich is saying that you guys are planning on undoing the removal of the dedicated "Other Bookmarks" folder, but if you go the route of making "Other Bookmarks" an alias, please do not make it for the Bookmarks Bar, but instead a folder called "Other Bookmarks" that can be put wherever on the Bookmarks Bar. Otherwise we'll have a mess on our hands with any extensions that sync Bookmarks.

mcmxcdev commented 4 years ago

For me it happened that after brave auto-updated, all my 2000+ bookmarks disappeared, because https://www.everhelper.me/synchronizer.php couldn't deal with the changes.

@nsy16 and others, I recommend to install Brave version 1.1.23 temporarily to have the old bookmarks structure back and sync it, works for me.

bridiver commented 4 years ago

fyi - we're having a discussion now based on all the feedback and known incompatibilities with extensions and all options are on the table including restoring the other bookmarks node

nero120 commented 4 years ago

fyi - we're having a discussion now based on all the feedback and known incompatibilities with extensions and all options are on the table including restoring the other bookmarks node

If I may...

bridiver commented 4 years ago

Rolling the changes back may be what we end up doing, but it's not a simple thing to do at this point. It affects sync among other things so it's not something we can just quickly rollback and release

lbdl commented 4 years ago

this is somewhat of a deal breaker for many. It is more or less impossible to work productively without the ability to sync bookmarks across devices. I switched happily to Brave over a year ago despite the never ending frustration with no sync, and no comments of the multiple issues surrounding Braves own sync (which is still unusable). XBrowserSync fixed this for me, hooray and now the OtherBookmarks killing kills XbrowserSync and I'm forced to implement ugly hacks around exports/diffs/etc. Might I suggest the attitude that sync is ESSENTIAL for uptake of the platform NOT optional and as such changes that disable the ability to use third party sync tools (i.e. deviating from "standard" bookmark structures without providing a workaround) should not be implemented also the native sync's complete non workability might be addressed. I don't wish to to take away from the excellent thing that is the Brave browser team I like the platform and use it pretty much exclusively despite the (quite a few) issues as I am sure they will be sorted in due course. I think this needs to be looked at differently hence my comments. Thanks

t3mujin commented 4 years ago

Just wanted to add my $0.02 here because I'm have a similar issue with my own sync strategy, despite not using xBrowserSync. I'm using a self-hosted solution so I have installed Floccus on all my browsers that syncs to my own Nextcloud server. Since browsers have slightly different solutions for menu/toolbar boomarks what I do is set them up separately (no big deal there, two syncs instead of one), but with this option of nesting the Other Bookmarks inside the Toolbar bookmarks it messed everything up, and with no easy workaround...

bsclifton commented 4 years ago

Possibly causing crash in Brave introduced with Switchmark - reported via https://github.com/brave/brave-browser/issues/7638

jaredready commented 4 years ago

Chiming in as another Chrome convert to say that the removal of Other Bookmarks is really annoying and does not feel well thought out.

I won't pretend to understand the complexities behind bookmark syncing, but I have to say that every browser out there handles it better than Brave does right now. Even extensions developed by single developers handle it better.

If I were involved in the development of bookmark syncing on Brave, I would be seriously considering going back to the drawing board and rearchitecting it from scratch.

Best of luck to you folks, though. I'll still be here as a mostly happy user converting everybody that I can, but I'm having to warn people that bookmark syncing is kind of a mess right now. It's the only real stain on an otherwise great browser.

jerrykrinock commented 4 years ago

Chiming in as a bookmarks syncing extension and native app developer: A week or so ago we published updates which accommodate Brave's elimination of Other Bookmarks, and our users are no longer complaining. But if Other Bookmarks were to return, even as a folder with a special pin to the right attribute, this would likely break our extension again :(

The different structures among browsers is an unfortunate fact of life that we have been dealing with for ten years. It is messy, but we do it. Of the 8 or so browsers that we support, the only two which currently have the structure are now, surprise, Brave and Vivaldi. Personally, I think that Brave's new structure, zero hard folders – just put everything in the toolbar, is the best. The user can define whatever top-level folders they want in there. Also, there is less cognitive overhead for new users – the name Other Bookmarks is a head scratcher.

By the way, we do not need different extensions – our same extension is used in Brave, Chrome, Edge, Opera and Vivaldi. Our extension for Firefox runs the same code, but needs an extra key in the package manifest :( The accommodation of different structures occurs in our native app. For web-based syncing apps, the accommodation could be done in the server.

I blame the Chrome team for originally not documenting their structure in the chrome.bookmarks API at all – we had to reverse-engineer it as discussed above in this thread, and before them, the Netscape team or whomever it was that originally conceived of a separate Bookmarks Bar and Bookmarks Menu, which is where all this mess began.

jsecretan commented 4 years ago

Hi @jerrykrinock is there a way for you to change behavior by versions? Or anything easy we could do to accommodate your use case? It's a tough spot to be in because of course many are asking us to bring back the Other Bookmarks node.

LaurentOngaro commented 4 years ago

For my part, I don't miss the "Other bookmarks" node, I find the new bookmarks structure much cleaner, but the loss of compatibility with synchronization extensions is really annoying

jerrykrinock commented 4 years ago

@jsecretan: My native app could check the Brave version. But I probably would not do it this way, because I foresee a high probability of bugs due to race conditions and other unforeseen phenomena. Instead, I would get the dev version of Brave, reverse-engineer as necessary and update the code in my native app to handle either old or new. This will be messy, but I've done stuff like this before and survived.

This is the one disadvantage of Brave not registering developers in its own extensions "store" – Brave has does not even know who the extension developers are and has no way to warn developers of breaking changes. Piggybacking on the Chrome store makes much less work for extension developers, and I very like it, muchos gracias. What would be most helpful is some kind of channel which developers could "tune" to in order to be advised of upcoming breaking changes.

jerrykrinock commented 4 years ago

@LaurentOngaro, bookmarks synchronization extensions can be updated to map between browsers that have Other Bookmarks, such as Chrome and Edge, and browsers which do not, like Brave and Vivaldi. Presumably whatever extension you using will be updated eventually. When it is, you will get best results by leaving the Other Bookmarks in Chrome, Edge or whatever empty, so that no mapping is necessary.

antonycourtney commented 4 years ago

@bridiver Is this issue still under active review by the Brave team? I'd really like to get Tabli working on Brave again. I can easily apply what I consider the hacky fix (putting "Tabli Saved Windows" on the Bookmarks Bar) to get Tabli working again, but I have held off on that for the past month in the hopes of some clear guidance on what will happen with this issue from Brave devs.

darkdh commented 4 years ago

@antonycourtney the PR for bring back "Other Bookmarks" is current under review

ch0rn commented 4 years ago

For the love of god bring back the ability to pin a folder to the right hand side. This is total crap, I personally use 3 different desktop devices all with different resolutions. So now my damn bookmarks are always in a different place.

gabrielmulle commented 4 years ago

Today I updated Brave to Version 1.3.113, but it didn't resolve the issue.

sp1nkick commented 4 years ago

I was hoping that the update today would have a fix it. Any eta?

gabrielmulle commented 4 years ago

same here, now Version 1.3.118 and still no bookmarks in my browser

bsclifton commented 4 years ago

Fix has landed and should now be in Nightly (1.7, which is slated for Release channel on April 28). Next step would be planning the back-port of this (ex: Dev 1.6/April 7, Beta 1.5/Mar 17, Release 1.4/Feb 25)

@kjozwiak @rebron @davidtemkin would this be something we want for 1.4? (upcoming release channel). Or would we want to shoot for 1.5 or 1.6? (give the fix a little more time to bake)

jerrykrinock commented 4 years ago

To summarize: There was some difficulty with bookmarks syncing, and it was determined that the complicated and ill-defined bookmarks structure inherited from Chrome was preventing a clean solution. Now, after the migration to a simpler structure has been in the field, it is decided to revert to the original structure. Our brave developers shall, without noticeable disruption, reverse-migrate users, some of whom may have several synced devices, some migrated and some not, using various versions of Brave, back to the original structure. And then, I presume, solve the original syncing difficulty.

This will be a very impressive accomplishment.

nero120 commented 4 years ago

@jerrykrinock out of interest, what exactly was so difficult about syncing the original chrome bookmarks structure?

jerrykrinock commented 4 years ago

@nero120 First of all, I'm not with Brave. I am just an bookmarks-related extension deveIoper who subscribed to this issue because I need to support Brave's past and future changes.

To answer your question, I'm just recalling what I read, maybe it was in the Brave forums or maybe it was in the original issue wherein they decided to remove Other Bookmarks. And I do recall that, in the original 2009 chrome.bookmarks API, the Other Bookmarks was not properly defined by Google, so I had to do some reverse engineering. I suspect it has something to do with the fact that synchronizing a tree is complicated enough if there is only one "hard" root item.

gabrielmulle commented 4 years ago

Another update, another version and another disappointment. How can a famous browser in 2020 be this broken? Yeah the fix is in nightly I saw, but I think I just wanted you to be this cautious before you have implemented the change that broke the bookmarks in the first place This is extremely frustrating in the point of view of an end user.

stmuk commented 4 years ago

Fix has landed and should now be in Nightly (1.7, which is slated for Release channel on April 28). Next step would be planning the back-port of this (ex: Dev 1.6/April 7, Beta 1.5/Mar 17, Release 1.4/Feb 25)

@kjozwiak @rebron @davidtemkin would this be something we want for 1.4? (upcoming release channel). Or would we want to shoot for 1.5 or 1.6? (give the fix a little more time to bake)

I am right in thinking this fix isn't in Version 1.4.95?

bsclifton commented 4 years ago

@stmuk this is currently only in Nightly (1.7)

Per our release schedule, 1.7 is scheduled to be publicly released on April 28

@rebron @kjozwiak should this fix be uplifted to 1.6? (scheduled for April 7). It's definitely too late to pull into 1.5 (scheduled for March 17)

Johann999 commented 4 years ago

Please correct the problem as soon as possible. Thank you very much.

jsecretan commented 4 years ago

FWIW, @bsclifton I would say to try to uplift into 1.6.

bsclifton commented 4 years ago

Actually- uplift may not be needed... current plan is to expedite 1.7 to April 7 😄 (skipping the 1.6 version)

jerrykrinock commented 4 years ago

I see that 1.5 is now in production and 1.7 is now in beta. So is this true that 1.6 is being skipped? And that if anyone does have 1.6 (from a nightly or whatever), that 1.6 does not have Other Bookmarks?

gabrielmulle commented 4 years ago

Actually- uplift may not be needed... current plan is to expedite 1.7 to April 7 😄 (skipping the 1.6 version)

I've been visiting the "About Brave" today to update brave and finally resolve this issue that is affecting us since January, but until now, no update. Do you know when the update will be released?

cc @bsclifton

LaurenWags commented 4 years ago

Verified passed with

Brave 1.7.86 Chromium: 80.0.3987.163 (Official Build) (64-bit)
Revision e7fbe071abe9328cdce4ffedac9822435fbd3656-refs/branch-heads/3987@{#1037}
OS macOS Version 10.14.6 (Build 18G3020)

Verified test plan from https://github.com/brave/brave-core/pull/4404

Migrate previous migration - Pass

Virtual "Other Bookmarks" Mapping

Other Scenarios I executed:

  1. Clean profile 1.7.x --> confirmed 'Other Bookmarks' is available on bookmarks manager

  2. Clean profile 1.5.x --> confirmed 'Other Bookmarks' is not available on bookmarks manager --> upgrade to 1.7.x --> confirmed 'Other Bookmarks' is available on bookmarks manager

  3. Upgrade profile from 1.1.x (Other Bookmarks exists):

    • Installed 1.1.23. (last version with 'Other Bookmarks' folder)
    • Confirmed presence of 'Other Bookmarks'
    • Added bookmarks/folders/both to 'Bookmarks' and 'Other Bookmarks' folders.
    • Upgraded to 1.2.41
    • Confirmed 'Other Bookmarks' is as per https://github.com/brave/brave-browser/issues/5158 (quick checks of this only, sync never enabled)
    • Upgrade normally to 1.5.x
    • Confirmed 'Other Bookmarks' did not return to same place as 1.1.x
    • Upgraded on test channel 1.7.x --> Confirmed 'Other Bookmarks' is moved to permanent node as expected --> Confirmed 'Other Bookmarks' displays on bookmarks bar on right side --> Confirmed able to add a bookmark to 'Other Bookmarks' after migration --> Confirmed able to add a folder to 'Other Bookmarks' after migration --> Confirmed able to add a bookmark into the folder added in step above.

before upgrade to 1.7.x:

1 5 x from 1 2 x pre migration - addtl scenario1

after upgrade to 1.7.x:

1 5 x to 1 7 x migration addtl scenario1

Verification is in progress

Brave 1.7.86 Chromium: 80.0.3987.163 (Official Build) (64-bit)
Revision e7fbe071abe9328cdce4ffedac9822435fbd3656-refs/branch-heads/3987@{#1037}
OS Windows 10 OS Version 1803 (Build 17134.1006)

Migrate previous migration

Virtual "Other Bookmarks" mapping

Device A Desktop 1.7.x, Device B Desktop 1.8.x, Device C iPhone 8 Scenario does not behave as expected, encountered https://github.com/brave/brave-browser/issues/9073#issuecomment-611375474 and logged https://github.com/brave/brave-ios/issues/2458

Verification passed on

Brave 1.7.92 Chromium: 80.0.3987.163 (Official Build) (64-bit)
Revision e7fbe071abe9328cdce4ffedac9822435fbd3656-refs/branch-heads/3987@{#1037}
OS Ubuntu 18.04 LTS

Migrate previous migration

Virtual "Other Bookmarks" mapping

rebron commented 4 years ago

Hi @gabrielmulle. We have a candidate build out now that we're testing, 1.7.86 and looking to make it available this Thursdaay.

rebron commented 4 years ago

@jerrykrinock Sorry for the super late response but yes 1.6 did not have other bookmarks. Other bookmarks was added back in 1.7.6.