-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Key Bindings overhaul: TableView enhancements, centralized config flow #4160
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
base: develop
Are you sure you want to change the base?
Conversation
3ee008f
to
2390dce
Compare
Updated: fixed an issue where trying to load a config file which didn't exist could result in infinite table refresh |
Fixed a minor bug which caused in-line editor to be opened at the same time popup editor was opened |
…for Lua scripts, key sequences, and other fixes. Complete overhaul of Conf & Binding tables in prefs UI to add dynamic table update, icons, coloring, animations, cut, copy, paste, context menus, drag & drop features, inline cell editing, multi-file import, much more
…lidate text formatting code
…o their defaults after their key bindings were removed.
…gs using the popup
…acters (and also case handling in general!), and also fix errors handling '#' and '+'
… formatting so it is XCode-friendly.
… Also add macro to search field to filter by normalized mpv key
Is this abandoned? |
@konkrotte not at all. I think this was too large and complex to be accepted as a PR. But I've rolled it into my fork along with a lot of other features I've been working on. I think I've finally gotten it stable enough to release a binary. You can download it here. Please feel free to give me feedback and definitely know if you run into any problems. |
Description:
(Technically should be called "input bindings" because mouse input is also supported...)
Finally pushing this out. I've pumped a huge amount of time and energy into the key bindings system and have finally come to a logical stopping point. It has gotten a lot bigger than I originally planned...
I had to refactor the whole thing so many times to get to its present state...I think if I had created PRs and had to justify each refactor, I'd look like a crazy person wasting everyone's time.
Rather than spend a bunch more time trying to break it into pieces, I decided to experiment with submitting one big PR, with an initial dump of documentation to explain it. Will decide which direction to take next based on the reception it does or doesn't receive. I've tested it thoroughly, but there's always the possibility of some edge case or configuration I missed. I'm more than willing to answer any questions about it, as well as feedback and/or requests for changes.
Some features:
Duplicate.Conf.mov
New context menu, and inline rename of configurations.
Export.to.Text.mov
Drag and drop bindings from Binding table to text editor to export as text.
Import.drag.between.files.edit.mov
Drag and drop Conf files from Finder to edit. Drag bindings onto Conf files to copy from one to another.
Sequence.Bindings.Fmt.mov
Sequence bindings are supported, and can be edited in "raw" mode. When "raw" mode is off, they are formatted using commas.
Override.Binding.mov
Color and formatting change in real-time when bindings conflict
For explanation purposes I can mostly break down this PR into 2 big parts: frontend & backend.
Enhancements to Key Bindings Settings UI
There are no new controls on this page. But a huge amount of work went into into adding all the bells and whistles to the Conf & Bindings tables on this page.
Search
box.Undo
andRedo
in both tables (any changes to any of the items in either table, and also the act of changing the current configuration). TheUndo
&Redo
items in the Edit menu change names to show which operation will be undone/redone. [Details: There is no limit to the number of undoes, but theUndoManger
is tied to the Settings window, so undo & redo for these tables is only accessible while the Settings window is being shown, but the possible undoes will stay remembered until IINA is quit. Undo of Conf file delete is made possible by storing the deleted conf in undo memory until quit.]Tab
&Shift
+Tab
is now supported on the page, to jump between text fields and tables without a mouse. It was a minor change to get this working, and as a side effect, the rest of theSettings
window should also now support it.System Settings
>Keyboard
>Keyboard navigation
is also supported for Key Bindings. When used, this extends the usual tab navigation to other things like buttons (see this video for a description of expected behavior). The chain isn't perfect - the key view loop includes one extra unusable tab stop - but it's at least usable for accessibility purposes now.Conf table
Edit
menu operations (Copy
,Paste
,Delete
) (although notCut
) and their key equivalents. Paste bindings anywhere in the same conf file, or into other conf files. Or paste into a text file.+
button adds a new config inline, instead of via a dialog. This is done by adding a blank row at the bottom of the table when the user clicks the+
button and opening an edit text field for it. The file isn't created until they close the editor, and if the user closes the editor before typing anything, the row is removed - same if the user enters a name which won't work, though an error message will pop up first.Key Bindings table
Display raw values
is checked, in-line editing is used:Tab
,Shift
+Tab
,Return
orEnter
keys to quickly save each edit & navigate between cells, orESC
to cancel changes.Display raw values
is unchecked.New backend logic
*.conf
file)mp.add_key_binding
,mp.add_forced_key_binding
, andmp.remove_key_binding
, most commonly by Lua scriptsBackend design
After a lot of effort working with mpv’s key bindings system to implement PR #3847 and examining how key bindings are set in IINA plugins, I realized that the way IINA plugins were setting menu item keys was basically identical to how mpv defines "weak" bindings. Then I came to the realization that the "stack" of input sections that mpv uses to prioritize key bindings could be simplified and represented as a single double-ended list of key-action pairs, where:
But now being able to put everything into one list with consistent rules, it seemed like it could actually be understandable enough to include it all in one table in the UI. IINA's Preferences window already had a Key Bindings table, but it was populated only with the bindings from the user's input.conf file (or some other conf file they chose), so I needed to add 3 more sources of bindings (see below). So in addition to all the UI changes there was a bunch of plumbing work needed to reroute the various sources of bindings into a central structure which was upstream of that table.
Rationale
There are many different actions in IINA which can be bound to a unique input (key combination, mouse button, touchpad gesture, …), and not all of them are specified in the same place.
But MacOS assumes exactly one keyboard and (at most) one pointing device. Adding a second keyboard doesn't double your number of characters, nor does adding a second mouse provide a second cursor (oddly...) - so, only one action can be mapped to a given input at a given time. In the case of menu items, MacOS will notice if we try to assign the same key equivalent to a new menu item, and will silently remove it from the previous menu item.
To avoid such resource contention and user frustration, it's useful to have a well-defined system of priorities.
Not all of IINA’s categories of key bindings have the same scope:
Video
>Video Filters…
, andAudio
>Audio Filters…
- but should these be prioritized above the config file's bindings, or below?So it seems like each group has its own requirements. But they can be made to play together in a coherent way without too much effort, as I'll try to show below.
Structs and state
Swift tries to gently nudge us toward the use of structs in certain ways (free constructor included!), and it does look like its compiler heavily optimizes their use - so much so, that it actually looks like creating copies of them doesn't actually allocate more memory than its reference. But I found that structs are also a great way to avoid concurrency issues. So wherever possible, when there is needed a compound object which holds some state: rather than changing its individual variables, I instead created a copy of the object with only the field I want changed. I haven't done serious performance testing but anecdotally the result appears to be basically as fast, while avoiding the unintended side effects which might occur from changing the data of a variable which has multiple references. See:
AppInputConfig
,BindingTableState
,ConfTableState
,InputConfFile
,CellEditTracker.CurrentFocus
. The classKeyMapping
is intended to be a struct "in spirit" but had to be a class which inherited fromNSObject
in order to be clipboard-representable.Core design
PlayerInputConfig
InputBinding
s is in effect, because only one window can have focus at a time.InputSectionStack
), and when a given player is active, all of the current bindings will be determined from that player's stack.InputSectionStack
is itself contained within aPlayerInputConfig
instance, which is attached to a singlePlayerCore
instance.PlayerInputConfig
is to encapsulate all the state a player needs to resolve its input when requested. At the moment, the only other state it has other than the stack is and a buffer of its last 4 typed keys, which is needed for detecting key sequences.AppInputConfig
InputSectionStack
has all the data needed to determine key priorities, some compilation is necessary to get the final set of mappings. First all the sections are arranged into a single flat candidate list according to priority, and then that list is checked for duplicate keys, with only the highest priority of each duplicate being enabled.AppInputConfig
.AppInputConfig
is built to replace the previous instance. By keeping each instance immutable once it's built, a lot of concurrency issues can be avoided without the need to use locks or extra queues, and this more than compensates for the price of building new objects each time.AppInputConfig.current
holds the most authoritative, up-to-date version, which will be consulted to do key lookups for user input.AppInputConfig.rebuildCurrent()
can be called from anywhere in the code to regenerate it. But it only needs to be called when the active set of bindings needs to be updated. As stated before this means either:InputSection
s in the current active player, including adding, removing, or changing bindings; or adding, removing, or replacing a section.InputSectionStack
into anAppInputConfig
is encapsulated in the classAppInputConfigBuilder
.bindingCandidateList
field of anAppInputConfig
is the same list of bindings that is shown in the Key Bindings table in in the UI, and itsresolverDict
serves as the primary dictionary for key lookups in the player window.InputSectionStack
Bindings are grouped into
InputSection
s, AnInputSection
is just an ordered list of bindings plus a flag that indicates whether the section is strong or weak - also known as forced or default, in the language of mpv.If a section is strong, that means all of its bindings should be treated as strong. When it is weak, all of its bindings should be treated as weak.
The "default" section, as inherited from mpv, is populated from the user's input conf file. This is not to be confused with "default" (weak) bindings (which mpv also sometimes refers to as "builtin"). In the standalone mpv player, the "default' section has the unique property of being able to contain both "forced" and "default" bindings at the same time, but some of its downstream projects, IINA among them, see this as needless complexity and choose not to enable its "default" bindings. So in IINA it can be considered a "strong" section.
Every
PlayerCore
will have aPlayerInputConfig
, which includes anInputSectionStack
. The first 4 sections in everyInputSectionStack
will be the following, which are shared for every player:New sections can be created and added to the top of a player's stack. Currently this is only done for dynamic bindings set via mpv.
When this player is made active, or when any of the active player's sections is updated, then
AppInputConfig.rebuildCurrent()
is called, which extracts the bindings from the player’sInputSectionStack
and puts them into a big list of increasing priority. To do this, it iterates through its list of sections, and for each section, it adds each binding to:See diagram for Backend Design above.
Dynamic bindings
Here's an example which uses a config directory which contains
webm.lua
. This script creates a single weak binding forShift+w
when it starts up. When the user presses it, an OSD is displayed and additional bindings are set. One of those bindings isESC
, and if the user presses that, the OSD is closed andmp.remove_key_binding(name)
is called to remove them.Dynamic-Lua-bindings-OLD.mp4
(Note: the UI in this example is from an earlier prototype, so the icons and colors are not up-to-date, but the behavior is still accurate).
Frontend notes
Inline table editing
Apple added a built-in cell editor and double-click action handling (
doubleAction
) toNSTableView
a few years ago, but it has some annoying shortcomings, the most glaring of which is that double-clicks don't work on the whole cell; they only work on the parts of the cell which have text in them. So I rewrote large parts of this functionality by scouring StackOverflow and piecing information together, and fine-tuning the functionality with tons of experiments. The result are theEditableTableView
,EditableTextField
, andCellEditTracker
classes, withEditableTableViewDelegate
for callbacks. The solution still uses AppKit's Field Editor - it just provides its own control over when an edit session is started (usually double-click orEnter
), and provides support for navigation between editors viaTab
andEnter
, which seems to have been left half=complete by Apple.Table datasource rewrites
For the Key Bindings prefs page, I pulled all the code relating to the 2 tables out of
PrefKeyBindingViewController
and gave them three files each: (1) a table view controller, and a "datastore" for encapsulating the table's data and making atomized updates - which was originally one file, but when I added support forUndo
andRedo
, it made sense to divide the datastore into (2) the current "state" instance, and (3) its associated "state manager" which builds new states & handles undo/redo between states. More on that below.The tables were using Cocoa Bindings to map array data to table rows, but all these updates needed much finer grained control, and I ended up rewriting them using the
NSTableViewDataSource
method. It was a lot of upfront work, but the result should be a lot more intuitive: all the code is right there, so you know what it's doing, and when. And it should be easier to make minor changes to them in the future.Table animations
Trial and error, StackOverflow scouring, and a few complete rewrites later, the result is the
TableUIChange
class, in conjunction with theEditableTableView
class. For animations in a table which can have an arbitrary number of duplicate values, it's painful but necessary to keep track of exactly which rows changed, and how; my solution to this is theTableUIChange
class, which encapsulates the changes needed for a single "change" (which may include a single-line operation or a full-table reload).For tables where each row is guaranteed to be unique, such as the Configuration table, or for table animations which don't need to be 100% correct, I stumbled upon the amazing
Swift.Collection.difference()
method which computes a Git-style diff over two sets of data, but adapting it to anNSTableView
was non-trivial and there's need to credit Giles Hammond for use of hisRemainingRemovalTracker
class. It's kind of amazing that Apple didn't build this functionality intoNSTableView
already because it took a lot of work to pull it together.*TableState, *TableStateManager
Each table needs its own datasource to guarantee that any changes to its state are atomic and serialized, similar to a database. This is especially important for animations and undo/redo. But as noted in "Structs and State" above, state changes are regulated by making each "version" of the state into an immutable
struct
. For example, the current Key Bindings table state can be accessed by gettingBindingTableState.current
, and the state object contains all the methods needed to kick off a state change. Because it's immutable, its methods can access its state data and use it to compute changes without the danger of it being modified while being accessed. But the state object doesn't execute the actual changes; it submits them to its state manager.The state manager (e.g.
BindingTableStateManager
) is in charge of changing the current state (e.g.BindingTableState.current
), register for undo and store any data needed to do the undo (as well as carrying out any undoes), save the new state anywhere else that's needed, and notify the UI of the change viaTableUIChange
.Animated table data flow
For animations to work properly, and for data integrity generally, the data must always flow in one direction, and in a consistent order between components. Here's a summary of the Conf table flow.
ConfigTableViewController
has an action handler called. Checks & rejects it if not valid (e.g. can't edit a default config). If a file operation is requested, executes it, and if it fails, stops and spits out an error msg.ConfigTableState
) with the requested operation.ConfigTableState
prepares the changes needed, and then calls its state manager (ConfigTableStateManager
) to carry out the update.TableUIChangeBuilder.buildDiff()
to compare the updated table data with the previous table data, which will compile lists of row inserts, removes, updates, and moves which can later be used to animate the table changes. The resultingTableUIChange
object can be further customized to specify row selection or a completion handler to be run after the update.ConfigTableStateManager
stores the updated list of configs and the current config to the IINA preferences. It then setsConfigTableState.current
to the new state.BindingTableStateManager
is notified that an update is needed for its state. It triggers updates to the shared "default" section, which triggers a new build ofAppInputConfig
, which it waits for and uses to finally compute changes to its own table.NSTableView
can queryNSTableViewDataSource
for info on the data in a particular row, or the total number of rows, at seemingly arbitrary times during the redraw process. So it's important to update the backing data before executing the animation.TableUIChange
object to the table via an asynchronous notification.TableUIChange
when it receives the notification, which does the animations and updates the table's UI state.Feedback appreciated
powerplug.fill
for IINA plugins,applescript.fill
for Lua bindings,camera.filters
for saved filters, andmenubar.rectangle
to indicate a bound menu item. Also any of the above will be overridden withexclamationmark.circle
if another binding overrides it.input.conf
?