From 6e8ef7550a64272e2ae615724dfbbb50a3010fa2 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Fri, 24 May 2024 17:07:25 +0200 Subject: [PATCH 1/6] Update ShortcutsBlob data Make sure that ShrotcutsBlob data reflects the current location of the user. Previously the class was instantiated with file permalink, but would not update if user would navigate back and choose another file. Changelog: fixed --- .../javascripts/behaviors/shortcuts/shortcuts.js | 5 +++++ .../javascripts/behaviors/shortcuts/shortcuts_blob.js | 11 +++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts.js index da367d92a17a4c..86373b3f4c08fe 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts.js @@ -30,6 +30,7 @@ import { GO_TO_YOUR_REVIEW_REQUESTS, } from './keybindings'; import { disableShortcuts, shouldDisableShortcuts } from './shortcuts_toggle'; +import ShortcutsBlob from './shortcuts_blob'; /** * The key used to save and fetch the local Mousetrap instance @@ -162,6 +163,10 @@ export default class Shortcuts { this.extensions.set(Extension, instance); } + if (instance instanceof ShortcutsBlob) { + instance.setOptions = { ...args[0] }; + } + extensionsCurrentlyLoading.delete(Extension); return instance; } diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index a0bfd337d10a79..9aa86f084d3ff2 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -20,14 +20,21 @@ function eventHasModifierKeys(event) { export default class ShortcutsBlob { constructor(shortcuts, opts) { - const options = { ...defaults, ...opts }; - this.options = options; + this.options = { ...defaults, ...opts }; this.shortcircuitPermalinkButton(); shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, this.moveToFilePermalink.bind(this)); } + set setOptions(newOptions) { + this.options = { ...newOptions }; + } + + get getOptions() { + return this.options; + } + moveToFilePermalink() { const permalink = this.options.fileBlobPermalinkUrl; -- GitLab From 828a7bc09919044a38df6a916fcccb571a53b8bc Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Mon, 27 May 2024 18:41:11 +0200 Subject: [PATCH 2/6] Extract logic out of ShortcutsBlob class Keep ShortcutsBlob class to minimum and have it only add a shortcut. moveToFilePermalink and shortcircuitPermalink Buttons become utility functions, still stored in the same file to be reused in Vue and JS version of Blob. Querying of the permalink button and url is that on the spot, inside utility functions to avoid instantiating a shortcuts class with data that becomes outdated after user navigates to a different file. Changelog: fixed --- .../behaviors/shortcuts/shortcuts.js | 5 -- .../behaviors/shortcuts/shortcuts_blob.js | 76 +++++++------------ .../javascripts/pages/projects/init_blob.js | 11 +-- .../repository/components/blob_controls.vue | 13 +--- 4 files changed, 34 insertions(+), 71 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts.js index 86373b3f4c08fe..da367d92a17a4c 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts.js @@ -30,7 +30,6 @@ import { GO_TO_YOUR_REVIEW_REQUESTS, } from './keybindings'; import { disableShortcuts, shouldDisableShortcuts } from './shortcuts_toggle'; -import ShortcutsBlob from './shortcuts_blob'; /** * The key used to save and fetch the local Mousetrap instance @@ -163,10 +162,6 @@ export default class Shortcuts { this.extensions.set(Extension, instance); } - if (instance instanceof ShortcutsBlob) { - instance.setOptions = { ...args[0] }; - } - extensionsCurrentlyLoading.delete(Extension); return instance; } diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index 9aa86f084d3ff2..31a71c70d0c8b1 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -1,6 +1,5 @@ import { PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; import { - getLocationHash, updateHistory, urlIsDifferent, urlContainsSha, @@ -8,64 +7,45 @@ import { } from '~/lib/utils/url_utility'; import { updateRefPortionOfTitle } from '~/repository/utils/title'; -const defaults = { - fileBlobPermalinkUrl: null, - fileBlobPermalinkUrlElement: null, -}; - function eventHasModifierKeys(event) { // We ignore alt because I don't think alt clicks normally do anything special? return event.ctrlKey || event.metaKey || event.shiftKey; } -export default class ShortcutsBlob { - constructor(shortcuts, opts) { - this.options = { ...defaults, ...opts }; - - this.shortcircuitPermalinkButton(); - - shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, this.moveToFilePermalink.bind(this)); - } +function moveToFilePermalink() { + const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); + const permalink = fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href'); - set setOptions(newOptions) { - this.options = { ...newOptions }; - } + if (permalink) { + if (urlIsDifferent(permalink)) { + updateHistory({ + url: permalink, + title: document.title, + }); + } - get getOptions() { - return this.options; + if (urlContainsSha({ url: permalink })) { + updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); + } } +} - moveToFilePermalink() { - const permalink = this.options.fileBlobPermalinkUrl; - - if (permalink) { - const hash = getLocationHash(); - const hashUrlString = hash ? `#${hash}` : ''; - - if (urlIsDifferent(permalink)) { - updateHistory({ - url: `${permalink}${hashUrlString}`, - title: document.title, - }); - } - - if (urlContainsSha({ url: permalink })) { - updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); - } +export function shortcircuitPermalinkButton() { + const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); + const handleButton = (e) => { + if (!eventHasModifierKeys(e)) { + e.preventDefault(); + moveToFilePermalink(); } - } + }; - shortcircuitPermalinkButton() { - const button = this.options.fileBlobPermalinkUrlElement; - const handleButton = (e) => { - if (!eventHasModifierKeys(e)) { - e.preventDefault(); - this.moveToFilePermalink(); - } - }; + if (fileBlobPermalinkUrlElement) { + fileBlobPermalinkUrlElement.addEventListener('click', handleButton); + } +} - if (button) { - button.addEventListener('click', handleButton); - } +export default class ShortcutsBlob { + constructor(shortcuts) { + shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, () => moveToFilePermalink()); } } diff --git a/app/assets/javascripts/pages/projects/init_blob.js b/app/assets/javascripts/pages/projects/init_blob.js index 6e3e1a35bd27d9..aaf2566085fbbd 100644 --- a/app/assets/javascripts/pages/projects/init_blob.js +++ b/app/assets/javascripts/pages/projects/init_blob.js @@ -1,5 +1,5 @@ import { addShortcutsExtension } from '~/behaviors/shortcuts'; -import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; +import ShortcutsBlob, { shortcircuitPermalinkButton } from '~/behaviors/shortcuts/shortcuts_blob'; import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation'; import BlobForkSuggestion from '~/blob/blob_fork_suggestion'; import BlobLinePermalinkUpdater from '~/blob/blob_line_permalink_updater'; @@ -15,15 +15,10 @@ export default () => { document.querySelectorAll('.js-data-file-blob-permalink-url, .js-blob-blame-link'), ); - const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); - const fileBlobPermalinkUrl = - fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href'); + shortcircuitPermalinkButton(); addShortcutsExtension(ShortcutsNavigation); - addShortcutsExtension(ShortcutsBlob, { - fileBlobPermalinkUrl, - fileBlobPermalinkUrlElement, - }); + addShortcutsExtension(ShortcutsBlob); new BlobForkSuggestion({ openButtons: document.querySelectorAll('.js-edit-blob-link-fork-toggler'), diff --git a/app/assets/javascripts/repository/components/blob_controls.vue b/app/assets/javascripts/repository/components/blob_controls.vue index af7dc9ecc2af66..a7f78436972ae8 100644 --- a/app/assets/javascripts/repository/components/blob_controls.vue +++ b/app/assets/javascripts/repository/components/blob_controls.vue @@ -7,7 +7,7 @@ import initSourcegraph from '~/sourcegraph'; import Shortcuts from '~/behaviors/shortcuts/shortcuts'; import { addShortcutsExtension } from '~/behaviors/shortcuts'; import { shouldDisableShortcuts } from '~/behaviors/shortcuts/shortcuts_toggle'; -import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; +import ShortcutsBlob, { shortcircuitPermalinkButton } from '~/behaviors/shortcuts/shortcuts_blob'; import BlobLinePermalinkUpdater from '~/blob/blob_line_permalink_updater'; import { keysFor, @@ -133,15 +133,8 @@ export default { }, methods: { initShortcuts() { - const fileBlobPermalinkUrlElement = document.querySelector( - '.js-data-file-blob-permalink-url', - ); - const fileBlobPermalinkUrl = - fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href'); - addShortcutsExtension(ShortcutsBlob, { - fileBlobPermalinkUrl, - fileBlobPermalinkUrlElement, - }); + shortcircuitPermalinkButton(); + addShortcutsExtension(ShortcutsBlob); }, initLinksUpdate() { // eslint-disable-next-line no-new -- GitLab From b17adf43ad148ef76419a3a246179291daf363c4 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Wed, 29 May 2024 14:41:01 +0200 Subject: [PATCH 3/6] Apply reviewer suggestions --- .../behaviors/shortcuts/shortcuts_blob.js | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index 31a71c70d0c8b1..5c7f0ebeda2770 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -14,38 +14,37 @@ function eventHasModifierKeys(event) { function moveToFilePermalink() { const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); - const permalink = fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href'); - - if (permalink) { - if (urlIsDifferent(permalink)) { - updateHistory({ - url: permalink, - title: document.title, - }); - } + const permalink = fileBlobPermalinkUrlElement?.getAttribute('href'); - if (urlContainsSha({ url: permalink })) { - updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); - } + if (!permalink) { + return; + } + + if (urlIsDifferent(permalink)) { + updateHistory({ + url: permalink, + title: document.title, + }); + } + + if (urlContainsSha({ url: permalink })) { + updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); } } export function shortcircuitPermalinkButton() { const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); - const handleButton = (e) => { + + fileBlobPermalinkUrlElement?.addEventListener('click', (e) => { if (!eventHasModifierKeys(e)) { e.preventDefault(); moveToFilePermalink(); } - }; - - if (fileBlobPermalinkUrlElement) { - fileBlobPermalinkUrlElement.addEventListener('click', handleButton); - } + }); } export default class ShortcutsBlob { constructor(shortcuts) { - shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, () => moveToFilePermalink()); + shortcuts.add(PROJECT_FILES_GO_TO_PERMALINK, moveToFilePermalink); } } -- GitLab From 8b2976b6a77c55ed543d0cf7021b81f60d3a734a Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Wed, 29 May 2024 14:41:54 +0200 Subject: [PATCH 4/6] Add a test case for key y shortcut Make sure a correct permalink is shown, when user navigates back between different files. Changelog: fixed --- .../features/projects/blobs/shortcuts_blob_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/features/projects/blobs/shortcuts_blob_spec.rb b/spec/features/projects/blobs/shortcuts_blob_spec.rb index 162066540d9d4c..0de9137f9ec70b 100644 --- a/spec/features/projects/blobs/shortcuts_blob_spec.rb +++ b/spec/features/projects/blobs/shortcuts_blob_spec.rb @@ -27,6 +27,20 @@ def visit_blob(fragment = nil) expect(page).to have_current_path(get_absolute_url(project_blob_path(project, tree_join(sha, path))), url: true) end + it 'redirects to permalink of a currently viewed file' do + visit project_path(project) + wait_for_requests + click_link 'VERSION' + wait_for_requests + page.driver.go_back + click_link path + wait_for_requests + + find('body').native.send_key('y') + + expect(page).to have_current_path(get_absolute_url(project_blob_path(project, tree_join(sha, path))), url: true) + end + it 'maintains fragment hash when redirecting' do fragment = "L1" visit_blob(fragment) -- GitLab From a82d0d9573f5b5e1fd9690d4ddc8355fa8f22197 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Wed, 29 May 2024 14:54:33 +0200 Subject: [PATCH 5/6] Keep shortcuts_blob.js for ShortcutsBlob only Utility functions moved to blob/utils.js Changelog: changed --- .../behaviors/shortcuts/shortcuts_blob.js | 44 +----------------- app/assets/javascripts/blob/utils.js | 45 ++++++++++++++++++- .../javascripts/pages/projects/init_blob.js | 3 +- .../repository/components/blob_controls.vue | 3 +- 4 files changed, 49 insertions(+), 46 deletions(-) diff --git a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js index 5c7f0ebeda2770..9f8d7272e5c52e 100644 --- a/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js +++ b/app/assets/javascripts/behaviors/shortcuts/shortcuts_blob.js @@ -1,47 +1,5 @@ import { PROJECT_FILES_GO_TO_PERMALINK } from '~/behaviors/shortcuts/keybindings'; -import { - updateHistory, - urlIsDifferent, - urlContainsSha, - getShaFromUrl, -} from '~/lib/utils/url_utility'; -import { updateRefPortionOfTitle } from '~/repository/utils/title'; - -function eventHasModifierKeys(event) { - // We ignore alt because I don't think alt clicks normally do anything special? - return event.ctrlKey || event.metaKey || event.shiftKey; -} - -function moveToFilePermalink() { - const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); - const permalink = fileBlobPermalinkUrlElement?.getAttribute('href'); - - if (!permalink) { - return; - } - - if (urlIsDifferent(permalink)) { - updateHistory({ - url: permalink, - title: document.title, - }); - } - - if (urlContainsSha({ url: permalink })) { - updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); - } -} - -export function shortcircuitPermalinkButton() { - const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); - - fileBlobPermalinkUrlElement?.addEventListener('click', (e) => { - if (!eventHasModifierKeys(e)) { - e.preventDefault(); - moveToFilePermalink(); - } - }); -} +import { moveToFilePermalink } from '~/blob/utils'; export default class ShortcutsBlob { constructor(shortcuts) { diff --git a/app/assets/javascripts/blob/utils.js b/app/assets/javascripts/blob/utils.js index b2400a4b224736..5a119e71286176 100644 --- a/app/assets/javascripts/blob/utils.js +++ b/app/assets/javascripts/blob/utils.js @@ -1,4 +1,11 @@ -import { getBaseURL } from '~/lib/utils/url_utility'; +import { + getBaseURL, + updateHistory, + urlIsDifferent, + urlContainsSha, + getShaFromUrl, +} from '~/lib/utils/url_utility'; +import { updateRefPortionOfTitle } from '~/repository/utils/title'; const blameLinesPerPage = document.querySelector('.js-per-page')?.dataset?.blamePerPage; @@ -15,4 +22,40 @@ export const getPageSearchString = (path, page) => { return url.search; }; +export function moveToFilePermalink() { + const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); + const permalink = fileBlobPermalinkUrlElement?.getAttribute('href'); + + if (!permalink) { + return; + } + + if (urlIsDifferent(permalink)) { + updateHistory({ + url: permalink, + title: document.title, + }); + } + + if (urlContainsSha({ url: permalink })) { + updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); + } +} + +function eventHasModifierKeys(event) { + // We ignore alt because I don't think alt clicks normally do anything special? + return event.ctrlKey || event.metaKey || event.shiftKey; +} + +export function shortcircuitPermalinkButton() { + const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url'); + + fileBlobPermalinkUrlElement?.addEventListener('click', (e) => { + if (!eventHasModifierKeys(e)) { + e.preventDefault(); + moveToFilePermalink(); + } + }); +} + export default () => ({}); diff --git a/app/assets/javascripts/pages/projects/init_blob.js b/app/assets/javascripts/pages/projects/init_blob.js index aaf2566085fbbd..0cf109c7121068 100644 --- a/app/assets/javascripts/pages/projects/init_blob.js +++ b/app/assets/javascripts/pages/projects/init_blob.js @@ -1,5 +1,6 @@ import { addShortcutsExtension } from '~/behaviors/shortcuts'; -import ShortcutsBlob, { shortcircuitPermalinkButton } from '~/behaviors/shortcuts/shortcuts_blob'; +import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; +import { shortcircuitPermalinkButton } from '~/blob/utils'; import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation'; import BlobForkSuggestion from '~/blob/blob_fork_suggestion'; import BlobLinePermalinkUpdater from '~/blob/blob_line_permalink_updater'; diff --git a/app/assets/javascripts/repository/components/blob_controls.vue b/app/assets/javascripts/repository/components/blob_controls.vue index a7f78436972ae8..5571796cd9227c 100644 --- a/app/assets/javascripts/repository/components/blob_controls.vue +++ b/app/assets/javascripts/repository/components/blob_controls.vue @@ -7,7 +7,8 @@ import initSourcegraph from '~/sourcegraph'; import Shortcuts from '~/behaviors/shortcuts/shortcuts'; import { addShortcutsExtension } from '~/behaviors/shortcuts'; import { shouldDisableShortcuts } from '~/behaviors/shortcuts/shortcuts_toggle'; -import ShortcutsBlob, { shortcircuitPermalinkButton } from '~/behaviors/shortcuts/shortcuts_blob'; +import ShortcutsBlob from '~/behaviors/shortcuts/shortcuts_blob'; +import { shortcircuitPermalinkButton } from '~/blob/utils'; import BlobLinePermalinkUpdater from '~/blob/blob_line_permalink_updater'; import { keysFor, -- GitLab From ecea864119b35bf00d2c133ede5d0687bb9715a7 Mon Sep 17 00:00:00 2001 From: psjakubowska Date: Wed, 5 Jun 2024 14:59:40 +0200 Subject: [PATCH 6/6] Move updating title inside the condition it needs to satisfy anyways --- app/assets/javascripts/blob/utils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/blob/utils.js b/app/assets/javascripts/blob/utils.js index 5a119e71286176..0ecc8be1027211 100644 --- a/app/assets/javascripts/blob/utils.js +++ b/app/assets/javascripts/blob/utils.js @@ -35,10 +35,10 @@ export function moveToFilePermalink() { url: permalink, title: document.title, }); - } - if (urlContainsSha({ url: permalink })) { - updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); + if (urlContainsSha({ url: permalink })) { + updateRefPortionOfTitle(getShaFromUrl({ url: permalink })); + } } } -- GitLab