Skip to content

Fix failure to write '#' to key bindings file #4271

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

Merged
merged 5 commits into from
May 9, 2023

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Mar 16, 2023


Description:

  • # and could previously be entered into the Key Bindings editor by the user, but they would get written out to the .conf file as-is, so when they were read in again on the next launch, they would be ignored. Even more confusing for the user, the values in memory would still work, but only until the next launch.
  • Since there is no other legitimate reason for a user to enter these into the editor, this patch alters the logic so that # is written out as SHARP and is written out as SPACE.

let keyMappings = mappingController.arrangedObjects as! [KeyMapping]
for mapping in keyMappings {
mapping.rawKey = KeyCodeHelper.escapeReservedMpvKeys(mapping.rawKey)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to change, but could use forEach instead of for:

keyMappings.forEach { $0.rawKey = KeyCodeHelper.escapeReservedMpvKeys($0.rawKey) }

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost worked for me.

With these changes the .conf file now contains:

SHARP quit
SPACE quit

The key bindings all look correct in settings. Looks as expected when I toggle Display raw values. When I press space IINA quits. BUT when I press # IINA responds with the unrecognized key sound and does not quit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to change, but could use forEach instead of for:

keyMappings.forEach { $0.rawKey = KeyCodeHelper.escapeReservedMpvKeys($0.rawKey) }

I actually prefer to use a traditional for loop instead of a forEach, unless there's a really good reason to use it. I like having named variables and being able to introduce break, and maybe there's some jadedness from using debuggers which can't handle closures as well and make it harder to debug. I get the sense that some developers tell themselves they're making their code more efficient by reducing the number of lines written, but I think usually they're just increasing the bug density.

BUT when I press # IINA responds with the unrecognized key sound and does not quit.

Well...yikes. I dug into this and found a whole bunch of bugs.... will comment more below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is why I mentioned no need to change. I definitely will use a traditional for if the situation makes it useful to be able to use break or continue.

@low-batt low-batt linked an issue Mar 16, 2023 that may be closed by this pull request
1 task
…+' keys to be ignored, and caused special keys which have "uppercase" versions (e.g. '4'->'$') to be ignored if using an explicit "Shift+" in the .conf file. Also fix a bug which caused equalsIgnoreCase to do a case sensitive comparison.
@svobs
Copy link
Contributor Author

svobs commented Mar 17, 2023

Well...when I dug into your feedback, I found a bunch of issues with parsing. I suppose I could pack these changes into a separate PR...

Background refresher:

  • There are multiple ways to represent a key sequence in the spec, but IINA does a string comparison to match the user's typed key(s) to a given key binding. Thus, I added a bunch of code to "normalize" it into a common format when doing the comparison (a lot of which used a bunch of code that was already there in KeyCodeHelper but tied it together).

1: SHARP & PLUS

There were several problems with the code which deals with special characters. The fix which allowed the SHARP key to be written to file broke the code which recognized whether that key was pressed. This underlying issue was that SHARP and PLUS are special token characters used by mpv, so they need to always been normalized to their whole-word representations instead of their literal # and + versions. But they weren't being normalized at all.

For these special symbols, the mpv .conf file spec allows for either literal (e.g., +) or whole-word identifier (PLUS). When MacOS sends a key event to IINA, it is always translated to the literal version by default, so that's what it uses for the comparison by default. That means that anything specified in the whole-word version wouldn't be recognized when typed.

2: Uppercase handling was broken for non-alpha uppercase chars

There some code which I never dug into or tested, but with testing is clearly broken. The check for key.lowercased() != key.uppercased() doesn't do what we want to detect keys like > (we wanted it to detect it as a shifted . key). Likewise, the event.readableKeyDescription code is not returning valid data.

So I re-purposed the keyMap data structure in KeyCodeHelper, which largely wasn't being used and only needed a few fixes, to add a check for these special chars.

3: Case insensitive comparison broken

Most embarrassingly, localizedCompare() was being used instead of localizedCaseInsensitiveCompare(), which meant that equalsIgnoreCase() was actually not ignoring case... So this showed up when I tried Shift+shift++ as a test case.

I put a few examples in the code comments which illustrate what was breaking. It sounds like a lot but most of these will probably only be noticed by people coming to IINA from mpv.

if rawKeyChar != char {
modifiers.remove(.shift)
}
// The char in `charactersIgnoringModifiers` will be either uppercase or lowercase,
Copy link
Contributor

Choose a reason for hiding this comment

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

From the Swift DocC section Add a Basic Description for Each Symbol:

If a symbol already has a source comment that begins with two forward slashes (//), insert an additional forward slash (/) to convert it to a documentation comment.

I'm pretty sure Markdown formatting can only be used in swift documentation comments. I'm having trouble finding a reference, but I believe documentation comments will only be recognized before declarations, not inlined in a code block.

/// Ex04: "meta+shift+k" → "Meta+K"
/// Ex05: "esc" → "ESC"
/// Ex06: "CTRL+SHIFT+SHIFT+SHIFT+x" → "Ctrl+X"
/// _
Copy link
Contributor

Choose a reason for hiding this comment

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

In Xcode, clicking on normalizeSingleMpvKeystroke and then on the ? button Xcode will show you what the rendered documentation looks like.

Normalizes a single “press” of possibly multiple keys (as joined with ‘+’) Examples: Ex01: “Shift+-” → “” Ex02: “Shift+=” → “PLUS” Ex03: “+-+” → “PLUS-PLUS” Ex04: “meta+shift+k” → “Meta+K” Ex05: “esc” → “ESC” Ex06: “CTRL+SHIFT+SHIFT+SHIFT+x” → “Ctrl+X” _

Markdown squishes it into one line. Maybe make use of a Markdown numbered list?

With a quick change to:

/// Normalizes a single "press" of possibly multiple keys (as joined with '+')
///
/// _Examples:_
/// 1. "Shift+-" → "_"
/// 2. "Shift+=" → "PLUS"
/// 3. "+-+" → "PLUS-PLUS"
/// 4. "meta+shift+k" → "Meta+K"
/// 5. "esc" → "ESC"
/// 6. "CTRL+SHIFT+SHIFT+SHIFT+x" → "Ctrl+X"

Xcode shows:

issue-4267

Could still be improved. I have the Basic Syntax and Extended Syntax pages from the Markdown Guide site bookmarked.

There are various apps that support editing Markdown. I've been using the open source Joplin application. Sources can be found on GitHub here.

@low-batt
Copy link
Contributor

Something it still not quite right.

As a test I created some bindings using the IINA UI that were stored like this by IINA in the conf file:

SHARP quit
SPACE quit
+ quit
! quit
@ quit
$ quit
% quit
TAB quit
BS quit
ESC quit
{ quit
} quit
` quit
F2 quit
ENTER quit

A few of these bindings still did not work for me:

Key Results Notes
SHARP FAILED Made the unrecognized key sound
SPACE Passed
+ FAILED Made the unrecognized key sound
! Passed
@ Passed
$ Passed
% Passed
TAB FAILED Did not make the unrecognized key sound
BS Passed
ESC Passed
{ Passed
} Passed
` Passed
F2 Passed
ENTER Passed

@svobs
Copy link
Contributor Author

svobs commented Mar 18, 2023

Thanks for the tips on XCode/Markdown formatting. It's one of those things I sort of care about but haven't spent any real time studying. I updated the comments in question so it looks nicer.

On the use of caret symbols: even though I wasn't looking at the formatted version, I had fallen into using them as a practical way to indicate that the things inside identifiers and/or code snippets. It looks less ambiguous to me than using quotes or no punctuation at all. And XCode renders them in bold inside code blocks. Though I understand how some people might find them ugly or off-putting, so I'll try to avoid using them outside of declarations in the future.

I've been working in a few branches and I didn't notice the call to normalize the key input wasn't included (oops). Fixed in the latest commit.

But I need to look more at the issue of TAB not working. It's working in my big binding PR (#4160), but I made lots of fixes there which smooth out some weird stuff that is being done to the responder chain in the develop branch. Odd that the arrow keys work, but TAB doesn't...

Copy link
Contributor

@low-batt low-batt left a comment

Choose a reason for hiding this comment

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

Pulled PR, built IINA, confirmed # key binding is working. Looks good to me.

@svobs svobs closed this Apr 11, 2023
@svobs svobs deleted the pr-fix-octothorpe-key branch April 11, 2023 08:08
@svobs svobs restored the pr-fix-octothorpe-key branch April 23, 2023 06:44
@svobs
Copy link
Contributor Author

svobs commented Apr 23, 2023

Oops. I think I tried renaming some branches a couple of weeks ago, and this got automatically deleted when I did! Will see if this happened to any other active PRs.

@svobs svobs reopened this Apr 23, 2023
@uiryuu uiryuu merged commit b869aad into iina:develop May 9, 2023
@uiryuu
Copy link
Member

uiryuu commented May 9, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key Binding to "#" (Shift+3) fail upon IINA restarts
3 participants