spicetify / cli

Command-line tool to customize Spotify client. Supports Windows, MacOS, and Linux.
https://spicetify.app
GNU Lesser General Public License v2.1
18.46k stars 725 forks source link

[exts] Re-add vim bindings to keyboardShortcut.js #2857

Closed rien333 closed 7 months ago

rien333 commented 7 months ago

📝 Provide a description of the new feature

History & motivation

A few years ago, I opened https://github.com/spicetify/spicetify-cli/issues/27, which was meant to start a discussion about making more vim-like shortcuts available to spicetify users. The community responded enthusiastically, and a number of vim-inspired shortcuts were added.

However, on my shiny new installation, I noticed that a few of these shortcuts were removed, https://github.com/spicetify/spicetify-cli/pull/2506 being the culprit. Determining whether this change is a step in the right direction would, I think, require a discussion about the purpose of the bundled keyboard shortcut extension.

Why have a user-modifiable shortcut extension?

Currently, the docs describe keyboardShortcut.js as follows:

Extends Spotify's default keybinds (toggle help modal with ?) with vim like shortcuts. Less time touching the mouse.

  • Ctrl Tab / Ctrl Shift Tab: Navigate items in left sidebar menu. ...
  • J/K: Scroll app page up/down. *Tips hat to Vim users*
  • G/Shift G: Scroll to top or bottom
  • F: Open up keyboard-driven navigation. [this is a staple in other GUI apps with vim-like shortcuts]

Why is this nice to have? To me, this boils down to two things:

  1. Consistency. I do not like thinking about what shortcuts I have to press on a per-app basis. Vanilla spotify really misses the mark here — Ctrl+l is an extremely odd shortcut for searching.[^1] Hence, why this extension exists in the first place: allow users to have a consistent keyboard-based workflow.
  2. Hackability. More of an emacs or GNOME user? Make some simple modifications to keyboardShortcuts.js, and your app behaves as expected.

I could add that vim-style shortcuts are just plain better than spotify's build-in shortcuts. People might disagree, but then again, those who hold that position do not seem to be the intended audience of this extension, going by the wording of the docs ;)

Proposal

Two vim-like shortcuts were removed: going backwards/forwards in history (bound to H/L by default), and a shortcut to open the search menu (bound to /). The reasoning by @ohitstom in https://github.com/spicetify/spicetify-cli/pull/2506 was that these are duplicates, and hence were up for removal.

However, I would politely point out that what attracts people to this extension was never that spotify was missing shortcuts (see above). In fact, you can already scroll with your mouse and the arrow keys, so by that logic, most of the remaining vim shortcuts should be removed as well.

Thus, I believe this extension should either be rebranded as something that does not cater to people that prefer customizing their shortcuts and/or people that like vim-style navigation, or that some of the removed shortcuts (especially the search shortcut, for which the vanilla binding is frankly bizarre) should be re-added.

[^1]: I can understand the reasoning here — in browsers, focusing the URL bar with Ctrl+l is roughly the same as starting a search — but the fact of the matter is that this is objectively unusual.

➕ Additional Information

No response

rxri commented 7 months ago

I understand the frustration(?), and I agree with the points you made here. If that's something like you describe is nice-to-have, you're more than welcome to make a pull request adding them back, or let me know if I should do it

rien333 commented 7 months ago

I understand the frustration(?) here

Haha, all is good! All of this is fairly minor. I just wanted to start a discussion, because I think the reasoning for removing the old features is not air tight.

you're welcome to make a pull request adding them back, or let me know if I should do it

I can have a look at the diff of https://github.com/spicetify/spicetify-cli/pull/2506 and see how difficult restoring some of the previous functionality is. If I have not done that by the end of next week, consider my attempt failed.

ohitstom commented 7 months ago

I understand the frustration(?) here

Haha, all is good! All of this is fairly minor. I just wanted to start a discussion, because I think the reasoning for removing the old features is not air tight.

you're welcome to make a pull request adding them back, or let me know if I should do it

I can have a look at the diff of https://github.com/spicetify/spicetify-cli/pull/2506 and see how difficult restoring some of the previous functionality is. If I have not done that by the end of next week, consider my attempt failed.

I'm happy to restore them myself, since you are now the second person to complain as such...

Just never saw the use in having functions that Spotify already provides, I believe one function had an identical keybind to Spotifys default functionality too, and another was completely broken and already had a default bind

rien333 commented 7 months ago

I'm happy to restore them myself, since you are now the second person to complain as such...

Thanks, I gather you are a more competent programmer and more familiair with the code base.

rxri commented 7 months ago

i hate this bot If the function had the identical keybind to Spotify's then it's fine that it was removed. Other ones could be restored in that case then

ohitstom commented 7 months ago

i hate this bot If the function had the identical keybind to Spotify's then it's fine that it was removed. Other ones could be restored in that case then

Sounds good, will restore previous functionality and try and repair the broken stuff

Unless there are any recommendations from OP for new bindings all together for search etc

rien333 commented 7 months ago

Unless there are any recommendations from OP for new bindings all together for search etc

The bindings were fine how they were — / for searching, and H/L for forwards/backwards in history. People can always modify them if they prefer something less vim-like (granted they know a tiny bit of js). For the reasoning behind these shortcuts, refer to https://github.com/spicetify/spicetify-cli/issues/27. There's some other suggestions for shortcuts there, but I do not consider them a top priority.

Thanks again!

ohitstom commented 7 months ago

Unless there are any recommendations from OP for new bindings all together for search etc

The bindings were fine how they were — / for searching, and H/L for forwards/backwards in history. People can always modify them if they prefer something less vim-like (granted they know a tiny bit of js). For the reasoning behind these shortcuts, refer to #27. There's some other suggestions for shortcuts there, but I do not consider them a top priority.

Thanks again!

I'm having issues getting the vim mode to even activate on my end (using latest spotify and spicetify) is this an issue you have faced?

https://github.com/ohitstom/spicetify-cli/blob/5f77de501800fcb56d4a533ed5bd59a92e6f6095/Extensions/keyboardShortcut.js#L182 seems to return 0 very often and https://github.com/ohitstom/spicetify-cli/blob/5f77de501800fcb56d4a533ed5bd59a92e6f6095/Extensions/keyboardShortcut.js#L178 seems to be displaying zero of the relevant styling tags required

riri might have some pointers but this isnt as simple as i first thought since the base extension is broken in the latest version as it is

rien333 commented 7 months ago

I'm having issues getting the vim mode to even activate on my end (using latest spotify and spicetify) is this an issue you have faced?

Yes, I'm also seeing that. In fact, pressing f puts spotify in an unusable state, where I cannot click on any UI elements. The other vim-inspired shortcuts still work fine, from what I can see.

Oh and btw: the linux spotify client does not respond to ctrl+w or ctrl+q, as others have also noted. Is there any change of making "quit application" a key-bindable action (or "close window", if quiting is not exposed)?

ohitstom commented 7 months ago

I'm having issues getting the vim mode to even activate on my end (using latest spotify and spicetify) is this an issue you have faced?

Yes, I'm also seeing that. In fact, pressing f puts spotify in an unusable state, where I cannot click on any UI elements. The other vim-inspired shortcuts still work fine, from what I can see.

Oh and btw: the linux spotify client does not respond to ctrl+w or ctrl+q, as others have also noted. Is there any change of making "quit application" a key-bindable action (or "close window", if quiting is not exposed)?

Yes we have the ability to close Spotify through platform methods so this can be updated! Will find time to fix and update stuff hopefully tonight or tomorrow

rxri commented 7 months ago

Looks like an issue with vim-overlay element. z-index or something 🤷‍♀️

ohitstom commented 7 months ago

Looks like an issue with vim-overlay element. z-index or something 🤷‍♀️

seems like its returning prematurely since the bounding is failing

rxri commented 7 months ago

Yeah no wonder it is broken because it should use continue and not return. return returns out of the for function

rxri commented 7 months ago
-           if (e.style.display === "none" || e.style.visibility === "hidden" || e.style.opacity === "0") {
-               return;
-           }
+           if (e.style.display === "none" || e.style.visibility === "hidden" || e.style.opacity === "0") {
+               continue;
+           }

and

-               bound.height === 0
-           ) {
-               return;
-           }
+               bound.height === 0
+           ) {
+               continue;
+           }
rxri commented 7 months ago

This might have happened when we were doing the biome migration and we did not notice when switching from forEach

ohitstom commented 7 months ago

yeah managed to figure that much out, still unsure as to why all the .style objects are messed up maybe spotify is preventing it somehow?

rxri commented 7 months ago

wdym by "messed up" lol

ohitstom commented 7 months ago

wdym by "messed up" lol

all the items e.g opacity are just "" and when a style is applied its 0: "opacity" for example whereas it should be opacity: "0" image

edit: image all fixed gonna open a pr tomorrow

rxri commented 7 months ago

wdym by "messed up" lol

all the items e.g opacity are just "" and when a style is applied its 0: "opacity" for example whereas it should be opacity: "0" image

getPropertyValue() should be used. This is how CSSStyleDeclaration object looks in general iirc

ohitstom commented 7 months ago

getPropertyValue() should be used. This is how CSSStyleDeclaration object looks in general iirc

ah, just confused me since i assumed it must have worked at some point

ohitstom commented 7 months ago

Oh and btw: the linux spotify client does not respond to ctrl+w or ctrl+q, as others have also noted. Is there any change of making "quit application" a key-bindable action (or "close window", if quiting is not exposed)?

could you elaborate on what ctrl + w is? also do you have a link to existing keybind specifications used on linux as it seems we previously used ctrl + q for opening the queue page which doesnt seem quite right so i will need to come up with a new binding for that functionality if its to be re-implemented.

I hope to improve the extension all around and implement the suggestions khanas never got to, so collaboration would be great!

rien333 commented 7 months ago

could you elaborate on what ctrl + w is

Ctrl+w (cmd+w on macOS) closes the focused window. It actually works on Windows too (including Microsoft's own applications, I think), but not in all applications. In other words, it's a somewhat "cross-platform" shortcut for closing a window, which for spotify, is about the same thing as quitting the application.

also do you have a link to existing keybind specifications used on linux

That doesn't exist, since that will depend on what desktop environment (DE) the Linux user is using, if any. However, the two major DEs — Gnome spec and KDE spec — are modeled closely after macOS and Windows. For example, both use ctrl+q for quitting the application, analogous to macOS.

However, if you are going to add more binding options, maybe just stick with vim-inspired key binds? vim binds are generally easy to remember since they often require pressing a single key (without any modifiers).

And again, if people do not like a bind, they can always create their own copy of keyboardShortcut.js — provided everything is kept easy to modify and understand.

ohitstom commented 7 months ago

Ctrl+w (cmd+w on macOS) closes the focused window. It actually works on Windows too (including Microsoft's own applications, I think), but not in all applications. In other words, it's a somewhat "cross-platform" shortcut for closing a window, which for spotify, is about the same thing as quitting the application.

so how does this differ from ctrl + q closing the application? im just a little bit confused, im happy to make both ctrl q and ctrl w do the same thing, or i can bind ctrl + w to minimizing?

rien333 commented 7 months ago

how does this differ from ctrl + q closing the application

On most operating systems, an application can live on without having any open windows (macOS has a few apps of this kind, and spot, a spotify alternative on linux, also will not die when the last window is closed). Moreover, some applications can spawn multiple windows, and ctrl+w allows one to close these individually.

However, as you seem to have noticed, neither of these things really applies to spotify. So I would say make ctrl+w do nothing, or make it do the same thing as ctrl+q.

I have no preference either way, though it might be nice to embed an example in the js code that shows users how you can bind multiple keys to a single action.

rien333 commented 7 months ago

Will try this in a bit! Thanks again.

rien333 commented 7 months ago

Works like a charm 🙂

rxri commented 7 months ago

glad to hear

rien333 commented 1 month ago

I thought I already reported this a few months ago (maybe I did, though somewhere else), but scrolling with J/K has been broken for a while now. iirc, it worked for a bit after @ohitstom work.

Though maybe I should just open a new issue.

ohitstom commented 1 month ago

I thought I already reported this a few months ago (maybe I did, though somewhere else), but scrolling with J/K has been broken for a while now. iirc, it worked for a bit after @ohitstom work.

Though maybe I should just open a new issue.

feel free to open a new issue yeah