feat: keyboard shortcut to export all bookmarks #25827

hamirmahal · 2024-10-04T23:33:05Z

Allow users to create a keyboard shortcut that exports all bookmarks.

This is an action that some users, like myself, do very often. It would improve the user experience (UX) if there was a way to do this with one keyboard shortcut, as opposed to several, successive clicks.

CI: #26113

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally:
- npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
- npm run presubmit wiki, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

browser/ui/browser_commands.cc

hamirmahal · 2024-10-06T18:02:29Z

Current behavior

Screencast.from.10-06-2024.10.58.15.AM.webm

fallaciousreasoning · 2024-10-06T21:10:50Z

Nice work @hamirmahal! If you want to show a FilePicker this seems like a good place to start looking:

https://source.chromium.org/chromium/chromium/src/+/main:ui/shell_dialogs/select_file_dialog.h?q=symbol:%5Cbui::SelectFileDialog::Create%5Cb%20case:yes

(it sounds like you can pass nullptr as the second argument 😄)

hamirmahal · 2024-10-17T23:56:31Z

With this pull request

I recommend watching this at 2x speed.

Screencast.from.10-17-2024.03.54.15.PM.webm

fallaciousreasoning

This is looking great! Few more changes and I think I can kick off CI

browser/ui/browser_commands.cc

fallaciousreasoning

This is looking great - I'm kicking of CI

fallaciousreasoning · 2024-10-21T02:41:41Z

The PR seems to be failing to build on Windows with the following:

fallaciousreasoning · 2024-10-22T22:41:09Z

Would you mind rebasing? I just tried to run CI on latest but the rebase failed for conflicts

hamirmahal · 2024-10-22T23:42:18Z

I can try! The last time I did, brave/brave-browser#40509 happened.

What are the instructions to rebase properly? Is it just following the steps in https://github.com/brave/brave-browser/wiki/Chromium-rebases, exactly?

I think I rebased both repositories last time, which may have caused issues.

fallaciousreasoning · 2024-10-23T00:02:11Z

Ah yeah so in the brave repo (probably at brave-browser/src/brave):

git fetch
git rebase --onto origin/master
npm run sync
npm run build
npm run start

I think those instructions are for rebasing brave on top of the latest Chromium, which is done by our rebase team. You shouldn't need to do anything in the Chromium repo (at brave-browser/src)

hamirmahal · 2024-10-23T15:50:11Z

Thanks for the detailed instructions.

$  npm run build
 
> [email protected] build
> node ./build/commands/scripts/commands.js build

touch original files overridden by chromium_src...
...touch original files overridden by chromium_src done
update branding...
Recursing through GRD to find GRDP files...
Done recursing through GRD to find GRDP files.
...update branding done
generate ninja files...
Widevine cdm host verification is disabled
...generate ninja files done
build brave (Component, id=526f7522-97bb-469d-a182-6a44a59a20ab)...
---------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> autoninja -C /home/hamir/brave-browser/src/out/Component brave -k 1 --offline
ninja: Entering directory `/home/hamir/brave-browser/src/out/Component'
ninja: no work to do.
...build brave (Component, id=526f7522-97bb-469d-a182-6a44a59a20ab) done

Should I git pull? I think this branch lost changes locally after git rebase --onto origin/master.

On branch feat/keyboard-shortcut-to-export-all-bookmarks
Your branch is behind 'fork/feat/keyboard-shortcut-to-export-all-bookmarks' by 15 commits, and can be fast-forwarded.
  (use "git pull" to update your local branch)

nothing to commit, working tree clean

fallaciousreasoning · 2024-10-24T22:41:24Z

Hmm yeah, maybe do a git pull so you don't lose your changes.

if you do a

git remote -v

what does it output? I'm wondering whether its using your fork as the remote, rather than brave-core? If it is, there are two options:

Sync your fork with brave-core (there should be a button on your Github, I think)
Add another remote for brave-core: git remote add brave [email protected]:brave/brave-core.git

Afterwards, do a fetch

# if you added a new remote
git fetch brave

# if you synced instead
git fetch

and try (maybe I was wrong about the --onto)


# if synced
git rebase origin/master

# if new remote
git rebase brave/master

and then

npm run sync
npm run build
npm run start

hamirmahal · 2024-10-25T00:16:39Z

feat/keyboard-shortcut-to-export-all-bookmarks ~/brave-browser/src/brave 0 $  git remote -v
fork	https://github.com/hamirmahal/brave-core (fetch)
fork	https://github.com/hamirmahal/brave-core (push)
origin	https://github.com/brave/brave-core (fetch)
origin	https://github.com/brave/brave-core (push)

I'm taking a look at the other commands now...

fallaciousreasoning · 2024-10-25T00:21:49Z

Use fork instead of origin

git fetch fork
git rebase fork/master

browser/ui/browser_commands.cc

hamirmahal · 2024-10-30T21:41:56Z

No worries!

I think it'd be nice if users could export bookmarks from any page without having to navigate to chrome://bookmarks, first, but that behavior could be a bit unexpected.

How much work would it be to add a shortcut to the WebUI itself?

I can look into enabling the command only if the active page is chrome://bookmarks. I'm not sure if there are any downsides to that implementation.

fallaciousreasoning · 2024-10-30T23:11:40Z

I think the approach is fine for now - I think if you add the shortcut you won't be too surprised if it works from anywhere (and I don't expect a huge number of people will add one).

The other advantage of this way is it will show up in the list of quick commands (i.e if you press Ctrl+Space)

bsclifton · 2024-10-31T07:57:06Z

Nice work @hamirmahal! I remember you were keen on shortcuts a while back and wow - you dug in for sure!

Since this isn't a bound shortcut by default, it should be fine to be a global shortcut (ex: not restricted to a Web UI). Like @fallaciousreasoning is saying, it'll show in the quick commands which is kind of a nice touch

@hamirmahal a good rule of thumb when doing browser dev when you're not sure about something: check what our existing code does or what Chromium does. I don't think we have any page specific shortcuts and I doubt Chromium does either. If that's the case, then you're safe having it as a global one. I would only change if there was a good example that Chromium had (ex: they set a precedent)

hamirmahal · 2024-10-31T16:51:25Z

Thanks @bsclifton! Speaking of checking existing code, I saw base::FilePath GetDefaultFilepathForBookmarkExport() in chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc.

That seems to have a lot of the functionality we want, but it looks like it's not in this repository, so maybe we want to reproduce the function in its entirety in browser/ui/browser_commands.cc. It looks like it gives a file in the format bookmarks_10_31_24.html.

fallaciousreasoning · 2024-10-31T19:31:01Z

@hamirmahal yeah that seems like a reasonable approach to me (just taking what that function does). Nice work 😄

the default filepath for bookmark export. This changes the default bookmark filename from `YYYY_MM_DD_brave_browser_bookmarks.html` to `bookmarks_MM_DD_YY.html`

browser/ui/browser_commands.cc

fallaciousreasoning · 2024-11-04T22:51:07Z

looks good - kicked off CI again 🤞

bsclifton · 2024-11-11T04:52:53Z

#26113 looks good! Merging 😄 Thanks @hamirmahal!

brave-builds · 2024-11-11T12:05:04Z

Released in v1.75.9

hamirmahal · 2024-11-11T18:54:48Z

You're welcome, @bsclifton!

fallaciousreasoning · 2024-11-11T19:09:26Z

Great work @hamirmahal!

hamirmahal · 2024-11-11T19:54:07Z

Thanks @fallaciousreasoning!

fallaciousreasoning · 2024-11-11T23:05:09Z

Glad we got it in eventually, sorry for the endless rebase-CI cycles :/

Looks like it made it into todays Nightly!

jonathansampson reviewed Oct 5, 2024

View reviewed changes

browser/ui/browser_commands.cc Outdated Show resolved Hide resolved

hamirmahal marked this pull request as ready for review October 17, 2024 23:55

hamirmahal requested a review from a team as a code owner October 17, 2024 23:55

fallaciousreasoning reviewed Oct 18, 2024

View reviewed changes

hamirmahal requested a review from fallaciousreasoning October 18, 2024 23:11

fallaciousreasoning reviewed Oct 20, 2024

View reviewed changes

browser/ui/browser_commands.cc Outdated Show resolved Hide resolved

fallaciousreasoning reviewed Oct 20, 2024

View reviewed changes

brave-builds mentioned this pull request Oct 20, 2024

CI run for contributor PR #25827 #26113

Closed

hamirmahal requested a review from fallaciousreasoning October 21, 2024 18:31

hamirmahal requested review from deeppandya, a team and iefremov as code owners October 25, 2024 03:24

fallaciousreasoning removed request for a team, bridiver, fmarier, iefremov and deeppandya October 30, 2024 19:43

petemill reviewed Oct 30, 2024

View reviewed changes

browser/ui/browser_commands.cc Outdated Show resolved Hide resolved

use GetDefaultFilepathForBookmarkExport() to get

356b364

the default filepath for bookmark export. This changes the default bookmark filename from `YYYY_MM_DD_brave_browser_bookmarks.html` to `bookmarks_MM_DD_YY.html`

hamirmahal commented Nov 1, 2024

View reviewed changes

browser/ui/browser_commands.cc Outdated Show resolved Hide resolved

refactor: move fn and Listener to top namespace

538142a

fallaciousreasoning approved these changes Nov 10, 2024

View reviewed changes

bsclifton merged commit 79ab3df into brave:master Nov 11, 2024
3 checks passed

bsclifton assigned hamirmahal Nov 11, 2024

hamirmahal deleted the feat/keyboard-shortcut-to-export-all-bookmarks branch November 11, 2024 18:54

rebron added this to the 1.75.x - Nightly milestone Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: keyboard shortcut to export all bookmarks #25827

feat: keyboard shortcut to export all bookmarks #25827

hamirmahal commented Oct 4, 2024 •

edited by fallaciousreasoning

Loading

hamirmahal commented Oct 6, 2024

fallaciousreasoning commented Oct 6, 2024

hamirmahal commented Oct 17, 2024

fallaciousreasoning left a comment

fallaciousreasoning left a comment

fallaciousreasoning commented Oct 21, 2024

fallaciousreasoning commented Oct 22, 2024 •

edited

Loading

hamirmahal commented Oct 22, 2024

fallaciousreasoning commented Oct 23, 2024 •

edited

Loading

hamirmahal commented Oct 23, 2024

fallaciousreasoning commented Oct 24, 2024 •

edited

Loading

hamirmahal commented Oct 25, 2024

fallaciousreasoning commented Oct 25, 2024

hamirmahal commented Oct 30, 2024

fallaciousreasoning commented Oct 30, 2024

bsclifton commented Oct 31, 2024

hamirmahal commented Oct 31, 2024

fallaciousreasoning commented Oct 31, 2024

fallaciousreasoning commented Nov 4, 2024

bsclifton commented Nov 11, 2024

brave-builds commented Nov 11, 2024

hamirmahal commented Nov 11, 2024

fallaciousreasoning commented Nov 11, 2024

hamirmahal commented Nov 11, 2024

fallaciousreasoning commented Nov 11, 2024 •

edited

Loading

feat: keyboard shortcut to export all bookmarks #25827

feat: keyboard shortcut to export all bookmarks #25827

Conversation

hamirmahal commented Oct 4, 2024 • edited by fallaciousreasoning Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

hamirmahal commented Oct 6, 2024

Current behavior

fallaciousreasoning commented Oct 6, 2024

hamirmahal commented Oct 17, 2024

With this pull request

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning left a comment

Choose a reason for hiding this comment

fallaciousreasoning commented Oct 21, 2024

fallaciousreasoning commented Oct 22, 2024 • edited Loading

hamirmahal commented Oct 22, 2024

fallaciousreasoning commented Oct 23, 2024 • edited Loading

hamirmahal commented Oct 23, 2024

fallaciousreasoning commented Oct 24, 2024 • edited Loading

hamirmahal commented Oct 25, 2024

fallaciousreasoning commented Oct 25, 2024

hamirmahal commented Oct 30, 2024

fallaciousreasoning commented Oct 30, 2024

bsclifton commented Oct 31, 2024

hamirmahal commented Oct 31, 2024

fallaciousreasoning commented Oct 31, 2024

fallaciousreasoning commented Nov 4, 2024

bsclifton commented Nov 11, 2024

brave-builds commented Nov 11, 2024

hamirmahal commented Nov 11, 2024

fallaciousreasoning commented Nov 11, 2024

hamirmahal commented Nov 11, 2024

fallaciousreasoning commented Nov 11, 2024 • edited Loading

hamirmahal commented Oct 4, 2024 •

edited by fallaciousreasoning

Loading

fallaciousreasoning commented Oct 22, 2024 •

edited

Loading

fallaciousreasoning commented Oct 23, 2024 •

edited

Loading

fallaciousreasoning commented Oct 24, 2024 •

edited

Loading

fallaciousreasoning commented Nov 11, 2024 •

edited

Loading