VIM-like Navigation #7336

xeon826 · 2025-05-01T15:47:11Z

VIM Navigation

Pull Request Type

Bugfix
Feature Implementation
Documentation
Other

Related issue

#251 (comment)
#1864
Closes #1864

Description

Implements VIM-like key-bindings, similar to Vimium.

Screenshots

Testing

I tested this with basic usage. Navigating to both general and video player keybinding contexts.

Desktop

OS: Arch
OS Version: Rolling
FreeTube version: v0.23.1 Beta

Additional context

Any conflicting native keybindings will change when vim navigation is enabled. The change will reflect in the keyboard shortcuts modal (Shift + ?). I've yet to implement back and forth page navigation but it should be pretty easy.

Note that there's a prior iteration of this PR in which it was requested that any custom formatting be removed, this is the follow up to that.

efb4f5ff-1298-471a-8973-3d47447115dc · 2025-05-01T16:30:02Z

I’m a bit concerned that the new PR might end up getting abandoned just like the previous one, especially since it was abandoned after one day. I’m worried that reviewing it might end up being a waste of time if it gets discarded again.

absidue

At first glace there are a few issues with this pull request:

Please fix the invalid code (referencing variables that you didn't actually create), missing translation strings and wrong indentation pointed out by the linter.
The title of this pull request is misleading, it sounds like you are just implementing VIM waypoints but this actually changes various keyboard shortcuts, which personally I don't think is a good idea unless it is made extremely obvious to the user that that is what is happening.
Please choose a unique keyboard shortcuts to show and hide the waypoints as well as for the other VIM interactions such as scrolling, they should not conflict with the existing keyboard shortcuts and hotkeys. Alternatively you could ensure that these VIM hotkeys are disabled on the watch page or make it very obvious to the user that the behaviour of various well-known keyboard shortcuts and hot keys will change when the VIM setting is enabled.
Please don't unnecessarily place functions inside of functions, as that will result in the inner functions getting treated like variables and recreated every time the outer function is called.
While a prompt is open only things inside that prompt can be triggered but this will show waypoints even on things that are not accessible while prompts are open.

src/renderer/store/modules/utils.js

src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js

absidue · 2025-05-01T17:00:01Z

Instead of pushing and commenting after every single commit, please just do one push and comment at the end, so that we don't get spammed with notifications.

Co-authored-by: absidue <[email protected]>

github-actions · 2025-05-02T13:20:27Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

xeon826 · 2025-05-02T13:34:03Z

The t hotkey was assigned to theatre mode, which now doesn't have a hot key. Please chose a multi-key combination for the VIM hotkeys. Additionally if you want to break existing keyboard shortcuts when this is enabled there needs to be some very clear messaging for the user e.g. a banner or a large message next to the setting when it is enabled like with the proxy settings.

When I press t in the development branch, it toggles the playback of the video, are you sure it's being used for theater mode?

absidue · 2025-05-02T14:02:19Z

Yes, I am, because this is where the KeyboardShortcuts.VIDEO_PLAYER.GENERAL.THEATRE_MODE property that you deleted is being used:

FreeTube/src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js

Lines 2260 to 2269 in e2a53d0

    
           case KeyboardShortcuts.VIDEO_PLAYER.GENERAL.THEATRE_MODE: 
        
             // Toggle theatre mode 
        
             if (props.theatrePossible) { 
        
               event.preventDefault() 
        
               events.dispatchEvent(new CustomEvent('toggleTheatreMode', { 
        
                 detail: !props.useTheatreMode 
        
               })) 
        
             } 
        
             break

xeon826 · 2025-05-02T14:06:28Z

Yes, I am, because this is where the KeyboardShortcuts.VIDEO_PLAYER.GENERAL.THEATRE_MODE property that you deleted is being used:

FreeTube/src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js

Lines 2260 to 2269 in e2a53d0

case KeyboardShortcuts.VIDEO_PLAYER.GENERAL.THEATRE_MODE:

// Toggle theatre mode

if (props.theatrePossible) {

event.preventDefault()

events.dispatchEvent(new CustomEvent('toggleTheatreMode', {

detail: !props.useTheatreMode

}))

}

break

I'm saying when you open the video player and press t, it toggles the playback, not theater mode.

absidue · 2025-05-02T14:09:29Z

For me it only does theatre mode, it doesn't pause or play the video.

xeon826 · 2025-05-02T20:06:06Z

For me it only does theatre mode, it doesn't pause or play the video.

That could be a bug then because for me it toggles the playback.

efb4f5ff-1298-471a-8973-3d47447115dc · 2025-05-02T20:08:24Z

There is no bug in the dev branch. You can verify that by looking at the code yourself

efb4f5ff-1298-471a-8973-3d47447115dc · 2025-05-02T20:12:51Z

I see that you commited something to the dev branch. That info in available to me through GH. This tends me to believe that you have made more commits locally to your dev branch.

xeon826 · 2025-05-03T16:35:25Z

I added a small banner above the vim toggle to inform the user that the large rewind key will be remapped to h if enabled.
Changed the way-point toggle key to w so we can keep the theater mode toggle as it is. It's important that the vim toggle key be easily accessible as most users will attest it's a very commonly used key particularly when working/writing code.
Re-added the theater mode toggle.
Created a subsection for vim-related hotkeys in the shortcut modal.
Removed the automated translations that I had created.

absidue

When adding a new optional setting, especially one as invasive as this one, please remember to test with the setting disabled too.

absidue · 2025-05-14T15:43:41Z

src/renderer/App.js

+      // 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) {


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.

absidue · 2025-05-14T15:44:52Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue

@@ -81,6 +81,10 @@ const situationalAppShortcuts = computed(() =>
  getLocalizedShortcutNamesAndValues(KeyboardShortcuts.APP.SITUATIONAL)
 )

+const VimAppShortcuts = computed(() =>


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(() =>

absidue · 2025-05-14T15:46:36Z

src/renderer/components/ProxySettings/ProxySettings.css

+.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;
+}
+


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

absidue · 2025-05-14T15:47:32Z

src/renderer/components/ft-shaka-video-player/ft-shaka-video-player.js

@@ -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']:


absidue · 2025-05-14T15:50:39Z

src/renderer/components/general-settings/general-settings.vue

+        <p class="vim-warning">
+          <FontAwesomeIcon
+            :icon="['fas', 'circle-exclamation']"
+            class="warning-icon"
+            fixed-width
+          />
+          {{ $t('Settings.General Settings.VIM Warning') }}
+        </p>


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.

absidue · 2025-05-14T15:55:51Z

src/renderer/components/FtKeyboardShortcutPrompt/FtKeyboardShortcutPrompt.vue

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

absidue · 2025-05-14T16:00:41Z

src/renderer/App.js

@@ -344,20 +351,59 @@ export default defineComponent({
    },

    activateKeyboardShortcuts: function () {
+      if (this.enableVimNavigation) {


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.

absidue · 2025-05-14T16:02:02Z

src/renderer/store/modules/utils.js

+  showVimWaypoints({ commit }) {
+    commit('setAreVimWaypointsShown', ['w'])
+  },
+
+  hideVimWaypoints({ commit }) {
+    commit('setAreVimWaypointsShown', [])
+  },
+


These are never used, please revert.

absidue · 2025-05-14T16:05:38Z

src/renderer/store/modules/utils.js

+    function close() {
+      document.querySelectorAll('.vimHint').forEach((el) => el.remove())
+      state.areVimWaypointsShown.selector = []
+    }
+
+    function numberToHintStr(number, chars, digits = 0) {
+      const base = chars.length
+      let hintStr = ''
+      let remainder = 0
+
+      do {
+        remainder = number % base
+        hintStr = chars[remainder] + hintStr // Unshift using string operations
+        number = Math.floor(number / base)
+      } while (number > 0)
+
+      // Use padStart to ensure the string is at least 'digits' in length.
+      hintStr = hintStr.padStart(digits, chars[0])
+
+      return hintStr
+    }
+
+    // Example usage: Generate hints for all link elements
+    function generateHints(minChars, chars) {
+      // Get all anchor (A) elements from the document.
+      const elems = (document.querySelector('.prompt') || document).querySelectorAll(
+        'a, button, [type="button"], [type="submit"], [role="tab"], [role="button"], input[type="text"], [role="link"]'
+      )
+      const numLinks = elems.length
+
+      // Determine the minimum number of digits needed for the hints.
+      const needed = Math.max(minChars, Math.ceil(Math.log(numLinks) / Math.log(chars.length)))
+
+      let shortCount = 0
+      if (needed > minChars && needed > 1) {
+        const totalSpace = Math.pow(chars.length, needed)
+        shortCount = Math.floor((totalSpace - numLinks) / (chars.length - 1))
+      }
+
+      const longCount = numLinks - shortCount
+      const hints = []
+
+      if (needed > 1) {
+        for (let i = 0; i < shortCount; i++) {
+          hints.push(numberToHintStr(i, chars, needed - 1))
+        }
+      }
+
+      const start = shortCount * chars.length
+      for (let i = start; i < start + longCount; i++) {
+        hints.push(numberToHintStr(i, chars, needed))
+      }
+
+      return hints
+    }


As was already mentioned in a previous review, please don't nest functions inside of functions. Not only does it make the code messier and less readable, but it also forces the JavaScript runtime to redefine the functions each time the outer one is called.

absidue · 2025-05-14T17:57:37Z

src/renderer/store/modules/utils.js

+        const hintStyles = {
+          position: 'absolute',
+          background: 'linear-gradient(to bottom, #fff204 0%, #d8cb03 100%)',
+          fontSize: '13px',
+          fontFamily: '-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, Arial, sans-serif',
+          fontWeight: '600',
+          borderRadius: '3px',
+          color: 'black',
+          zIndex: '10000', // Ensure it is above other content
+          padding: '1px 2px',
+          letterSpacing: '0.5px',
+          display: 'flex' // Use flexbox for letter alignment
+        }


As you are already assigning a class to this element, it would make a lot more sense to put the CSS in the App.css file. You'll need to use the :deep() selector as you are creating these elements outside of Vue.

xeon826 · 2025-05-23T23:09:02Z

Will have a look.

xeon826 added 4 commits May 1, 2025 09:34

Use f key to activate vim waypoints

076bebe

Toggle vim functionality

99e2120

Order hints alphabetically

2bd8ba9

Ensure waypoint hotkey doesn't trigger full screen in video player

48310ee

FreeTubeBot enabled auto-merge (squash) May 1, 2025 15:47

github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label May 1, 2025

absidue reviewed May 1, 2025

View reviewed changes

absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels May 1, 2025

Remove debugging code

e4b381d

auto-merge was automatically disabled May 1, 2025 16:46
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 1, 2025 16:46

Use native js functions for padding

e010f52

auto-merge was automatically disabled May 1, 2025 16:57
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 1, 2025 16:57

Use computed function to get store values

32a250e

auto-merge was automatically disabled May 1, 2025 17:08
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 1, 2025 17:08

xeon826 changed the title ~~Vim waypoints~~ VIM-like Navigation May 1, 2025

Update src/renderer/store/modules/utils.js

fb4e273

Co-authored-by: absidue <[email protected]>

auto-merge was automatically disabled May 1, 2025 17:14
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 1, 2025 17:14

xeon826 added 5 commits May 1, 2025 12:38

Consistent data type for areVimWaypointsShown

3b9f62d

Replaced unnecessary nested functions in setAreVimWaypointsShown

2912fed

Only display hints in prompt if it's displayed

afa614e

Declare enableVimNavigation computed variable

fabe9ec

Mitigate hotkey overlap

721352e

auto-merge was automatically disabled May 1, 2025 21:23
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 2, 2025 13:20

Merge remote-tracking branch 'upstream/development' into vim-waypoints

c06dfd5

auto-merge was automatically disabled May 2, 2025 13:21
Head branch was pushed to by a user without write access

xeon826 force-pushed the vim-waypoints branch from 44a43b0 to c06dfd5 Compare May 2, 2025 13:21

FreeTubeBot enabled auto-merge (squash) May 2, 2025 13:21

xeon826 added 2 commits May 3, 2025 10:02

Add a banner to denote the hotkey change above vim toggle

b3d424f

Change hotkey to w

72274ac

auto-merge was automatically disabled May 3, 2025 15:02
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 3, 2025 15:02

Section for vim navigation hotkeys

fed12b8

auto-merge was automatically disabled May 3, 2025 16:24
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 3, 2025 16:24

Remove duplicate logic

aaa777e

auto-merge was automatically disabled May 3, 2025 16:28
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 3, 2025 16:28

Removed non-existent tooltip translation

5d67b25

auto-merge was automatically disabled May 3, 2025 16:43
Head branch was pushed to by a user without write access

FreeTubeBot enabled auto-merge (squash) May 3, 2025 16:43

efb4f5ff-1298-471a-8973-3d47447115dc requested a review from absidue May 13, 2025 10:34

absidue reviewed May 14, 2025

View reviewed changes

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

VIM-like Navigation #7336

Are you sure you want to change the base?

VIM-like Navigation #7336

Uh oh!

Conversation

xeon826 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

VIM Navigation

Pull Request Type

Related issue

Description

Screenshots

Testing

Desktop

Additional context

Uh oh!

efb4f5ff-1298-471a-8973-3d47447115dc commented May 1, 2025

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

absidue commented May 1, 2025

Uh oh!

github-actions bot commented May 2, 2025

Uh oh!

xeon826 commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

absidue commented May 2, 2025

Uh oh!

xeon826 commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

absidue commented May 2, 2025

Uh oh!

xeon826 commented May 2, 2025

Uh oh!

efb4f5ff-1298-471a-8973-3d47447115dc commented May 2, 2025

Uh oh!

efb4f5ff-1298-471a-8973-3d47447115dc commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xeon826 commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

absidue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xeon826 commented May 23, 2025

Uh oh!

Uh oh!

xeon826 commented May 1, 2025 •

edited

Loading

xeon826 commented May 2, 2025 •

edited

Loading

xeon826 commented May 2, 2025 •

edited

Loading

efb4f5ff-1298-471a-8973-3d47447115dc commented May 2, 2025 •

edited

Loading

xeon826 commented May 3, 2025 •

edited

Loading