Skip to content

VIM-like Navigation #7336

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

Open
wants to merge 20 commits into
base: development
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ const KeyboardShortcuts = {
HISTORY_BACKWARD_ALT_MAC: 'cmd+[',
HISTORY_FORWARD_ALT_MAC: 'cmd+]',
FULLSCREEN: 'f11',
VIM_ENABLED_FULLSCREEN: 'f10',
NAVIGATE_TO_SETTINGS: 'ctrl+,',
NAVIGATE_TO_HISTORY: 'ctrl+H',
NAVIGATE_TO_HISTORY_MAC: 'cmd+Y',
Expand All @@ -165,7 +166,11 @@ const KeyboardShortcuts = {
RESET_ZOOM: 'ctrl+0',
ZOOM_IN: 'ctrl+plus',
ZOOM_OUT: 'ctrl+-'

},
VIM: {
SHOW_VIM_WAYPOINTS: 'w',
SCROLL_DOWN: 'j',
SCROLL_UP: 'k',
},
SITUATIONAL: {
REFRESH: 'r',
Expand All @@ -188,6 +193,7 @@ const KeyboardShortcuts = {
PLAYBACK: {
PLAY: 'k',
LARGE_REWIND: 'j',
VIM_ENABLED_LARGE_REWIND: 'h',
LARGE_FAST_FORWARD: 'l',
SMALL_REWIND: 'arrowleft',
SMALL_FAST_FORWARD: 'arrowright',
Expand Down
50 changes: 48 additions & 2 deletions src/renderer/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ export default defineComponent({
isKeyboardShortcutPromptShown: function () {
return this.$store.getters.getIsKeyboardShortcutPromptShown
},
areVimWaypointsShown: function () {
return this.$store.getters.getAreVimWaypointsShown
},
showAddToPlaylistPrompt: function () {
return this.$store.getters.getShowAddToPlaylistPrompt
},
Expand Down Expand Up @@ -156,6 +159,10 @@ export default defineComponent({

openDeepLinksInNewWindow: function () {
return this.$store.getters.getOpenDeepLinksInNewWindow
},

enableVimNavigation: function () {
return this.$store.getters.getEnableVimNavigation
}
},
watch: {
Expand Down Expand Up @@ -344,20 +351,59 @@ export default defineComponent({
},

activateKeyboardShortcuts: function () {
if (this.enableVimNavigation) {
Copy link
Member

Choose a reason for hiding this comment

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

Currently one has to restart the app after the setting is enabled and disabled, please change this so the listeners are added and removed when the setting is changed. Also keep in mind that anything that is still on screen such as the way points have to be cleaned up when the setting is disabled.

['click', 'scroll'].forEach((eventName) =>
document.addEventListener(eventName, (e) => {
this.$store.commit('setAreVimWaypointsShown', { key: 'Esc' })
})
)
document.addEventListener('keydown', (e) => {
// Only handle if not in an input field and not in waypoint mode
if (e.target.tagName !== 'INPUT' && !this.areVimWaypointsShown.selector.length) {
if (e.key === 'j') {
// If the 'J' key is pressed
window.scrollBy({
top: 30,
behavior: 'auto'
})
} else if (e.key === 'k') {
// If the 'K' key is pressed
Comment on lines +363 to +370
Copy link
Member

Choose a reason for hiding this comment

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

This code is self-explanatory, so these code comments don't add any value.

Suggested change
if (e.key === 'j') {
// If the 'J' key is pressed
window.scrollBy({
top: 30,
behavior: 'auto'
})
} else if (e.key === 'k') {
// If the 'K' key is pressed
if (e.key === 'j') {
window.scrollBy({
top: 30,
behavior: 'auto'
})
} else if (e.key === 'k') {

window.scrollBy({
top: -30,
behavior: 'auto'
})
}
}
})
}
document.addEventListener('keydown', this.handleKeyboardShortcuts)
document.addEventListener('mousedown', () => {
this.hideOutlines()
})
},

handleKeyboardShortcuts: function (event) {
// If this.areVimWaypointsShown.selector starts with an f it means
// the user is in nav mode so capture any input and pass it to
// setAreVimWaypointsShown
if (this.areVimWaypointsShown.selector[0] === 'w' && !event.ctrlKey && this.enableVimNavigation) {
Copy link
Member

Choose a reason for hiding this comment

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

Please reorder the checks (not just in this if) so that the check for the setting is first. For people that have the setting enabled it won't change anything, but for people that have it disabled it skips having to do unnecessary checks.

this.$store.commit('setAreVimWaypointsShown', { key: event.key })
return
}
// ignore user typing in HTML `input` elements
if (event.shiftKey && event.key === '?' && event.target.tagName !== 'INPUT') {
this.$store.commit('setIsKeyboardShortcutPromptShown', !this.isKeyboardShortcutPromptShown)
}

if (event.key === 'Tab') {
this.showOutlines()
switch (event.key) {
case 'w':
if (event.target.tagName !== 'INPUT' && this.enableVimNavigation && !event.ctrlKey) {
this.$store.commit('setAreVimWaypointsShown', { key: event.key })
}
break
case 'Tab':
this.showOutlines()
break
}
},

Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

To avoid confusing users, the VIM specific shortcuts should be hidden in the popup when the setting is disabled.

Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ const situationalAppShortcuts = computed(() =>
getLocalizedShortcutNamesAndValues(KeyboardShortcuts.APP.SITUATIONAL)
)

const VimAppShortcuts = computed(() =>
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the naming conventions that are used in the rest of the code base (remember to rename the references to this variable too).

Suggested change
const VimAppShortcuts = computed(() =>
const vimAppShortcuts = computed(() =>

getLocalizedShortcutNamesAndValues(KeyboardShortcuts.APP.VIM)
)

const primarySections = computed(() => [
[
{
Expand All @@ -101,14 +105,24 @@ const primarySections = computed(() => [
title: t('KeyboardShortcutPrompt.Sections.App.Situational'),
shortcutDictionary: situationalAppShortcuts.value
}
],
[
{
title: t('KeyboardShortcutPrompt.Sections.App.VIM'),
shortcutDictionary: VimAppShortcuts.value
}
]
])

const isMac = process.platform === 'darwin'
const enableVimNavigation = computed(() => store.getters.getEnableVimNavigation)

const localizedShortcutNameToShortcutsMappings = computed(() => {
return [
[t('KeyboardShortcutPrompt.Show Keyboard Shortcuts'), ['SHOW_SHORTCUTS']],
[t('KeyboardShortcutPrompt.Show Vim Waypoints'), ['SHOW_VIM_WAYPOINTS']],
[t('KeyboardShortcutPrompt.Scroll Down'), ['SCROLL_DOWN']],
[t('KeyboardShortcutPrompt.Scroll Up'), ['SCROLL_UP']],
[t('KeyboardShortcutPrompt.History Backward'), [
'HISTORY_BACKWARD',
...isMac ? ['HISTORY_BACKWARD_ALT_MAC'] : [],
Expand Down Expand Up @@ -147,8 +161,7 @@ const localizedShortcutNameToShortcutsMappings = computed(() => {
[t('KeyboardShortcutPrompt.Take Screenshot'), ['TAKE_SCREENSHOT']],
[t('KeyboardShortcutPrompt.Stats'), ['STATS']],

[t('KeyboardShortcutPrompt.Play'), ['PLAY']],
[t('KeyboardShortcutPrompt.Large Rewind'), ['LARGE_REWIND']],
[t('KeyboardShortcutPrompt.Large Rewind'), [enableVimNavigation.value ? 'VIM_ENABLED_LARGE_REWIND' : 'LARGE_REWIND']],
[t('KeyboardShortcutPrompt.Large Fast Forward'), ['LARGE_FAST_FORWARD']],
[t('KeyboardShortcutPrompt.Small Rewind'), ['SMALL_REWIND']],
[t('KeyboardShortcutPrompt.Small Fast Forward'), ['SMALL_FAST_FORWARD']],
Expand Down
7 changes: 6 additions & 1 deletion src/renderer/components/ProxySettings/ProxySettings.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@
margin-block-end: 1rem;
}

.proxy-warning {
.proxy-warning,
.vim-warning {
padding-block: 12px;
padding-inline: 4%;
background-color: var(--accent-color);
color: var(--text-with-accent-color);
font-weight: bold;
}

.vim-warning {
margin-block: 1px;
}

Comment on lines +5 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add CSS to the proxy settings, when your changes are in the general settings?

.warning-icon {
font-size: large;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2061,10 +2061,27 @@ export default defineComponent({
seekBySeconds(dist, true)
}

const areVimWaypointsShown = computed(() => {
return store.getters.getAreVimWaypointsShown
})

const vimEnabled = computed(() => {
return store.getters.getEnableVimNavigation
})

/**
* @param {KeyboardEvent} event
*/
function keyboardShortcutHandler(event) {
const passToVim =
// If vim keys are enabled, ctrl is not held down, and the key is not f, j, or k.
(vimEnabled.value && !event.ctrlKey && ['w', 'j', 'k'].includes(event.key)) || // OR
// The waypoints are already on the screen
((areVimWaypointsShown.value.selector || []).length && areVimWaypointsShown.value.selector[0] === 'w')

if (passToVim) {
return
}
if (!player || !hasLoaded.value) {
return
}
Expand Down Expand Up @@ -2105,12 +2122,12 @@ export default defineComponent({
switch (event.key.toLowerCase()) {
case ' ':
case 'spacebar': // older browsers might return spacebar instead of a space character
case KeyboardShortcuts.VIDEO_PLAYER.PLAYBACK.PLAY:
case KeyboardShortcuts.VIDEO_PLAYER.PLAYBACK[vimEnabled.value ? 'VIM_ENABLED_PLAY' : 'PLAY']:
// Toggle Play/Pause
event.preventDefault()
video_.paused ? video_.play() : video_.pause()
break
case KeyboardShortcuts.VIDEO_PLAYER.PLAYBACK.LARGE_REWIND:
case KeyboardShortcuts.VIDEO_PLAYER.PLAYBACK[vimEnabled.value ? 'VIM_ENABLED_LARGE_REWIND' : 'LARGE_REWIND']:
// Rewind by 2x the time-skip interval (in seconds)
event.preventDefault()
seekBySeconds(-defaultSkipInterval.value * player.getPlaybackRate() * 2)
Expand All @@ -2130,7 +2147,7 @@ export default defineComponent({
event.preventDefault()
changePlayBackRate(videoPlaybackRateInterval.value)
break
case KeyboardShortcuts.VIDEO_PLAYER.GENERAL.FULLSCREEN:
case KeyboardShortcuts.VIDEO_PLAYER.GENERAL['FULLSCREEN']:
Copy link
Member

Choose a reason for hiding this comment

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

Revert

// Toggle full screen
event.preventDefault()
ui.getControls().toggleFullScreen()
Expand Down
5 changes: 5 additions & 0 deletions src/renderer/components/general-settings/general-settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ export default defineComponent({

openDeepLinksInNewWindow: function () {
return this.$store.getters.getOpenDeepLinksInNewWindow
},

enableVimNavigation: function() {
return this.$store.getters.getEnableVimNavigation
}
},
created: function () {
Expand Down Expand Up @@ -273,6 +277,7 @@ export default defineComponent({
'updateExternalLinkHandling',
'updateGeneralAutoLoadMorePaginatedItemsEnabled',
'updateOpenDeepLinksInNewWindow',
'updateEnableVimNavigation',
])
}
})
14 changes: 14 additions & 0 deletions src/renderer/components/general-settings/general-settings.vue
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,20 @@
:tooltip="$t('Tooltips.General Settings.Open Deep Links In New Window')"
@change="updateOpenDeepLinksInNewWindow"
/>
<p class="vim-warning">
<FontAwesomeIcon
:icon="['fas', 'circle-exclamation']"
class="warning-icon"
fixed-width
/>
{{ $t('Settings.General Settings.VIM Warning') }}
</p>
Comment on lines +48 to +55
Copy link
Member

Choose a reason for hiding this comment

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

This warning should only be shown when the setting is enabled. Just like with the checks inside the ifs, when the setting is disabled, the app and code should also act like it is disabled.

<ft-toggle-switch
:label="$t('Settings.General Settings.Enable VIM key-bindings')"
:default-value="enableVimNavigation"
:compact="true"
@change="updateEnableVimNavigation"
/>
</div>
</div>
<div class="switchGrid">
Expand Down
1 change: 1 addition & 0 deletions src/renderer/store/modules/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ const state = {
onlyShowLatestFromChannel: false,
onlyShowLatestFromChannelNumber: 1,
openDeepLinksInNewWindow: false,
enableVimNavigation: false,
playNextVideo: false,
proxyHostname: '127.0.0.1',
proxyPort: '9050',
Expand Down
Loading