feat: keyboard shortcut to export all bookmarks #25827
Conversation
browser/ui/browser_commands.cc
Outdated
| void ExportAllBookmarks(Browser* browser) { | ||
| auto* profile = browser->profile(); | ||
| DCHECK(profile); | ||
| chrome::ExportBookmarks(profile, /*export_path=*/base::FilePath()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm trying to build locally to see if this approach is valid but
cd brave-browser/src/brave/ && npm run build -- Static && npm start Static
has been running for several hours and still hasn't finished.
There was a problem hiding this comment.
Unless I'm mistaken, you should see an early failure with this stating no member named 'ExportBookmarks' in namespace 'chrome'. Where did you see that method defined? You might find BookmarksFaviconFetcher to be helpful here; it contains a method ExportBookmarks which looks promising.
hamirmahal
force-pushed
the
feat/keyboard-shortcut-to-export-all-bookmarks
branch
from
2db2f06
to
3a5baf0
Compare
Current behaviorScreencast.from.10-06-2024.10.58.15.AM.webm |
|
Nice work @hamirmahal! If you want to show a FilePicker this seems like a good place to start looking: (it sounds like you can pass |
|
Thanks, @fallaciousreasoning! |
Without this pull requestScreencast.from.10-17-2024.04.53.44.PM.webm |
With this pull requestI recommend watching this at 2x speed. Screencast.from.10-17-2024.03.54.15.PM.webm |
browser/ui/browser_commands.cc
Outdated
| raw_ptr<Profile> profile_; | ||
|
|
||
| public: | ||
| scoped_refptr<ui::SelectFileDialog> fileSelector; |
There was a problem hiding this comment.
nit: Maybe pass this in as part of the constructor (or via a setter) and have this variable be private - Chromium doesn't allow public fields in classes
| scoped_refptr<ui::SelectFileDialog> fileSelector; | |
| scoped_refptr<ui::SelectFileDialog> file_selector_; |
actually, can we remove this? I don't think we need to hold a reference to the file_selector
There was a problem hiding this comment.
We can pass in file_selector_ as part of the constructor and have it be private.
I gave it several passes, and the way I had the code with file_selector_ removed entirely resulted in the file picker still appearing, but bookmarks not ultimately being saved to disk after confirming a path. But, I'm happy to give it another look if you have code suggestions.
browser/ui/browser_commands.cc
Outdated
| } | ||
| void FileSelectionCanceled() override { | ||
| std::cout << "File selection for bookmarks export canceled" << std::endl; | ||
| delete this; |
There was a problem hiding this comment.
do we need to remove this as a listener to the FileSelectionDialog too?
There was a problem hiding this comment.
I don't know if we need to remove it, but we can remove it without losing cancellation functionality, so I did.
Co-authored-by: Jay Harris <[email protected]>
Co-authored-by: Jay Harris <[email protected]>
hamirmahal
force-pushed
the
feat/keyboard-shortcut-to-export-all-bookmarks
branch
from
dfa6a3f
to
adb2959
Compare
Resolves brave/brave-browser#41412
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.
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: