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

Make it possible to view all commands in Brave, and their shortcuts #16845

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

fallaciousreasoning
Copy link
Contributor

@fallaciousreasoning fallaciousreasoning commented Jan 25, 2023

Fixes brave/brave-browser#28329

This PR makes it possible to view the available commands in Brave on the brave://commands page when the brave://flags/#brave-commands flag is on.

image

@fallaciousreasoning fallaciousreasoning requested a review from a team as a code owner January 25, 2023 03:05
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false labels Jan 25, 2023
app/command_utils.cc Outdated Show resolved Hide resolved
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

browser/about_flags.cc Outdated Show resolved Hide resolved
components/resources/BUILD.gn Outdated Show resolved Hide resolved
test/BUILD.gn Outdated Show resolved Hide resolved
resources/resource_ids.spec Outdated Show resolved Hide resolved
app/command_utils.cc Outdated Show resolved Hide resolved
app/command_utils.h Outdated Show resolved Hide resolved
app/command_utils.cc Outdated Show resolved Hide resolved
app/command_utils.cc Outdated Show resolved Hide resolved
app/command_utils.cc Outdated Show resolved Hide resolved
app/command_utils.cc Outdated Show resolved Hide resolved
browser/ui/webui/commands_ui.cc Outdated Show resolved Hide resolved
browser/ui/webui/commands_ui.h Outdated Show resolved Hide resolved
components/commands/browser/resources/utils/match.ts Outdated Show resolved Hide resolved
Comment on lines 16 to 23
std::string GetKeyName(ui::KeyboardCode code);
std::vector<std::string> GetModifierName(ui::KeyEventFlags flags);
Copy link
Contributor

@sangwoo108 sangwoo108 Feb 7, 2023

Choose a reason for hiding this comment

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

I'm wondering if we can use Accelerator::GetShortcutTexts() instead of managing all the characters. There could have problems like

  • We might have to split the shorcut text with +
  • We might have to distinguish which one is modifier - maybe we could consider names longer than 1 as modifiers. or hardcode modifiers but hope this could be fewer than all characters

I'm kind of prefer this version, as there're tons of characters, and we might need to consider localization per contries's keyboard layout when we manage them by ourselves 😹 But it's totally your call!

Copy link
Contributor Author

@fallaciousreasoning fallaciousreasoning Feb 7, 2023

Choose a reason for hiding this comment

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

I just had a play with changing it over, and it's quite a bit worse - there are a bunch of missing keys, and a bunch of keys which are displayed wrong:

  1. All function keys are missing
  2. Special keys are mislabelled (Keyboard back/forward is Arrow left/right)
  3. Cmd modifier doesn't seem to show up on Linux

I think longer term it might be good to try and fix the GetShortcutsText function (maybe upstream...) but for now this produces much better results :'(

Copy link
Contributor

@sangwoo108 sangwoo108 Feb 8, 2023

Choose a reason for hiding this comment

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

(maybe upstream...)

Yeah, I don't want you to go through that tough way haha. Maybe we can wrap the GetShortcutText() and fix up some of the results later on.

app/command_utils.cc Outdated Show resolved Hide resolved
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@fallaciousreasoning fallaciousreasoning requested a review from a team as a code owner February 15, 2023 00:46
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@fallaciousreasoning
Copy link
Contributor Author

@rillian I accidentally committed Cargo.lock, which was being modified when I was building. I've reverted the change, mind dismissing the review (or reviewing)

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@antonok-edm antonok-edm removed the request for review from a team February 17, 2023 19:49
@antonok-edm
Copy link
Collaborator

@fallaciousreasoning dismissed rust-deps-reviewers for you 😄

@fallaciousreasoning fallaciousreasoning force-pushed the shortcuts-list branch 2 times, most recently from f479497 to 3a18b17 Compare February 19, 2023 20:10
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@fallaciousreasoning fallaciousreasoning merged commit 48af20f into master Feb 20, 2023
@fallaciousreasoning fallaciousreasoning deleted the shortcuts-list branch February 20, 2023 23:35
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Feb 20, 2023
@aguscruiz
Copy link
Collaborator

@fallaciousreasoning Worked on the design here, it's a simple restyling for now to just use Brave's flavors:
https://www.figma.com/file/tLXWGCpNoiJxDZDdpfordj/Desktop-Settings?node-id=3617%3A77370&t=edamn8UoehFfaggp-1

source_set("browser_tests") {
testonly = true

if (!is_android && !is_ios) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I realize this is an old PR, but I'm trying to understand this part. Is it intentional to only build this browser test on desktop? Unless I'm not understanding, this browser test is definitely meant to run on Android at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the code doesn't run on Android/iOS at all, so the test only runs on desktop too - it should probably have its own buildflag.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I think you're saying that command_utils_browsertest isn't supported on Android. But this change also prevents brave_main_delegate_browsertest from running on Android (even though it was previously supported there). Maybe I'm not understanding though?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have a PR that's addressing this, just trying to figure out if it was intentional. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That probably was not intentional 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, no problem! I'm moving the feature tests into a unit test and also making them run on Android in this PR: #22516.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Keyboard Shortcuts more Discoverable
10 participants