From 2c03a906de795d148062ac8c24ce2f6987530d1c Mon Sep 17 00:00:00 2001 From: Logan Hanks Date: Fri, 5 Oct 2018 15:56:10 -0700 Subject: [PATCH 1/6] Redesign the keyboard shortcuts system The goal of this redesign is to provide two clear concepts that are treated as separate concerns: 1. Mapping of specific key bindings to a semantic shortcut 2. Mapping of a semantic shortcut to its behavior Previously, there was no notion of a semantic shortcut. This required manual crafting of the help dialog and made it difficult to find an approach to customizing key bindings. Although supporting key binding customization is outside the scope of this change, the path forward is more clear. Currently key bindings are hard-coded during the creation phase of the gr-app element. It's conceivable that a plugin or user settings feature could override these bindings at runtime. Or they might be read from some server-side config and passed down to gr-app via the index.html template. The universe of possible shortcuts is now defined in an enum by keyboard-shortcut-behavior. This is also where all the help text is declared and organized. The keyboard shortcut help dialog is now a generic container that reads the current state of actively bound shortcuts and their help content from keyboard-shortcut-behavior. The set of declared key bindings that are active (and registered with suitable listeners on the body element) is determined by monitoring the attachment and detachment of elements that mix in keyboard-shortcut-behavior and declare a keyBindings() method. This method replaces the keyBindings object property that iron-a11y-keys-behavior looks for. The method should return an object mapping Shortcut enum values to handler names. Elements that implement a shortcut behavior but only conditionally attach to the DOM must depend on a container element to declare the shortcut and pass the event down. An example is SAVE_COMMENT. The binding for this shortcut should be active on change and diff views regardless of whether any comments exist, so that it will be listed in the help dialog. The result of this change produces behavior that is close to previous behavior, but not quite identical. Shortcuts that used to be listed under a "Change list" or "Dashboard" section header now appear as "Actions." The exact ordering of shortcuts has not been preserved, either. Change-Id: Ib3fe44d502a83f7012e30615fbea9da2ab112eb7 (cherry picked from commit 5cde0849d4a865cb63af7ade8ad70ab9c66c668a) --- .../keyboard-shortcut-behavior.html | 593 ++++++++++++++++-- .../keyboard-shortcut-behavior_test.html | 275 ++++++++ .../gr-change-list/gr-change-list.js | 42 +- .../gr-change-list/gr-change-list_test.html | 11 + .../change/gr-change-view/gr-change-view.js | 42 +- .../gr-change-view/gr-change-view_test.html | 16 +- .../change/gr-file-list/gr-file-list.js | 70 ++- .../gr-file-list/gr-file-list_test.html | 28 +- .../gr-key-binding-display.html | 48 ++ .../gr-key-binding-display.js | 36 ++ .../gr-key-binding-display_test.html | 66 ++ .../gr-keyboard-shortcuts-dialog.html | 486 ++------------ .../gr-keyboard-shortcuts-dialog.js | 85 ++- .../gr-keyboard-shortcuts-dialog_test.html | 179 ++++++ .../core/gr-search-bar/gr-search-bar.js | 12 +- .../gr-search-bar/gr-search-bar_test.html | 3 + .../diff/gr-diff-view/gr-diff-view.js | 79 ++- .../diff/gr-diff-view/gr-diff-view_test.html | 31 +- polygerrit-ui/app/elements/gr-app.js | 160 ++++- polygerrit-ui/app/elements/gr-app_test.html | 50 -- polygerrit-ui/app/test/index.html | 2 + 21 files changed, 1631 insertions(+), 683 deletions(-) create mode 100644 polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.html create mode 100644 polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js create mode 100644 polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display_test.html create mode 100644 polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog_test.html diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html index 60eaf1fa64..0a685da2fc 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior.html @@ -14,6 +14,88 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. --> + + @@ -21,10 +103,188 @@ limitations under the License. (function(window) { 'use strict'; + const DOC_ONLY = 'DOC_ONLY'; + const GO_KEY = 'GO_KEY'; + + // The maximum age of a keydown event to be used in a jump navigation. This + // is only for cases when the keyup event is lost. + const GO_KEY_TIMEOUT_MS = 1000; + + const ShortcutSection = { + ACTIONS: 'Actions', + DIFFS: 'Diffs', + EVERYWHERE: 'Everywhere', + FILE_LIST: 'File list', + NAVIGATION: 'Navigation', + REPLY_DIALOG: 'Reply dialog', + }; + + const Shortcut = { + OPEN_SHORTCUT_HELP_DIALOG: 'OPEN_SHORTCUT_HELP_DIALOG', + GO_TO_OPENED_CHANGES: 'GO_TO_OPENED_CHANGES', + GO_TO_MERGED_CHANGES: 'GO_TO_MERGED_CHANGES', + GO_TO_ABANDONED_CHANGES: 'GO_TO_ABANDONED_CHANGES', + + CURSOR_NEXT_CHANGE: 'CURSOR_NEXT_CHANGE', + CURSOR_PREV_CHANGE: 'CURSOR_PREV_CHANGE', + OPEN_CHANGE: 'OPEN_CHANGE', + NEXT_PAGE: 'NEXT_PAGE', + PREV_PAGE: 'PREV_PAGE', + TOGGLE_CHANGE_REVIEWED: 'TOGGLE_CHANGE_REVIEWED', + TOGGLE_CHANGE_STAR: 'TOGGLE_CHANGE_STAR', + REFRESH_CHANGE_LIST: 'REFRESH_CHANGE_LIST', + + OPEN_REPLY_DIALOG: 'OPEN_REPLY_DIALOG', + OPEN_DOWNLOAD_DIALOG: 'OPEN_DOWNLOAD_DIALOG', + EXPAND_ALL_MESSAGES: 'EXPAND_ALL_MESSAGES', + COLLAPSE_ALL_MESSAGES: 'COLLAPSE_ALL_MESSAGES', + UP_TO_DASHBOARD: 'UP_TO_DASHBOARD', + UP_TO_CHANGE: 'UP_TO_CHANGE', + TOGGLE_DIFF_MODE: 'TOGGLE_DIFF_MODE', + REFRESH_CHANGE: 'REFRESH_CHANGE', + + NEXT_LINE: 'NEXT_LINE', + PREV_LINE: 'PREV_LINE', + NEXT_CHUNK: 'NEXT_CHUNK', + PREV_CHUNK: 'PREV_CHUNK', + EXPAND_ALL_DIFF_CONTEXT: 'EXPAND_ALL_DIFF_CONTEXT', + NEXT_COMMENT_THREAD: 'NEXT_COMMENT_THREAD', + PREV_COMMENT_THREAD: 'PREV_COMMENT_THREAD', + EXPAND_ALL_COMMENT_THREADS: 'EXPAND_ALL_COMMENT_THREADS', + COLLAPSE_ALL_COMMENT_THREADS: 'COLLAPSE_ALL_COMMENT_THREADS', + LEFT_PANE: 'LEFT_PANE', + RIGHT_PANE: 'RIGHT_PANE', + TOGGLE_LEFT_PANE: 'TOGGLE_LEFT_PANE', + NEW_COMMENT: 'NEW_COMMENT', + SAVE_COMMENT: 'SAVE_COMMENT', + OPEN_DIFF_PREFS: 'OPEN_DIFF_PREFS', + TOGGLE_DIFF_REVIEWED: 'TOGGLE_DIFF_REVIEWED', + + NEXT_FILE: 'NEXT_FILE', + PREV_FILE: 'PREV_FILE', + NEXT_FILE_WITH_COMMENTS: 'NEXT_FILE_WITH_COMMENTS', + PREV_FILE_WITH_COMMENTS: 'PREV_FILE_WITH_COMMENTS', + CURSOR_NEXT_FILE: 'CURSOR_NEXT_FILE', + CURSOR_PREV_FILE: 'CURSOR_PREV_FILE', + OPEN_FILE: 'OPEN_FILE', + TOGGLE_FILE_REVIEWED: 'TOGGLE_FILE_REVIEWED', + TOGGLE_ALL_INLINE_DIFFS: 'TOGGLE_ALL_INLINE_DIFFS', + TOGGLE_INLINE_DIFF: 'TOGGLE_INLINE_DIFF', + + OPEN_FIRST_FILE: 'OPEN_FIRST_FILE', + OPEN_LAST_FILE: 'OPEN_LAST_FILE', + + SEARCH: 'SEARCH', + SEND_REPLY: 'SEND_REPLY', + }; + + const _help = new Map(); + + function _describe(shortcut, section, text) { + if (!_help.has(section)) { + _help.set(section, []); + } + _help.get(section).push({shortcut, text}); + } + + _describe(Shortcut.SEARCH, ShortcutSection.EVERYWHERE, 'Search'); + _describe(Shortcut.OPEN_SHORTCUT_HELP_DIALOG, ShortcutSection.EVERYWHERE, + 'Show this dialog'); + _describe(Shortcut.GO_TO_OPENED_CHANGES, ShortcutSection.EVERYWHERE, + 'Go to Opened Changes'); + _describe(Shortcut.GO_TO_MERGED_CHANGES, ShortcutSection.EVERYWHERE, + 'Go to Merged Changes'); + _describe(Shortcut.GO_TO_ABANDONED_CHANGES, ShortcutSection.EVERYWHERE, + 'Go to Abandoned Changes'); + + _describe(Shortcut.CURSOR_NEXT_CHANGE, ShortcutSection.ACTIONS, + 'Select next change'); + _describe(Shortcut.CURSOR_PREV_CHANGE, ShortcutSection.ACTIONS, + 'Select previous change'); + _describe(Shortcut.OPEN_CHANGE, ShortcutSection.ACTIONS, + 'Show selected change'); + _describe(Shortcut.NEXT_PAGE, ShortcutSection.ACTIONS, 'Go to next page'); + _describe(Shortcut.PREV_PAGE, ShortcutSection.ACTIONS, 'Go to previous page'); + _describe(Shortcut.OPEN_REPLY_DIALOG, ShortcutSection.ACTIONS, + 'Open reply dialog to publish comments and add reviewers'); + _describe(Shortcut.OPEN_DOWNLOAD_DIALOG, ShortcutSection.ACTIONS, + 'Open download overlay'); + _describe(Shortcut.EXPAND_ALL_MESSAGES, ShortcutSection.ACTIONS, + 'Expand all messages'); + _describe(Shortcut.COLLAPSE_ALL_MESSAGES, ShortcutSection.ACTIONS, + 'Collapse all messages'); + _describe(Shortcut.REFRESH_CHANGE, ShortcutSection.ACTIONS, + 'Reload the change at the latest patch'); + _describe(Shortcut.TOGGLE_CHANGE_REVIEWED, ShortcutSection.ACTIONS, + 'Mark/unmark change as reviewed'); + _describe(Shortcut.TOGGLE_FILE_REVIEWED, ShortcutSection.ACTIONS, + 'Toggle review flag on selected file'); + _describe(Shortcut.REFRESH_CHANGE_LIST, ShortcutSection.ACTIONS, + 'Refresh list of changes'); + _describe(Shortcut.TOGGLE_CHANGE_STAR, ShortcutSection.ACTIONS, + 'Star/unstar change'); + + _describe(Shortcut.NEXT_LINE, ShortcutSection.DIFFS, 'Go to next line'); + _describe(Shortcut.PREV_LINE, ShortcutSection.DIFFS, 'Go to previous line'); + _describe(Shortcut.NEXT_CHUNK, ShortcutSection.DIFFS, + 'Go to next diff chunk'); + _describe(Shortcut.PREV_CHUNK, ShortcutSection.DIFFS, + 'Go to previous diff chunk'); + _describe(Shortcut.EXPAND_ALL_DIFF_CONTEXT, ShortcutSection.DIFFS, + 'Expand all diff context'); + _describe(Shortcut.NEXT_COMMENT_THREAD, ShortcutSection.DIFFS, + 'Go to next comment thread'); + _describe(Shortcut.PREV_COMMENT_THREAD, ShortcutSection.DIFFS, + 'Go to previous comment thread'); + _describe(Shortcut.EXPAND_ALL_COMMENT_THREADS, ShortcutSection.DIFFS, + 'Expand all comment threads'); + _describe(Shortcut.COLLAPSE_ALL_COMMENT_THREADS, ShortcutSection.DIFFS, + 'Collapse all comment threads'); + _describe(Shortcut.LEFT_PANE, ShortcutSection.DIFFS, 'Select left pane'); + _describe(Shortcut.RIGHT_PANE, ShortcutSection.DIFFS, 'Select right pane'); + _describe(Shortcut.TOGGLE_LEFT_PANE, ShortcutSection.DIFFS, + 'Hide/show left diff'); + _describe(Shortcut.NEW_COMMENT, ShortcutSection.DIFFS, 'Draft new comment'); + _describe(Shortcut.SAVE_COMMENT, ShortcutSection.DIFFS, 'Save comment'); + _describe(Shortcut.OPEN_DIFF_PREFS, ShortcutSection.DIFFS, + 'Show diff preferences'); + _describe(Shortcut.TOGGLE_DIFF_REVIEWED, ShortcutSection.DIFFS, + 'Mark/unmark file as reviewed'); + _describe(Shortcut.TOGGLE_DIFF_MODE, ShortcutSection.DIFFS, + 'Toggle unified/side-by-side diff'); + + _describe(Shortcut.NEXT_FILE, ShortcutSection.NAVIGATION, 'Select next file'); + _describe(Shortcut.PREV_FILE, ShortcutSection.NAVIGATION, + 'Select previous file'); + _describe(Shortcut.NEXT_FILE_WITH_COMMENTS, ShortcutSection.NAVIGATION, + 'Select next file that has comments'); + _describe(Shortcut.PREV_FILE_WITH_COMMENTS, ShortcutSection.NAVIGATION, + 'Select previous file that has comments'); + _describe(Shortcut.OPEN_FIRST_FILE, ShortcutSection.NAVIGATION, + 'Show first file'); + _describe(Shortcut.OPEN_LAST_FILE, ShortcutSection.NAVIGATION, + 'Show last file'); + _describe(Shortcut.UP_TO_DASHBOARD, ShortcutSection.NAVIGATION, + 'Up to dashboard'); + _describe(Shortcut.UP_TO_CHANGE, ShortcutSection.NAVIGATION, 'Up to change'); + + _describe(Shortcut.CURSOR_NEXT_FILE, ShortcutSection.FILE_LIST, + 'Select next file'); + _describe(Shortcut.CURSOR_PREV_FILE, ShortcutSection.FILE_LIST, + 'Select previous file'); + _describe(Shortcut.OPEN_FILE, ShortcutSection.FILE_LIST, + 'Go to selected file'); + _describe(Shortcut.TOGGLE_ALL_INLINE_DIFFS, ShortcutSection.FILE_LIST, + 'Show/hide all inline diffs'); + _describe(Shortcut.TOGGLE_INLINE_DIFF, ShortcutSection.FILE_LIST, + 'Show/hide selected inline diff'); + + _describe(Shortcut.SEND_REPLY, ShortcutSection.REPLY_DIALOG, 'Send reply'); + // Must be declared outside behavior implementation to be accessed inside // behavior functions. - /** @return {!Object} */ + /** @return {!(Event|PolymerDomApi|PolymerEventApi)} */ const getKeyboardEvent = function(e) { e = Polymer.dom(e.detail ? e.detail.keyboardEvent : e); // When e is a keyboardEvent, e.event is not null. @@ -32,44 +292,307 @@ limitations under the License. return e; }; + class ShortcutManager { + constructor() { + this.activeHosts = new Map(); + this.bindings = new Map(); + this.listeners = new Set(); + } + + bindShortcut(shortcut, ...bindings) { + this.bindings.set(shortcut, bindings); + } + + getBindingsForShortcut(shortcut) { + return this.bindings.get(shortcut); + } + + attachHost(host) { + if (!host.keyboardShortcuts) { return; } + const shortcuts = host.keyboardShortcuts(); + this.activeHosts.set(host, new Map(Object.entries(shortcuts))); + this.notifyListeners(); + return shortcuts; + } + + detachHost(host) { + if (this.activeHosts.delete(host)) { + this.notifyListeners(); + return true; + } + return false; + } + + addListener(listener) { + this.listeners.add(listener); + listener(this.directoryView()); + } + + removeListener(listener) { + return this.listeners.delete(listener); + } + + activeShortcutsBySection() { + const activeShortcuts = new Set(); + this.activeHosts.forEach(shortcuts => { + shortcuts.forEach((_, shortcut) => activeShortcuts.add(shortcut)); + }); + + const activeShortcutsBySection = new Map(); + _help.forEach((shortcutList, section) => { + shortcutList.forEach(shortcutHelp => { + if (activeShortcuts.has(shortcutHelp.shortcut)) { + if (!activeShortcutsBySection.has(section)) { + activeShortcutsBySection.set(section, []); + } + activeShortcutsBySection.get(section).push(shortcutHelp); + } + }); + }); + return activeShortcutsBySection; + } + + directoryView() { + const view = new Map(); + this.activeShortcutsBySection().forEach((shortcutHelps, section) => { + const sectionView = []; + shortcutHelps.forEach(shortcutHelp => { + const bindingDesc = this.describeBindings(shortcutHelp.shortcut); + if (!bindingDesc) { return; } + this.distributeBindingDesc(bindingDesc).forEach(bindingDesc => { + sectionView.push({ + binding: bindingDesc, + text: shortcutHelp.text, + }); + }); + }); + view.set(section, sectionView); + }); + return view; + } + + distributeBindingDesc(bindingDesc) { + if (bindingDesc.length === 1 || + this.comboSetDisplayWidth(bindingDesc) < 21) { + return [bindingDesc]; + } + // Find the largest prefix of bindings that is under the + // size threshold. + const head = [bindingDesc[0]]; + for (let i = 1; i < bindingDesc.length; i++) { + head.push(bindingDesc[i]); + if (this.comboSetDisplayWidth(head) >= 21) { + head.pop(); + return [head].concat( + this.distributeBindingDesc(bindingDesc.slice(i))); + } + } + } + + comboSetDisplayWidth(bindingDesc) { + const bindingSizer = binding => binding.reduce( + (acc, key) => acc + key.length, 0); + // Width is the sum of strings + (n-1) * 2 to account for the word + // "or" joining them. + return bindingDesc.reduce( + (acc, binding) => acc + bindingSizer(binding), 0) + + 2 * (bindingDesc.length - 1); + } + + describeBindings(shortcut) { + const bindings = this.bindings.get(shortcut); + if (!bindings) { return null; } + if (bindings[0] === GO_KEY) { + return [['g'].concat(bindings.slice(1))]; + } + return bindings + .filter(binding => binding !== DOC_ONLY) + .map(binding => this.describeBinding(binding)); + } + + describeBinding(binding) { + return binding.split(':')[0].split('+').map(part => { + switch (part) { + case 'shift': + return 'Shift'; + case 'meta': + return 'Meta'; + case 'ctrl': + return 'Ctrl'; + case 'enter': + return 'Enter'; + case 'up': + return '↑'; + case 'down': + return '↓'; + case 'left': + return '←'; + case 'right': + return '→'; + default: + return part; + } + }); + } + + notifyListeners() { + const view = this.directoryView(); + this.listeners.forEach(listener => listener(view)); + } + } + + const shortcutManager = new ShortcutManager(); + window.Gerrit = window.Gerrit || {}; /** @polymerBehavior KeyboardShortcutBehavior */ - Gerrit.KeyboardShortcutBehavior = [{ - modifierPressed(e) { - e = getKeyboardEvent(e); - return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey; - }, - - isModifierPressed(e, modifier) { - return getKeyboardEvent(e)[modifier]; - }, - - shouldSuppressKeyboardShortcut(e) { - e = getKeyboardEvent(e); - const tagName = Polymer.dom(e).rootTarget.tagName; - if (tagName === 'INPUT' || tagName === 'TEXTAREA' || - (e.keyCode === 13 && tagName === 'A')) { - // Suppress shortcuts if the key is 'enter' and target is an anchor. - return true; - } - for (let i = 0; e.path && i < e.path.length; i++) { - if (e.path[i].tagName === 'GR-OVERLAY') { return true; } - } - return false; - }, - - // Alias for getKeyboardEvent. - /** @return {!Object} */ - getKeyboardEvent(e) { - return getKeyboardEvent(e); - }, - - getRootTarget(e) { - return Polymer.dom(getKeyboardEvent(e)).rootTarget; - }, - }, + Gerrit.KeyboardShortcutBehavior = [ Polymer.IronA11yKeysBehavior, + { + // Exports for convenience. Note: Closure compiler crashes when + // object-shorthand syntax is used here. + // eslint-disable-next-line object-shorthand + DOC_ONLY: DOC_ONLY, + // eslint-disable-next-line object-shorthand + GO_KEY: GO_KEY, + // eslint-disable-next-line object-shorthand + Shortcut: Shortcut, + + properties: { + _shortcut_go_key_last_pressed: { + type: Number, + value: null, + }, + + _shortcut_go_table: { + type: Array, + value() { return new Map(); }, + }, + }, + + modifierPressed(e) { + e = getKeyboardEvent(e); + return e.altKey || e.ctrlKey || e.metaKey || e.shiftKey; + }, + + isModifierPressed(e, modifier) { + return getKeyboardEvent(e)[modifier]; + }, + + shouldSuppressKeyboardShortcut(e) { + e = getKeyboardEvent(e); + const tagName = Polymer.dom(e).rootTarget.tagName; + if (tagName === 'INPUT' || tagName === 'TEXTAREA' || + (e.keyCode === 13 && tagName === 'A')) { + // Suppress shortcuts if the key is 'enter' and target is an anchor. + return true; + } + for (let i = 0; e.path && i < e.path.length; i++) { + if (e.path[i].tagName === 'GR-OVERLAY') { return true; } + } + return false; + }, + + // Alias for getKeyboardEvent. + /** @return {!(Event|PolymerDomApi|PolymerEventApi)} */ + getKeyboardEvent(e) { + return getKeyboardEvent(e); + }, + + getRootTarget(e) { + return Polymer.dom(getKeyboardEvent(e)).rootTarget; + }, + + bindShortcut(shortcut, ...bindings) { + shortcutManager.bindShortcut(shortcut, ...bindings); + }, + + _addOwnKeyBindings(shortcut, handler) { + const bindings = shortcutManager.getBindingsForShortcut(shortcut); + if (!bindings) { + return; + } + if (bindings[0] === DOC_ONLY) { + return; + } + if (bindings[0] === GO_KEY) { + this._shortcut_go_table.set(bindings[1], handler); + } else { + this.addOwnKeyBinding(bindings.join(' '), handler); + } + }, + + attached() { + const shortcuts = shortcutManager.attachHost(this); + if (!shortcuts) { return; } + + for (const key of Object.keys(shortcuts)) { + this._addOwnKeyBindings(key, shortcuts[key]); + } + + // If any of the shortcuts utilized GO_KEY, then they are handled + // directly by this behavior. + if (this._shortcut_go_table.size > 0) { + this.addOwnKeyBinding('g:keydown', '_handleGoKeyDown'); + this.addOwnKeyBinding('g:keyup', '_handleGoKeyUp'); + this._shortcut_go_table.forEach((handler, key) => { + this.addOwnKeyBinding(key, '_handleGoAction'); + }); + } + }, + + detached() { + if (shortcutManager.detachHost(this)) { + this.removeOwnKeyBindings(); + } + }, + + keyboardShortcuts() { + return {}; + }, + + addKeyboardShortcutDirectoryListener(listener) { + shortcutManager.addListener(listener); + }, + + removeKeyboardShortcutDirectoryListener(listener) { + shortcutManager.removeListener(listener); + }, + + _handleGoKeyDown(e) { + if (this.modifierPressed(e)) { return; } + this._shortcut_go_key_last_pressed = Date.now(); + }, + + _handleGoKeyUp(e) { + this._shortcut_go_key_last_pressed = null; + }, + + _handleGoAction(e) { + if (!this._shortcut_go_key_last_pressed || + (Date.now() - this._shortcut_go_key_last_pressed > + GO_KEY_TIMEOUT_MS) || + !this._shortcut_go_table.has(e.detail.key) || + this.shouldSuppressKeyboardShortcut(e)) { + return; + } + e.preventDefault(); + const handler = this._shortcut_go_table.get(e.detail.key); + this[handler](e); + }, + }, ]; + + Gerrit.KeyboardShortcutBinder = { + DOC_ONLY, + GO_KEY, + Shortcut, + ShortcutManager, + ShortcutSection, + + bindShortcut(shortcut, ...bindings) { + shortcutManager.bindShortcut(shortcut, ...bindings); + }, + }; })(window); diff --git a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html index 04193dde5a..dac90f8606 100644 --- a/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html +++ b/polygerrit-ui/app/behaviors/keyboard-shortcut-behavior/keyboard-shortcut-behavior_test.html @@ -40,6 +40,8 @@ limitations under the License. diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js index ba768d9acd..de97e62507 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list.js @@ -106,17 +106,6 @@ Gerrit.URLEncodingBehavior, ], - keyBindings: { - 'j': '_handleJKey', - 'k': '_handleKKey', - 'n ]': '_handleNKey', - 'o': '_handleOKey', - 'p [': '_handlePKey', - 'r': '_handleRKey', - 'shift+r': '_handleShiftRKey', - 's': '_handleSKey', - }, - listeners: { keydown: '_scopedKeydownHandler', }, @@ -126,6 +115,19 @@ '_computePreferences(account, preferences)', ], + keyboardShortcuts() { + return { + [this.Shortcut.CURSOR_NEXT_CHANGE]: '_nextChange', + [this.Shortcut.CURSOR_PREV_CHANGE]: '_prevChange', + [this.Shortcut.NEXT_PAGE]: '_nextPage', + [this.Shortcut.PREV_PAGE]: '_prevPage', + [this.Shortcut.OPEN_CHANGE]: '_openChange', + [this.Shortcut.TOGGLE_CHANGE_REVIEWED]: '_toggleChangeReviewed', + [this.Shortcut.TOGGLE_CHANGE_STAR]: '_toggleChangeStar', + [this.Shortcut.REFRESH_CHANGE_LIST]: '_refreshChangeList', + }; + }, + /** * Iron-a11y-keys-behavior catches keyboard events globally. Some keyboard * events must be scoped to a component level (e.g. `enter`) in order to not @@ -136,7 +138,7 @@ _scopedKeydownHandler(e) { if (e.keyCode === 13) { // Enter. - this._handleOKey(e); + this._openChange(e); } }, @@ -238,7 +240,7 @@ return account._account_id === change.assignee._account_id; }, - _handleJKey(e) { + _nextChange(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e)) { return; } @@ -246,7 +248,7 @@ this.$.cursor.next(); }, - _handleKKey(e) { + _prevChange(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e)) { return; } @@ -254,7 +256,7 @@ this.$.cursor.previous(); }, - _handleOKey(e) { + _openChange(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e)) { return; } @@ -262,7 +264,7 @@ Gerrit.Nav.navigateToChange(this._changeForIndex(this.selectedIndex)); }, - _handleNKey(e) { + _nextPage(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e) && !this.isModifierPressed(e, 'shiftKey')) { return; @@ -272,7 +274,7 @@ this.fire('next-page'); }, - _handlePKey(e) { + _prevPage(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e) && !this.isModifierPressed(e, 'shiftKey')) { return; @@ -282,7 +284,7 @@ this.fire('previous-page'); }, - _handleRKey(e) { + _toggleChangeReviewed(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e)) { return; } @@ -300,7 +302,7 @@ changeEl.toggleReviewed(); }, - _handleShiftRKey(e) { + _refreshChangeList(e) { if (this.shouldSuppressKeyboardShortcut(e)) { return; } e.preventDefault(); @@ -311,7 +313,7 @@ window.location.reload(); }, - _handleSKey(e) { + _toggleChangeStar(e) { if (this.shouldSuppressKeyboardShortcut(e) || this.modifierPressed(e)) { return; } diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html index bb904b5f1d..d20d40a9b3 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html +++ b/polygerrit-ui/app/elements/change-list/gr-change-list/gr-change-list_test.html @@ -42,6 +42,17 @@ limitations under the License. + diff --git a/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js new file mode 100644 index 0000000000..89d1091086 --- /dev/null +++ b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display.js @@ -0,0 +1,36 @@ +/** + * @license + * Copyright (C) 2018 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +(function() { + 'use strict'; + + Polymer({ + is: 'gr-key-binding-display', + + properties: { + /** @type {Array} */ + binding: Array, + }, + + _computeModifiers(binding) { + return binding.slice(0, binding.length - 1); + }, + + _computeKey(binding) { + return binding[binding.length - 1]; + }, + }); +})(); diff --git a/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display_test.html b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display_test.html new file mode 100644 index 0000000000..0361d76d93 --- /dev/null +++ b/polygerrit-ui/app/elements/core/gr-key-binding-display/gr-key-binding-display_test.html @@ -0,0 +1,66 @@ + + + +gr-key-binding-display + + + + + + + + + + + + + diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html index 9fda898f83..e3552ccd25 100644 --- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html +++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.html @@ -16,7 +16,9 @@ limitations under the License. --> + + @@ -54,15 +56,6 @@ limitations under the License. font-weight: var(--font-weight-bold); padding-top: 1em; } - .key { - background-color: var(--chip-background-color); - border: 1px solid var(--border-color); - border-radius: 3px; - display: inline-block; - font-weight: var(--font-weight-bold); - padding: .1em .5em; - text-align: center; - } .modifier { font-weight: normal; } @@ -74,449 +67,42 @@ limitations under the License.
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Everywhere
/Search
?Show this dialog
- g - o - Go to Opened Changes
- g - m - Go to Merged Changes
- g - a - Go to Abandoned Changes
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +
+
diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.js b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.js index e5dd019eec..5b29972e40 100644 --- a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.js +++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog.js @@ -17,6 +17,8 @@ (function() { 'use strict'; + const {ShortcutSection} = window.Gerrit.KeyboardShortcutBinder; + Polymer({ is: 'gr-keyboard-shortcuts-dialog', @@ -27,20 +29,97 @@ */ properties: { - view: String, + _left: Array, + _right: Array, + + _propertyBySection: { + type: Object, + value() { + return { + [ShortcutSection.EVERYWHERE]: '_everywhere', + [ShortcutSection.NAVIGATION]: '_navigation', + [ShortcutSection.DASHBOARD]: '_dashboard', + [ShortcutSection.CHANGE_LIST]: '_changeList', + [ShortcutSection.ACTIONS]: '_actions', + [ShortcutSection.REPLY_DIALOG]: '_replyDialog', + [ShortcutSection.FILE_LIST]: '_fileList', + [ShortcutSection.DIFFS]: '_diffs', + }; + }, + }, }, + behaviors: [ + Gerrit.KeyboardShortcutBehavior, + ], + hostAttributes: { role: 'dialog', }, - _computeInView(currentView, view) { - return view === currentView; + attached() { + this.addKeyboardShortcutDirectoryListener( + this._onDirectoryUpdated.bind(this)); + }, + + detached() { + this.removeKeyboardShortcutDirectoryListener( + this._onDirectoryUpdated.bind(this)); }, _handleCloseTap(e) { e.preventDefault(); this.fire('close', null, {bubbles: false}); }, + + _onDirectoryUpdated(directory) { + const left = []; + const right = []; + + if (directory.has(ShortcutSection.EVERYWHERE)) { + left.push({ + section: ShortcutSection.EVERYWHERE, + shortcuts: directory.get(ShortcutSection.EVERYWHERE), + }); + } + + if (directory.has(ShortcutSection.NAVIGATION)) { + left.push({ + section: ShortcutSection.NAVIGATION, + shortcuts: directory.get(ShortcutSection.NAVIGATION), + }); + } + + if (directory.has(ShortcutSection.ACTIONS)) { + right.push({ + section: ShortcutSection.ACTIONS, + shortcuts: directory.get(ShortcutSection.ACTIONS), + }); + } + + if (directory.has(ShortcutSection.REPLY_DIALOG)) { + right.push({ + section: ShortcutSection.REPLY_DIALOG, + shortcuts: directory.get(ShortcutSection.REPLY_DIALOG), + }); + } + + if (directory.has(ShortcutSection.FILE_LIST)) { + right.push({ + section: ShortcutSection.FILE_LIST, + shortcuts: directory.get(ShortcutSection.FILE_LIST), + }); + } + + if (directory.has(ShortcutSection.DIFFS)) { + right.push({ + section: ShortcutSection.DIFFS, + shortcuts: directory.get(ShortcutSection.DIFFS), + }); + } + + this.set('_left', left); + this.set('_right', right); + }, }); })(); diff --git a/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog_test.html b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog_test.html new file mode 100644 index 0000000000..50579dda40 --- /dev/null +++ b/polygerrit-ui/app/elements/core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog_test.html @@ -0,0 +1,179 @@ + + + +gr-key-binding-display + + + + + + + + + + + + + + diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js index 1513a7f343..a81526ceb1 100644 --- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js +++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar.js @@ -110,10 +110,6 @@ Gerrit.URLEncodingBehavior, ], - keyBindings: { - '/': '_handleForwardSlashKey', - }, - properties: { value: { type: String, @@ -156,6 +152,12 @@ }, }, + keyboardShortcuts() { + return { + [this.Shortcut.SEARCH]: '_handleSearch', + }; + }, + _valueChanged(value) { this._inputVal = value; }, @@ -274,7 +276,7 @@ }); }, - _handleForwardSlashKey(e) { + _handleSearch(e) { const keyboardEvent = this.getKeyboardEvent(e); if (this.shouldSuppressKeyboardShortcut(e) || (this.modifierPressed(e) && !keyboardEvent.shiftKey)) { return; } diff --git a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html index 9a7023e4fa..93e0e30739 100644 --- a/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html +++ b/polygerrit-ui/app/elements/core/gr-search-bar/gr-search-bar_test.html @@ -37,6 +37,9 @@ limitations under the License. diff --git a/polygerrit-ui/app/test/index.html b/polygerrit-ui/app/test/index.html index 08324597f8..5b9ae1576e 100644 --- a/polygerrit-ui/app/test/index.html +++ b/polygerrit-ui/app/test/index.html @@ -92,6 +92,8 @@ limitations under the License. 'core/gr-account-dropdown/gr-account-dropdown_test.html', 'core/gr-error-dialog/gr-error-dialog_test.html', 'core/gr-error-manager/gr-error-manager_test.html', + 'core/gr-key-binding-display/gr-key-binding-display_test.html', + 'core/gr-keyboard-shortcuts-dialog/gr-keyboard-shortcuts-dialog_test.html', 'core/gr-main-header/gr-main-header_test.html', 'core/gr-navigation/gr-navigation_test.html', 'core/gr-reporting/gr-jank-detector_test.html', From 8d71be75c74b1368cbd0d1f2b395a0134a938ae8 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 25 Oct 2018 11:06:46 +0900 Subject: [PATCH 2/6] Set version to 2.15.7-SNAPSHOT Change-Id: I6eb7ca1e0bc538506c3fa632ff20275f2f6b13d0 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index c54feb12b3..ac5644074b 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.6 + 2.15.7-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 5b8e3f45f2..c858293ac1 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.6 + 2.15.7-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 27f44beafb..7d8005aeef 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.6 + 2.15.7-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 8045b22160..9ae44e0102 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.6 + 2.15.7-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index f9ec3c895e..8994ee72c5 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.6 + 2.15.7-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index a0d3910203..22a193f7cb 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.6" +GERRIT_VERSION = "2.15.7-SNAPSHOT" From 36d32d9d6cf82863c483b1b90eb604f24945626a Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 23 Oct 2018 14:40:09 +0900 Subject: [PATCH 3/6] AccountManager: Fix logic for updating display name on authentication The actual update was being guarded by a condition that would only be true if the realm _does not_ allow the display name to be updated. Change-Id: Icc284542d69294f3c931bfa07a6df8866952560b --- .../acceptance/api/accounts/AccountIT.java | 16 ++++++++++++++++ .../gerrit/server/account/AccountManager.java | 12 +++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 1fb25c7ef9..31c0ecfbe8 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -86,7 +86,9 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.Sequences; import com.google.gerrit.server.account.AccountConfig; +import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AccountsUpdate; +import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.Emails; import com.google.gerrit.server.account.WatchConfig; import com.google.gerrit.server.account.WatchConfig.NotifyType; @@ -168,6 +170,8 @@ public class AccountIT extends AbstractDaemonTest { @Inject protected Emails emails; + @Inject private AccountManager accountManager; + private AccountIndexedCounter accountIndexedCounter; private RegistrationHandle accountIndexEventCounterHandle; private RefUpdateCounter refUpdateCounter; @@ -1820,6 +1824,18 @@ public class AccountIT extends AbstractDaemonTest { assertGroups(newUser, ImmutableList.of(group)); } + @Test + public void updateDisplayName() throws Exception { + String name = name("test"); + gApi.accounts().create(name); + AuthRequest who = AuthRequest.forUser(name); + accountManager.authenticate(who); + assertThat(gApi.accounts().id(name).get().name).isEqualTo(name); + who.setDisplayName("Something Else"); + accountManager.authenticate(who); + assertThat(gApi.accounts().id(name).get().name).isEqualTo("Something Else"); + } + private void assertGroups(String user, List expected) throws Exception { List actual = gApi.accounts().id(user).getGroups().stream().map(g -> g.name).collect(toList()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java index 2967208281..41b0c9fe9b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java @@ -235,10 +235,16 @@ public class AccountManager { extId, ExternalId.create(extId.key(), extId.accountId(), newEmail, extId.password())); } - if (!realm.allowsEdit(AccountFieldName.FULL_NAME) - && !Strings.isNullOrEmpty(who.getDisplayName()) + if (!Strings.isNullOrEmpty(who.getDisplayName()) && !eq(user.getAccount().getFullName(), who.getDisplayName())) { - accountUpdates.add(a -> a.setFullName(who.getDisplayName())); + if (realm.allowsEdit(AccountFieldName.FULL_NAME)) { + accountUpdates.add(a -> a.setFullName(who.getDisplayName())); + } else { + log.warn( + "Not changing already set display name '{}' to '{}'", + user.getAccount().getFullName(), + who.getDisplayName()); + } } if (!realm.allowsEdit(AccountFieldName.USER_NAME) From 779680dda51d097d51a500a10f48d0d73a1d8470 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 19 Oct 2018 10:18:19 +0200 Subject: [PATCH 4/6] GetPreferences: Don't return unsupported download scheme In the preferences users can set their preferred download scheme. When setting the download scheme it is validated that the download scheme is supported. A download scheme is supported if there is a plugin that provides a DownloadScheme implementation for it that is enabled. However this check does not guarantee that the download scheme that a user has set in the preferences is always supported. It can happen that a user sets a supported download scheme that later becomes unsupported because the plugin is uninstalled or the download scheme is set to disabled. In this case we don't want to return the unspported download scheme to callers of the GetPreferences REST endpoint. E.g. if the caller tries to use the current values for a SetPreferences call this would be rejected due to the unsupported download scheme. Bug: Issue 9857 Change-Id: Ic8c0313efa9fa9c22f3a6f71c1b34e979b200875 Signed-off-by: Edwin Kempin --- .../restapi/account/GetPreferences.java | 37 ++++++++-- .../api/accounts/GeneralPreferencesIT.java | 70 +++++++++++++++++++ 2 files changed, 102 insertions(+), 5 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/account/GetPreferences.java b/java/com/google/gerrit/server/restapi/account/GetPreferences.java index a185898486..3d2064232f 100644 --- a/java/com/google/gerrit/server/restapi/account/GetPreferences.java +++ b/java/com/google/gerrit/server/restapi/account/GetPreferences.java @@ -15,6 +15,9 @@ package com.google.gerrit.server.restapi.account; import com.google.gerrit.extensions.client.GeneralPreferencesInfo; +import com.google.gerrit.extensions.config.DownloadScheme; +import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.extensions.registration.Extension; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -36,13 +39,18 @@ public class GetPreferences implements RestReadView { private final Provider self; private final PermissionBackend permissionBackend; private final AccountCache accountCache; + private final DynamicMap downloadSchemes; @Inject GetPreferences( - Provider self, PermissionBackend permissionBackend, AccountCache accountCache) { + Provider self, + PermissionBackend permissionBackend, + AccountCache accountCache, + DynamicMap downloadSchemes) { this.self = self; this.permissionBackend = permissionBackend; this.accountCache = accountCache; + this.downloadSchemes = downloadSchemes; } @Override @@ -53,9 +61,28 @@ public class GetPreferences implements RestReadView { } Account.Id id = rsrc.getUser().getAccountId(); - return accountCache - .get(id) - .map(AccountState::getGeneralPreferences) - .orElseThrow(() -> new ResourceNotFoundException(IdString.fromDecoded(id.toString()))); + GeneralPreferencesInfo preferencesInfo = + accountCache + .get(id) + .map(AccountState::getGeneralPreferences) + .orElseThrow(() -> new ResourceNotFoundException(IdString.fromDecoded(id.toString()))); + return unsetDownloadSchemeIfUnsupported(preferencesInfo); + } + + private GeneralPreferencesInfo unsetDownloadSchemeIfUnsupported( + GeneralPreferencesInfo preferencesInfo) { + if (preferencesInfo.downloadScheme == null) { + return preferencesInfo; + } + + for (Extension e : downloadSchemes) { + if (e.getExportName().equals(preferencesInfo.downloadScheme) + && e.getProvider().get().isEnabled()) { + return preferencesInfo; + } + } + + preferencesInfo.downloadScheme = null; + return preferencesInfo; } } diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java index 946e15c4e0..24040a4136 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java @@ -30,7 +30,13 @@ import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.ReviewCategoryStrategy; import com.google.gerrit.extensions.client.GeneralPreferencesInfo.TimeFormat; import com.google.gerrit.extensions.client.MenuItem; +import com.google.gerrit.extensions.config.DownloadScheme; +import com.google.gerrit.extensions.registration.DynamicMap; +import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl; +import com.google.gerrit.extensions.registration.RegistrationHandle; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.inject.Inject; +import com.google.inject.util.Providers; import java.util.ArrayList; import java.util.HashMap; import org.junit.Before; @@ -38,6 +44,8 @@ import org.junit.Test; @NoHttpd public class GeneralPreferencesIT extends AbstractDaemonTest { + @Inject private DynamicMap downloadSchemes; + private TestAccount user42; @Before @@ -177,4 +185,66 @@ public class GeneralPreferencesIT extends AbstractDaemonTest { GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).setPreferences(i); assertThat(o.my).containsExactly(new MenuItem("name", "url", "_blank", "id")); } + + @Test + public void rejectUnsupportedDownloadScheme() throws Exception { + GeneralPreferencesInfo i = GeneralPreferencesInfo.defaults(); + i.downloadScheme = "foo"; + + exception.expect(BadRequestException.class); + exception.expectMessage("Unsupported download scheme: " + i.downloadScheme); + gApi.accounts().id(user42.getId().toString()).setPreferences(i); + } + + @Test + public void setDownloadScheme() throws Exception { + String schemeName = "foo"; + RegistrationHandle registrationHandle = + ((PrivateInternals_DynamicMapImpl) downloadSchemes) + .put("myPlugin", schemeName, Providers.of(new TestDownloadScheme())); + try { + GeneralPreferencesInfo i = GeneralPreferencesInfo.defaults(); + i.downloadScheme = schemeName; + + GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).setPreferences(i); + assertThat(o.downloadScheme).isEqualTo(schemeName); + + o = gApi.accounts().id(user42.getId().toString()).getPreferences(); + assertThat(o.downloadScheme).isEqualTo(schemeName); + } finally { + registrationHandle.remove(); + } + } + + @Test + public void unsupportedDownloadSchemeIsNotReturned() throws Exception { + // Set a download scheme and unregister the plugin that provides this download scheme so that it + // becomes unsupported. + setDownloadScheme(); + + GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).getPreferences(); + assertThat(o.downloadScheme).isNull(); + } + + private static class TestDownloadScheme extends DownloadScheme { + @Override + public String getUrl(String project) { + return "http://foo/" + project; + } + + @Override + public boolean isAuthRequired() { + return false; + } + + @Override + public boolean isAuthSupported() { + return false; + } + + @Override + public boolean isEnabled() { + return true; + } + } } From 063bdc632d3bc523036fb62029ac37d4cffcc458 Mon Sep 17 00:00:00 2001 From: viktard Date: Mon, 22 Oct 2018 14:39:46 -0700 Subject: [PATCH 5/6] Update polygerrit_plugin() build rule Add support for .js based plugins, custom plugin_name to prevent conflicts with server-side plugins with the same name. Here's an example: https://chromium-review.googlesource.com/c/infra/gerrit-plugins/landingwidget/+/1295049 So what we want to achieve is to have: gerrit_plugin( name = "landingwidget", [...] and polygerrit_plugin( name = "landingwidget_ui", plugin_name = "landingwidget", [...] and produce the following final artifacts: landingwidget.jar landingwidget.js Change-Id: Ic9c68809cbaaf4650301d98e56d7c1c76e008251 (cherry picked from commit 3c1139ee689ae0df4f99a6b6fd7575c994490c93) --- tools/bzl/js.bzl | 76 +++++++++++++++++++++++++++--------------------- 1 file changed, 43 insertions(+), 33 deletions(-) diff --git a/tools/bzl/js.bzl b/tools/bzl/js.bzl index 0997bcb8df..23d1ccd516 100644 --- a/tools/bzl/js.bzl +++ b/tools/bzl/js.bzl @@ -428,7 +428,7 @@ def bundle_assets(*args, **kwargs): """Combine html, js, css files and optionally split into js and html bundles.""" _bundle_rule(*args, pkg = native.package_name(), **kwargs) -def polygerrit_plugin(name, app, srcs = [], assets = None, **kwargs): +def polygerrit_plugin(name, app, srcs = [], assets = None, plugin_name = None, **kwargs): """Bundles plugin dependencies for deployment. This rule bundles all Polymer elements and JS dependencies into .html and .js files. @@ -436,19 +436,39 @@ def polygerrit_plugin(name, app, srcs = [], assets = None, **kwargs): Output of this rule is a FileSet with "${name}_fs", with deploy artifacts in "plugins/${name}/static". Args: - name: String, plugin name. + name: String, rule name. app: String, the main or root source file. assets: Fileset, additional files to be used by plugin in runtime, exported to "plugins/${name}/static". - srcs: Source files required for combining. + plugin_name: String, plugin name. ${name} is used if not provided. """ + if not plugin_name: + plugin_name = name - # Combines all .js and .html files into foo_combined.js and foo_combined.html - _bundle_rule( - name = name + "_combined", - app = app, - srcs = srcs if app in srcs else srcs + [app], - pkg = native.package_name(), - **kwargs + html_plugin = app.endswith(".html") + srcs = srcs if app in srcs else srcs + [app] + + if html_plugin: + # Combines all .js and .html files into foo_combined.js and foo_combined.html + _bundle_rule( + name = name + "_combined", + app = app, + srcs = srcs, + pkg = native.package_name(), + **kwargs + ) + js_srcs = [name + "_combined.js"] + else: + js_srcs = srcs + + closure_js_library( + name = name + "_closure_lib", + srcs = js_srcs, + convention = "GOOGLE", + no_closure_library = True, + deps = [ + "//lib/polymer_externs:polymer_closure", + "//polygerrit-ui/app/externs:plugin", + ], ) closure_js_binary( @@ -464,37 +484,27 @@ def polygerrit_plugin(name, app, srcs = [], assets = None, **kwargs): ], ) - closure_js_library( - name = name + "_closure_lib", - srcs = [name + "_combined.js"], - convention = "GOOGLE", - no_closure_library = True, - deps = [ - "//lib/polymer_externs:polymer_closure", - "//polygerrit-ui/app/externs:plugin", - ], - ) - - native.genrule( - name = name + "_rename_html", - srcs = [name + "_combined.html"], - outs = [name + ".html"], - cmd = "sed 's/