Multiple fixes to key binding parsing #3887

svobs · 2022-07-22T21:38:40Z

This change has been discussed with the author.
It implements / fixes issues:
Multiple keys bound to the same menu item results in a random key being chosen #3831
Cannot Quit with Q-Key #3881
Using different forms of uppercase letters can cause wrong key binding to be set #3851

Description:
This patch is a subset of (and is actually back-ported from) #3847, and contains only the changes to parsing and mapping key bindings. It is the successor to #3833: it fixes a loophole in that logic which result in unused duplicate entries being stored, while also adding other fixes which couldn't be merged separately.

It does the following

Splits KeyMapping's key field into rawKey and normalizedMpvKey, similar to how its action field has raw & parsed version.
Moves the existing mpv normalization logic out of the KeyMapping constructor and into KeyCodeHelper.
Expands the mpv normalization logic to ensure that in normal form (fixes Using different forms of uppercase letters can cause wrong key binding to be set #3851):
- Special key symbols are capitalized
- A key's uppercase symbol is used if it is available, instead of containing "Shift+"
- Key sequences containing the - (minus) key are properly parsed
Use normal form always when identifying key bindings; use raw form when editing in the UI, to ensure consistent functionality while keeping a smooth user experience
Add check for ignoring the line default-bindings begin, rather than binding it to a ke
Add check for {section_name} (section name in curly braces): if it is "default" then strip the section and include the binding; for other section names do not include the binding (fixes Cannot Quit with Q-Key #3881)

uiryuu · 2022-10-18T07:57:42Z

iina/KeyCodeHelper.swift

+      case META_KEY: modifiers.insert(.command)
+      case CTRL_KEY: modifiers.insert(.control)
+      case ALT_KEY: modifiers.insert(.option)
+        case SHIFT_KEY: modifiers.insert(.shift)


Remove the space

svobs · 2022-10-19T03:58:53Z

I made another commit based on more recent code - this was a little out of date - and fixed typos. The only significant change was that it needed a check for the special line default-bindings start so it didn't look ugly in the UI (even though IINA ignores it otherwise).

svobs · 2022-10-20T10:54:55Z

Ack - found a bug - and just committed the fix. It incorrectly parsed if the rawKey included ended in the minus key (-) but also had modifiers (example: Meta+-". Updated my tests to include that...

…and change use to one or the other appropriately. Improve and expand logic for normalizing MPV key sequences. Add logic to skip over "default bindings start" and ignore bindings which specify non-default input sections.

svobs · 2022-10-21T02:47:03Z

Found one more corner case with key sequences that wasn't handled (--d--). Swift's substring handling is really clunky...

I cleaned up the dash checks in getNextEndIndex() and added some comments. Sorry. Final answer this time.

lhc70000

Regarding the mpv key sequence part: It's great to handle key sequences (and we probably should add support for them later), but since IINA doesn't support key sequences, perhaps currently we should filter them out before displaying/setting the key bindings?

lhc70000 · 2022-10-22T02:35:45Z

iina/AppData.swift

+  static let iinaInputConfListChanged = Notification.Name("IINAInputConfListChanged")
+  static let iinaCurrentInputConfChanged = Notification.Name("IINACurrentInputConfChanged")


These two notifications are unused?

lhc70000 · 2022-10-23T01:11:07Z

iina/AppData.swift

+  static let iinaInputConfListChanged = Notification.Name("IINAInputConfListChanged")
+  static let iinaCurrentInputConfChanged = Notification.Name("IINACurrentInputConfChanged")


These names are unused?

iina/PlayerCore.swift

lhc70000 · 2022-10-23T01:22:00Z

iina/PlayerCore.swift

+      if $0.rawKey == "default-bindings" && $0.action.count == 1 && $0.action[0] == "start" {
+        Logger.log("Skipping line: \"default-bindings start\"", level: .verbose)
+      } else {
+        if let kb = filterSectionBindings($0) {


Can be } else if let kb = ... {

uiryuu

Didn't test for the issues linked, hopefully that has been done by someone else. Just some suggestions based on the code.

uiryuu · 2022-10-22T03:51:15Z

iina/KeyMapping.swift

-      normalizedKey += "+"
+  var confFileFormat: String {
+    get {
+      let iinaCommandString = isIINACommand ? "#@iina " : ""


Consider making "#@iina" a constant instead of writing literals every time?

uiryuu · 2022-10-23T01:09:56Z

iina/KeyCodeHelper.swift

+    var normalizedList: [String] = []
+    for keystroke in keystrokesList {
+      normalizedList.append(normalizeSingleMpvKeystroke(keystroke))
+    }


Suggested change

var normalizedList: [String] = []

for keystroke in keystrokesList {

normalizedList.append(normalizeSingleMpvKeystroke(keystroke))

}

let normalizedList = keystrokesList.map({ normalizeSingleMpvKeystroke($0) })

uiryuu · 2022-10-23T01:10:31Z

iina/KeyCodeHelper.swift

+  }
+
+  /*
+   MPV accepts several forms for the same keystroke. This ensures that it is reduced to a single standardized form


Suggested change

MPV accepts several forms for the same keystroke. This ensures that it is reduced to a single standardized form

mpv accepts several forms for the same keystroke. This ensures that it is reduced to a single standardized form

svobs · 2022-10-24T00:40:07Z

I pushed a new commit, non-forced so you can see the changes.

Why don't we drop the default-bindings line when parsing the conf file?

I agree it's not pretty to put it here, but it's hard to find another place to put it. If we drop the line while parsing the file, we'll end up removing it from the file the first time the user makes any other changes to it, because of the way config files are handled. It's one thing to strip the comments out of a file, but it's bigger thing to strip valid lines out of it, and I'd rather avoid doing that (and this is basically a section start token, so the consequences would be big). There are apparently some users who use the same config files for both IINA and mpv, and they would be adversely affected by it.

Regarding the mpv key sequence part: It's great to handle key sequences (and we probably should add support for them later), but since IINA doesn't support key sequences, perhaps currently we should filter them out before displaying/setting the key bindings?

Fair point. I've updated the normalization code to just pass back anything with more than one- in it. In practice this will cause key sequences to always fail to match against typed characters (as it was doing before). [Edit: ...which had a bug in it. I just force-pushed a fix to it. Need to work on testing my code before I commit, not after...] [Edit: fixed again. This really isn't my day...]

…and change use to one or the other appropriately. Improve and expand logic for normalizing MPV keys. Add logic to skip over "default bindings start" and ignore bindings which specify non-default input sections.

svobs · 2022-10-24T01:15:06Z

Now I see that changes are still retained either way, so I force pushed against the original commit, for tidiness.

uiryuu requested a review from lhc70000 September 22, 2022 12:16

uiryuu reviewed Oct 18, 2022

View reviewed changes

low-batt linked an issue Oct 21, 2022 that may be closed by this pull request

Multiple keys bound to the same menu item results in a random key being chosen #3831

Closed

1 task

svobs force-pushed the pr-improved-bindings-parsing branch from 1e6c52b to 3de6687 Compare October 21, 2022 02:33

svobs force-pushed the pr-improved-bindings-parsing branch from 3de6687 to 756739d Compare October 21, 2022 02:35

lhc70000 requested changes Oct 23, 2022

View reviewed changes

uiryuu requested changes Oct 23, 2022

View reviewed changes

low-batt mentioned this pull request Oct 23, 2022

Controls overlap in video quick settings panel #3790

Closed

1 task

svobs force-pushed the pr-improved-bindings-parsing branch 3 times, most recently from b9911f8 to 997af49 Compare October 24, 2022 01:09

Split KeyMapping's "key" field into "rawKey" and "normalizedMpvKey", …

a0e5354

…and change use to one or the other appropriately. Improve and expand logic for normalizing MPV keys. Add logic to skip over "default bindings start" and ignore bindings which specify non-default input sections.

svobs force-pushed the pr-improved-bindings-parsing branch from 997af49 to a0e5354 Compare October 24, 2022 01:14

uiryuu approved these changes Oct 24, 2022

View reviewed changes

lhc70000 merged commit 15d0451 into iina:develop Oct 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Multiple fixes to key binding parsing #3887

Multiple fixes to key binding parsing #3887

Uh oh!

svobs commented Jul 22, 2022

Uh oh!

uiryuu Oct 18, 2022

Uh oh!

svobs commented Oct 19, 2022

Uh oh!

svobs commented Oct 20, 2022

Uh oh!

svobs commented Oct 21, 2022

Uh oh!

lhc70000 left a comment

Uh oh!

lhc70000 Oct 22, 2022

Uh oh!

lhc70000 Oct 23, 2022

Uh oh!

Uh oh!

lhc70000 Oct 23, 2022

Uh oh!

uiryuu left a comment

Uh oh!

uiryuu Oct 22, 2022

Uh oh!

uiryuu Oct 23, 2022

Uh oh!

uiryuu Oct 23, 2022

Uh oh!

svobs commented Oct 24, 2022 •

edited

Loading

Uh oh!

svobs commented Oct 24, 2022

Uh oh!

Uh oh!

		static let iinaInputConfListChanged = Notification.Name("IINAInputConfListChanged")
		static let iinaCurrentInputConfChanged = Notification.Name("IINACurrentInputConfChanged")

	MPV accepts several forms for the same keystroke. This ensures that it is reduced to a single standardized form
	mpv accepts several forms for the same keystroke. This ensures that it is reduced to a single standardized form

Uh oh!

Multiple fixes to key binding parsing #3887

Multiple fixes to key binding parsing #3887

Uh oh!

Conversation

svobs commented Jul 22, 2022

Uh oh!

uiryuu Oct 18, 2022

Choose a reason for hiding this comment

Uh oh!

svobs commented Oct 19, 2022

Uh oh!

svobs commented Oct 20, 2022

Uh oh!

svobs commented Oct 21, 2022

Uh oh!

lhc70000 left a comment

Choose a reason for hiding this comment

Uh oh!

lhc70000 Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

lhc70000 Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lhc70000 Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

uiryuu left a comment

Choose a reason for hiding this comment

Uh oh!

uiryuu Oct 22, 2022

Choose a reason for hiding this comment

Uh oh!

uiryuu Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

uiryuu Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

svobs commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svobs commented Oct 24, 2022

Uh oh!

Uh oh!

svobs commented Oct 24, 2022 •

edited

Loading