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

Add new keyboard shortcuts for things not already customizable #39489

Open
hamirmahal opened this issue Jun 30, 2024 · 20 comments
Open

Add new keyboard shortcuts for things not already customizable #39489

hamirmahal opened this issue Jun 30, 2024 · 20 comments
Labels
feature/keyboard-shortcuts feature-request priority/P5 Not scheduled. Don't anticipate work on this any time soon.

Comments

@hamirmahal
Copy link

Platforms

Linux, macOS, Windows

Description

It'd be nice if users could create custom keyboard shortcuts to do specific things in Brave.

@hamirmahal
Copy link
Author

For example, it'd be useful if I could do Ctrl + Shift + E to export all bookmarks when on the bookmarks page.

@hamirmahal
Copy link
Author

It'd also be nice if I could add a shortcut to clear all downloads, maybe with Ctrl + Shift + C, when on the downloads page.

@hamirmahal
Copy link
Author

I think it'd be really helpful if user-created custom keyboard shortcuts could be page-specific.

@bsclifton
Copy link
Member

@hamirmahal we have brave://settings/system/shortcuts - are there things we're missing? I guess check the page and see if the above actions are registered on the page

cc: @fallaciousreasoning

@hamirmahal
Copy link
Author

@bsclifton it looks like there aren't any preexisting shortcuts for clearing all downloads and exporting bookmarks.

image

I think it'd be really useful for users if we could add custom keyboard shortcuts for arbitrary browser actions.

@fallaciousreasoning
Copy link

fallaciousreasoning commented Jul 1, 2024

Hey @hamirmahal! Thanks, for you feedback - I agree that it'd be pretty handy, but unfortunately it will probably be quite a bit of work to expose everything the browser can do as a command 🫤

If you wanted to add a command for exporting bookmarks and/or clearing downloads we'd definitely accept a PR - otherwise it's probably going to be a while until we can get to it though.

@bsclifton bsclifton added the priority/P5 Not scheduled. Don't anticipate work on this any time soon. label Jul 1, 2024
@bsclifton bsclifton changed the title Allow users to create custom keyboard shortcuts Add new keyboard shortcuts for things not already customizable Jul 1, 2024
@hamirmahal
Copy link
Author

Hello, @fallaciousreasoning! You're welcome.

I'm interested in what the process looks like for adding a new keyboard shortcut. What parts of the code base would someone need to modify to do that?

@fallaciousreasoning
Copy link

fallaciousreasoning commented Jul 2, 2024

This PR here adds a few new commands:
brave/brave-core#19035

Basically there are a few different bits:

  1. Add a new command to app/brave_command_ids.h (normally at the bottom with a little gap with the other ids)
  2. Add case statements for the new command in browser/ui/brave_browser_command_controller.cc
  3. Implement the functionality in browser/ui/browser_commands.cc (probably the hardest part will be finding out where the existing code to Clear history / export bookmarks lives, and how to call it)
  4. Add a string for the command as IDS_IDC_<COMMAND_NAME> in commands.grdp

After this the commands should be available from the command palette and brave://settings/system/shortcuts

  1. Optional: If you want to add a default shortcut, add an entry in chromium_src/chrome/browser/ui/views/accelerator_table.cc

The hardest parts will probably be:

  1. Getting Brave initially building locally
  2. Finding the code which you want your command to trigger

Happy to lend a hand if you need it!

@hamirmahal
Copy link
Author

Thanks for offering to help!

I did some rebasing after making some changes locally and some commands like npm run sync don't work anymore.

I don't mind re-cloning the repository for a fresh slate, but I'd like to avoid re-downloading 30 GB for chromium. Are there any docs you can point me to for fixing sync issues?

@hamirmahal
Copy link
Author

#40509

@fallaciousreasoning
Copy link

Hmm, that's weird, normally sync is pretty consistent. (completely understand not wanting to reclone Chromium - its massive)!

Can you try:

npm run init -- --force -C -D

followed by an

npm run sync

@hamirmahal
Copy link
Author

Hmm... I should be running npm run init -- --force -C -D in brave-browser/src/brave, right?

  1. I did git pull in brave-browser/.
  2. cd src/brave/
  3. git pull
  4. npm run init -- --force -C -D

It errors with a similar message. The working tree was clean in brave-browser/src/brave and brave-browser before doing npm run init -- --force -C -D, although src/.gitkeep is gone from brave-browser/ afterward.

[Error: ENOENT: no such file or directory, open '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'
}
Full output
~/.../src/brave (master)$   npm run init -- --force -C -D

> [email protected] init
> cd ../../ && npm run --prefix src/brave sync -- --init --force -C -D


> [email protected] sync
> node ./build/commands/scripts/sync.js --init --force -C -D

gclient sync...
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git rev-parse HEAD
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git rev-parse refs/tags/128.0.6613.40
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git log -n 1 --pretty=format:%h%d
Chromium repo needs sync.
  target is refs/tags/128.0.6613.40 at commit ce7f05019fe091c912aa09c989eb3d7d406d0f3c
  current commit is 4e22a93b12f76 (HEAD -> main, origin/main, origin/HEAD) at commit 4e22a93b12f76cc59ee084f7f6887afc232e2afd
  latest successful sync is {
    "chromiumRef": "refs/tags/128.0.6613.40",
    "gClientTimestamp": "1722654117101.6826"
}
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser
> gclient sync --nohooks --revision src@refs/tags/128.0.6613.40 --reset --upstream --force -D
Syncing projects: 100% (166/166), done.                                                                                         
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git log -n 1 --pretty=format:%h%d
Chromium is now at 4e22a93b12f76 (HEAD -> main, origin/main, origin/HEAD)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src/brave
> gclient sync --nohooks --force -D
Syncing projects: 100% (14/14), done.                                                     
...gclient sync done
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> vpython3 /home/hamir/brave-browser/src/brave/vendor/depot_tools/build_telemetry.py opt-out
apply patches...
Brave Browser Sync ERROR:
[Error: ENOENT: no such file or directory, open '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'
}

1 ~/.../src/brave (master)$  git status
 On branch master
Your branch is up to date with 'origin/master'.

nothing to commit, working tree clean

0 ~/.../src/brave (master)$  git pull
Already up to date.

@fallaciousreasoning
Copy link

Yeah that sounds right @hamirmahal.

Hmm, could you try manually resetting Chromium (in your brave-browser/src folder git reset --hard ce7f05019fe091c912aa09c989eb3d7d406d0f3c)

And in your brave folder

git fetch
git rebase --onto origin/master

and then try to init again?

@fallaciousreasoning
Copy link

Ah, the git pull should be in brave-browser/src/brave - we pretty much soley work in the brave-browser/src/brave directory 😄

@hamirmahal
Copy link
Author

Hmm... That doesn't seem to work, either.

Full output
0 ~/brave-browser/src (main)$  git reset --hard ce7f05019fe091c912aa09c989eb3d7d406d0f3c
Updating files: 100% (226131/226131), done.
HEAD is now at ce7f05019fe09 Incrementing VERSION to 128.0.6613.40

0 ~/brave-browser/src (main)$  cd brave/

0 ~/.../src/brave (master)$  git fetch

0 ~/.../src/brave (master)$  git rebase --onto origin/mastegit rebase --onto origin/master
Current branch master is up to date.

0 ~/.../src/brave (master)$   npm run init -- --force -C -D

> [email protected] init
> cd ../../ && npm run --prefix src/brave sync -- --init --force -C -D


> [email protected] sync
> node ./build/commands/scripts/sync.js --init --force -C -D

gclient sync...
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git rev-parse HEAD
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git rev-parse refs/tags/128.0.6613.40
Chromium repo does not need sync as it is already refs/tags/128.0.6613.40 at commit ce7f05019fe091c912aa09c989eb3d7d406d0f3c.
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser
> gclient sync --nohooks --revision src@refs/tags/128.0.6613.40 --reset --upstream --force -D
Syncing projects: 100% (166/166), done.                                                                                         
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> git log -n 1 --pretty=format:%h%d
Chromium is now at 4e22a93b12f76 (HEAD -> main, origin/main, origin/HEAD)
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src/brave
> gclient sync --nohooks --force -D
Syncing projects: 100% (14/14), done.                                                     
...gclient sync done
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> vpython3 /home/hamir/brave-browser/src/brave/vendor/depot_tools/build_telemetry.py opt-out
apply patches...
Brave Browser Sync ERROR:
[Error: ENOENT: no such file or directory, open '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'
}

1 ~/.../src/brave (master)$

@fallaciousreasoning
Copy link

okay, that seems super weird - it seems to think you're on the right revision but the version_info/BUILD.gn file doesn't exist.

If you check, does that folder/file exist? I checked on my local and its there. Interestingly it looks like it might be failing to apply patches.

Do you get the same error running

npm run apply_patches

(in src/brave)

@hamirmahal
Copy link
Author

If you check, does that folder/file exist? I checked on my local and its there. Interestingly it looks like it might be failing to apply patches.

It looks like that file doesn't exist.

--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
/home/hamir/brave-browser/src
> vpython3 /home/hamir/brave-browser/src/brave/vendor/depot_tools/build_telemetry.py opt-out
apply patches...
Brave Browser Sync ERROR:
[Error: ENOENT: no such file or directory, open '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'
}

1 ~/.../src/brave (master)$  cd ..
0 ~/brave-browser/src (main)$  ls 
android_webview/         chrome/                  dbus/                    .gitignore               .mailmap                 PRESUBMIT_test.py        testing/
apps/                    chromecast/              DEPS                     .gn                      media/                   printing/                third_party/
ash/                     chromeos/                device/                  google_apis/             mojo/                    README.md                tools/
ATL_OWNERS               .clang-format            DIR_METADATA             google_update/           native_client/           remoting/                ui/
AUTHORS                  .clang-tidy              docs/                    gpu/                     native_client_sdk/       rlz/                     url/
base/                    codelabs/                .eslintrc.js             headless/                net/                     .rustfmt.toml            v8/
brave/                   CODE_OF_CONDUCT.md       extensions/              infra/                   out/                     sandbox/                 .vpython3
build/                   codereview.settings      fuchsia_web/             ios/                     OWNERS                   services/                WATCHLISTS
BUILD.gn                 components/              gin/                     ipc/                     pdf/                     skia/                    weblayer/
build_overrides/         content/                 .git/                    .landmines               ppapi/                   sql/                     .yapfignore
buildtools/              courgette/               .gitattributes           LICENSE                  PRESUBMIT.py             storage/                 
cc/                      crypto/                  .git-blame-ignore-revs   LICENSE.chromium_os      PRESUBMIT_test_mocks.py  styleguide/              
0 ~/brave-browser/src (main)$  ls base/version_info
ls: cannot access 'base/version_info': No such file or directory
2 ~/brave-browser/src (main)$  ls base/version
version.cc           version.h            version_unittest.cc

And I get a similar error when doing npm run apply_patches.

~/.../src/brave (master)$  npm run apply_patches

> [email protected] apply_patches
> node ./build/commands/scripts/commands.js apply_patches

apply patches...
[Error: ENOENT: no such file or directory, open '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/home/hamir/brave-browser/src/base/version_info/BUILD.gn'
}

@fallaciousreasoning
Copy link

fallaciousreasoning commented Aug 23, 2024

Okay that's interesting - it sounds like your Chromium checkout might be in a weird state. I just had a look at the base/version_info/BUILD.gn and it looks like its been around since 2015 at least.

My Chromium is at 5b672c5c04334afc4a7d0a025171853adedb799d as of today. If you do a git show --stat in the src directory, what does it show?

Additionally, what is the Chromium version you have in src/brave/package.json (mine says "tag": "128.0.6613.85")

One last debugging idea - what if you do a git status | grep version_info in src? Mine shows base/version_info/BUILD.gn as modified - maybe yours has somehow been removed (in which case, maybe a git checkout -- base/version_info/BUILD.gn might help).

I wonder how it got into this state? Normally sync is pretty good at making sure brave/chrome are pointing at the right version

@hamirmahal
Copy link
Author

git show --stat
~/brave-browser/src (main)$  git show --stat
commit 4e22a93b12f76cc59ee084f7f6887afc232e2afd (HEAD -> main, origin/main, origin/HEAD)
Author: Takashi Sakamoto <[email protected]>
Date:   Fri Jul 28 08:27:42 2023 +0000

    [PA] Replace PA_[D]CHECK() under partition_alloc_base with PA_BASE_[D]CHECK().
    
    Make //base/allocator/partition_allocator/partition_alloc_base not
    depend on //base/allocator/partition_allocator.
    
    Change-Id: I314736d5613d891ddf74ee7e6720e7871dc10610
    Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4720679
    Reviewed-by: Yuki Shiino <[email protected]>
    Commit-Queue: Takashi Sakamoto <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#1176487}

 base/allocator/partition_allocator/partition_alloc_base/bits.h                                         |  6 +++---
 base/allocator/partition_allocator/partition_alloc_base/cxx17_backports.h                              |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/files/file_path.cc                             |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/mac/foundation_util.mm                         |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/mac/mac_util.mm                                |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/mac/scoped_typeref.h                           |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/memory/ref_counted.cc                          |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/memory/ref_counted.h                           | 18 +++++++++---------
 base/allocator/partition_allocator/partition_alloc_base/memory/scoped_refptr.h                         | 10 +++++-----
 base/allocator/partition_allocator/partition_alloc_base/native_library_posix.cc                        |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/rand_util.cc                                   |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/rand_util_pa_unittest.cc                       |  5 +++--
 base/allocator/partition_allocator/partition_alloc_base/rand_util_posix.cc                             |  6 +++---
 base/allocator/partition_allocator/partition_alloc_base/rand_util_win.cc                               |  8 ++++----
 base/allocator/partition_allocator/partition_alloc_base/threading/platform_thread_mac_for_testing.mm   |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/threading/platform_thread_posix_for_testing.cc |  6 +++---
 base/allocator/partition_allocator/partition_alloc_base/threading/platform_thread_win_for_testing.cc   | 10 +++++-----
 base/allocator/partition_allocator/partition_alloc_base/time/time.h                                    | 14 +++++++-------
 base/allocator/partition_allocator/partition_alloc_base/time/time_conversion_posix.cc                  |  6 +++---
 base/allocator/partition_allocator/partition_alloc_base/time/time_fuchsia.cc                           |  8 ++++----
 base/allocator/partition_allocator/partition_alloc_base/time/time_mac.mm                               | 20 ++++++++++----------
 base/allocator/partition_allocator/partition_alloc_base/time/time_now_posix.cc                         |  6 +++---
 base/allocator/partition_allocator/partition_alloc_base/time/time_override.cc                          |  4 ++--
 base/allocator/partition_allocator/partition_alloc_base/time/time_win.cc                               | 17 +++++++++--------
 24 files changed, 91 insertions(+), 89 deletions(-)

src/brave/package.json

  "config": {
    "projects": {
      "chrome": {
        "dir": "src",
        "tag": "128.0.6613.40",
        "repository": {
          "url": "https://github.com/brave/chromium"
        }
      },
      "depot_tools": {
        "dir": "vendor/depot_tools",
        "repository": {
          "url": "https://chromium.googlesource.com/chromium/tools/depot_tools.git"
        }
      }
    }
  },

0 ~/brave-browser/src (main)$  git status | grep version_info
1 ~/brave-browser/src (main)$ 

1 ~/brave-browser/src (main)$  git checkout -- base/version_info/BUILD.gn
error: pathspec 'base/version_info/BUILD.gn' did not match any file(s) known to git
1 ~/brave-browser/src (main)$ 

Just as a sanity check, are the remote URLs for each of these directories correct?

0 ~/brave-browser (master)$ git remote get-url origin
https://github.com/brave/brave-browser/
0 ~/brave-browser (master)$ 
0 ~/brave-browser/src (main)$ git remote get-url origin
https://github.com/brave/chromium
0 ~/brave-browser/src (main)$ 
0 ~/.../src/brave (master)$ git remote get-url origin
https://github.com/brave/brave-core
0 ~/.../src/brave (master)$ 

@fallaciousreasoning
Copy link

@hamirmahal I had a talk to a few other people and it sounds like maybe something went wrong with the initial checkout? Not sure what to do at this point except try recloning.

(the remote URLs all look good to me)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/keyboard-shortcuts feature-request priority/P5 Not scheduled. Don't anticipate work on this any time soon.
Projects
None yet
Development

No branches or pull requests

3 participants