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.

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

feat: keyboard shortcut to export all bookmarks #25827

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions app/brave_command_ids.h
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@

// Bookmark commands.
#define IDC_TOGGLE_ALL_BOOKMARKS_BUTTON_VISIBILITY 56300
#define IDC_EXPORT_ALL_BOOKMARKS 56312

#define IDC_COMMANDER 56301

Expand Down
4 changes: 4 additions & 0 deletions browser/ui/brave_browser_command_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,7 @@ void BraveBrowserCommandController::InitBraveCommandState() {
UpdateCommandsForPin();

UpdateCommandEnabled(IDC_TOGGLE_ALL_BOOKMARKS_BUTTON_VISIBILITY, true);
UpdateCommandEnabled(IDC_EXPORT_ALL_BOOKMARKS, true);

if (browser_->is_type_normal()) {
// Delete these when upstream enables by default.
Expand Down Expand Up @@ -656,6 +657,9 @@ bool BraveBrowserCommandController::ExecuteBraveCommandWithDisposition(
case IDC_TOGGLE_ALL_BOOKMARKS_BUTTON_VISIBILITY:
brave::ToggleAllBookmarksButtonVisibility(base::to_address(browser_));
break;
case IDC_EXPORT_ALL_BOOKMARKS:
brave::ExportAllBookmarks(&*browser_);
break;
case IDC_COMMANDER:
#if BUILDFLAG(ENABLE_COMMANDER)
brave::ToggleCommander(base::to_address(browser_));
Expand Down
53 changes: 53 additions & 0 deletions browser/ui/browser_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "brave/components/speedreader/common/buildflags/buildflags.h"
#include "brave/components/tor/buildflags/buildflags.h"
#include "brave/components/url_sanitizer/browser/url_sanitizer_service.h"
#include "chrome/browser/bookmarks/bookmark_html_writer.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
Expand All @@ -60,6 +61,8 @@
#include "content/public/browser/web_contents.h"
#include "ui/base/clipboard/clipboard_buffer.h"
#include "ui/base/clipboard/scoped_clipboard_writer.h"
#include "ui/shell_dialogs/select_file_policy.h"
#include "ui/shell_dialogs/selected_file_info.h"
#include "url/origin.h"

#if defined(TOOLKIT_VIEWS)
Expand Down Expand Up @@ -898,6 +901,56 @@ void ScrollTabToBottom(Browser* browser) {
contents->ScrollToBottomOfDocument();
}

namespace {

class BookmarksExportListener : public ui::SelectFileDialog::Listener {
private:
raw_ptr<Profile> profile_;
hamirmahal marked this conversation as resolved.
Show resolved Hide resolved

public:
scoped_refptr<ui::SelectFileDialog> fileSelector;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


explicit BookmarksExportListener(Profile* profile) : profile_(profile) {}
void FileSelected(const ui::SelectedFileInfo& file, int index) override {
bookmark_html_writer::WriteBookmarks(profile_, file.file_path, nullptr);
std::cout << "Bookmarks exported to " << file.file_path.value()
<< std::endl;
hamirmahal marked this conversation as resolved.
Show resolved Hide resolved

delete this;
}
void FileSelectionCanceled() override {
std::cout << "File selection for bookmarks export canceled" << std::endl;
hamirmahal marked this conversation as resolved.
Show resolved Hide resolved
delete this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to remove this as a listener to the FileSelectionDialog too?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we need to remove it, but we can remove it without losing cancellation functionality, so I did.

}
};

} // namespace

void ExportAllBookmarks(Browser* browser) {
const std::time_t t = std::time(0);
const std::tm* time = std::localtime(&t);
const std::string year = std::to_string(time->tm_year + 1900);
const std::string month = std::to_string(time->tm_mon + 1);
const std::string day = std::to_string(time->tm_mday);
const std::string formatted_time = year + "_" + month + "_" + day;
const std::string defaultBookmarksFilename = base::StringPrintf(
"%s_Brave_browser_bookmarks.html", formatted_time.c_str());
hamirmahal marked this conversation as resolved.
Show resolved Hide resolved

ui::SelectFileDialog::FileTypeInfo file_types;

// Only show HTML files in the file dialog.
file_types.extensions.push_back({"html"});

BookmarksExportListener* listener =
new BookmarksExportListener(browser->profile());

listener->fileSelector = ui::SelectFileDialog::Create(listener, nullptr);
listener->fileSelector->SelectFile(
ui::SelectFileDialog::SELECT_SAVEAS_FILE, u"Export Bookmarks",
hamirmahal marked this conversation as resolved.
Show resolved Hide resolved
base::FilePath::FromUTF8Unsafe(defaultBookmarksFilename), &file_types, 1,
FILE_PATH_LITERAL("html"), browser->window()->GetNativeWindow(), nullptr);
}

void ToggleAllBookmarksButtonVisibility(Browser* browser) {
auto* prefs = browser->profile()->GetPrefs();
prefs->SetBoolean(
Expand Down
1 change: 1 addition & 0 deletions browser/ui/browser_commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ void UnmuteAllTabs(Browser* browser);
void ScrollTabToTop(Browser* browser);
void ScrollTabToBottom(Browser* browser);

void ExportAllBookmarks(Browser* browser);
void ToggleAllBookmarksButtonVisibility(Browser* browser);

// In case |tab| is not provided, the active tab will be used.
Expand Down
3 changes: 3 additions & 0 deletions components/resources/commands.grdp
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,9 @@
<message name="IDS_IDC_BRAVE_BOOKMARK_BAR_NEVER">
Hide bookmark bar
</message>
<message name="IDS_IDC_EXPORT_ALL_BOOKMARKS">
Export all bookmarks
</message>
<message name="IDS_IDC_TOGGLE_ALL_BOOKMARKS_BUTTON_VISIBILITY">
Toggle All bookmarks button visibility
</message>
Expand Down