-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
…em when writing to file.
let keyMappings = mappingController.arrangedObjects as! [KeyMapping] | ||
for mapping in keyMappings { | ||
mapping.rawKey = KeyCodeHelper.escapeReservedMpvKeys(mapping.rawKey) | ||
} |
There was a problem hiding this comment.
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) }
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 offor
: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.
There was a problem hiding this comment.
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
.
…+' 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.
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:
1: SHARP & PLUS There were several problems with the code which deals with special characters. The fix which allowed the For these special symbols, the mpv 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 So I re-purposed the 3: Case insensitive comparison broken Most embarrassingly, 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. |
iina/KeyCodeHelper.swift
Outdated
if rawKeyChar != char { | ||
modifiers.remove(.shift) | ||
} | ||
// The char in `charactersIgnoringModifiers` will be either uppercase or lowercase, |
There was a problem hiding this comment.
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.
iina/KeyCodeHelper.swift
Outdated
/// Ex04: "meta+shift+k" → "Meta+K" | ||
/// Ex05: "esc" → "ESC" | ||
/// Ex06: "CTRL+SHIFT+SHIFT+SHIFT+x" → "Ctrl+X" | ||
/// _ |
There was a problem hiding this comment.
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:
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.
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
A few of these bindings still did not work for me:
|
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 |
There was a problem hiding this 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.
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. |
Thanks! |
Description:
#
and.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.#
is written out asSHARP
andSPACE
.