Modify Ash and Chrome shortcuts should update Keyboard Shortcut Viewer content. |
||
Issue descriptionCreate this mega bug for future Keyboard Shortcut Viewer updates when we modify Ash and Chrome shortcuts (those available on Chrome OS). Whenever we modify any shortcuts, we need to also update the Keyboard Shortcut Viewer content. I.e. if you are deleting/adding/modifying shortcuts, please also delete/add/modify the corresponding strings and items to the list of KeyboardShortcutItem. Currently the Keyboard Shortcut Viewer is under ash/components/shortcut_viewer/. The shortcut strings is in ash/components/shortcut_viewer_strings.grdp. The shortcut metadata is defined in ash/components/shortcut_viewer/keyboard_shortcut_viewer_metadata.cc.
,
Aug 8
(I don't see anything R-V-G-worthy here.)
,
Aug 8
Hi derat@, There were several options when we implemented this feature. We had considered reusing the struct in ash/public/cpp/accelerators.cc. It is possible to unify them, but due to deps and some other implementation details, we choose current method. Please see the descriptions on page 4 in the design doc: go/chrome-os-ksv Thanks!
,
Aug 8
Yes, we are intentionally duplicating the data. There are dependency issues related to mustash and chrome browser accelerators -- imagine the shortcut viewer as its own app (maybe even its own binary). It should not have dependencies on chrome browser's accelerator data. The simplest approach is to duplicate all the accelerator data. The shortcut viewer isn't really part of //ash anymore. It just lives in //ash/components because there's no good place to put chromeos-specific UI apps. However, we should have tests that fail if the accelerator tables get out of sync. If that's not happening, we should fix the test.
,
Aug 8
Hi jamescook@, We do have a test [1] to catch ash and chrome side shortcut changes and suggest the devs to update the KeyboardShortcutViewerMetadata as well. But we still have three cases (maybe more) will cause out of sync: 1. Devs delete the items in KeyboardShortcutViewerMetadata. 2. Devs do not update the KeyboardShortcutViewerMetadata, but change the hash code in the test. 3. Shortcut got reused for different function, the description in KeyboardShortcutViewerMetadata is not accurate any more. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/ksv/keyboard_shortcut_viewer_metadata_unittest.cc?l=206&rcl=6db4f93b1ffc027f95e2f6122478525cbb94ea1e |
||
►
Sign in to add a comment |
||
Comment 1 by derat@chromium.org
, Aug 8Labels: -Pri-3 OS-Chrome Pri-2
Owner: wutao@chromium.org
Status: Assigned (was: Available)
Am I correct in thinking that we're storing (at least) two copies of each accelerator now, one in ash/public/cpp/accelerators.cc like this: {true, ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_CONTROL_DOWN, TAKE_SCREENSHOT}, and a second in ash/components/shortcut_viewer/keyboard_shortcut_viewer_metadata.cc like this: {// |categories| {ShortcutCategory::kPopular}, IDS_KSV_DESCRIPTION_TAKE_SCREENSHOT, IDS_KSV_SHORTCUT_ONE_MODIFIER_ONE_KEY, // |accelerator_ids| {{ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_CONTROL_DOWN}}}, ? If that's correct, please find a way to unify this data (e.g. add the KSV data to ash/public/cpp/accelerators.cc instead of storing it separately). These lists are just going to get further out of sync otherwise.