Skip to content

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

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Jan 5, 2023


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.

  • (MacOS 10.15+) Row changes are animated! This includes adds, removes, moves, and filtering rows via the Search box.
  • Add Drag & drop support to both tables:
  • Drag to reorder rows in the Bindings table, or hold down Option while dragging to drop copies of them in the desired location.
  • Rows from the Bindings table can also be dragged into a text editor and they will be exported there in mpv's input.conf format.
  • Copy rows from one configuration to another by dragging them from the Key Bindings table into a row in the Conf table.
  • Drag a config from Conf table to a Finder window to copy it there.
  • Drag config files from Finder into Conf table to import them.
  • Added support for Undo and Redo in both tables (any changes to any of the items in either table, and also the act of changing the current configuration). The Undo & 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 the UndoManger 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.]
  • Navigation via 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 the Settings 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

  • Configuration table now stays sorted. Built-in configs are listed first in fixed order, and user configs are listed afterwards in alphabetic order.
  • Added support for Edit menu operations (Copy, Paste, Delete) (although not Cut) and their key equivalents. Paste bindings anywhere in the same conf file, or into other conf files. Or paste into a text file.
  • Double-click on a row in the Configs table to rename the config file via an in-line editor.
  • The + 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.
  • Duplicate a config file inline, instead of via a dialog. It mimics the Finder's format ("My File copy.conf", "My File copy 2.conf", etc.) to create a new uniquely named file, but shows an editor to allow the user to immediately change it.
  • Ctrl-click or right-click on a configuration brings up a context menu with the above actions.

Key Bindings table

  • Bindings now indicate when they are disabled due to conflicts. Disabled bindings are colored in red strikethrough text, with a red warning icon.
  • Bindings which are menu items now have a special menu icon (note: better icons would be great)
  • Bindings set from places other than conf files (e.g. from IINA plugins or Lua scripts) are also displayed in the table so that users can see everything in one place and sources of conflicts are obvious at a glance. These "non-conf" bindings are updated in real-time and are displayed differently (currently I settled on the highlight color) and indicate clearly where they came from, but are also not editable or selectable.
  • Added support for Edit menu operations (Cut, Copy, Paste, Delete) and their key equivalents. Paste bindings anywhere in the same conf file, or into other conf files. Or paste into a text file.
  • Right-click/Ctrl-click on a configuration brings up a context menu with basic useful actions (loosely modelled after Google Sheets)
  • If Display raw values is checked, in-line editing is used:
  • Double-click on a cell to edit its contents directly.
  • While in-line editing, use Tab, Shift+Tab, Return or Enter keys to quickly save each edit & navigate between cells, or ESC to cancel changes.
  • Adding a new row creates a new empty row and opens an in-line editor. (The user can create as many empty rows as they like, but blank lines will not be saved when the user closes the window or changes the current config, unless the user enters values into them.)
  • Added formatting for key sequences: they should up as a comma-separated list when Display raw values is unchecked.
  • Column width for the Key column is now saved across launches.

New backend logic

  1. The traditional configuration (*.conf file)
  2. Menu item key equivalents set by IINA plugins
  3. Menu item key equivalents for toggling saved video & audio filters
  4. Player bindings set at runtime via libmpv's mp.add_key_binding, mp.add_forced_key_binding, and mp.remove_key_binding, most commonly by Lua scripts
  • The update establishes a subsystem which all bindings flow through, which enforces priorities and ensures consistent resolution between conflicts and predictable behavior - and allows them all to be displayed in the same Key Bindings table.

Binding-candidate-assembly

Backend 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:

  1. Bindings lower down the final list have higher priority (they are "stronger") than items higher up in the list;
  2. If more than one binding is defined with the same key, the binding with the highest priority will be chosen and all others ignored;
  3. The list is built from smaller lists called input sections (or just called sections). To build it, iterate over each of the sections from oldest to newest, and:
  • If the section is strong, append its bindings to the highest-priority end of the list, or
  • If the section is weak, prepend its bindings one-by-one to the lowest-priority, opposite end (e.g., the start) of the list.

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.

System Design

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:

    1. The current input conf file, as set in Preferences > Key Bindings, is application-wide.
    2. So are shortcuts for audio & video filters, as set via Video > Video Filters…, and Audio > Audio Filters… - but should these be prioritized above the config file's bindings, or below?
    3. Dynamic bindings set/unset by Lua scripts are specific to a single mpv core, and must be arranged into a section stack in order to work properly. They are typically defined as being either higher or lower priority than the bindings from the user's config file.
    4. IINA plugin menu items are set by plugin developers: it looks like they can be either application-wide or player-specific. And [some clarification needed] it looks like they are intended to only be enabled if they don't conflict with existing bindings.

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 class KeyMapping is intended to be a struct "in spirit" but had to be a class which inherited from NSObject in order to be clipboard-representable.

Core design

PlayerInputConfig

  • At a given time, only one list of InputBindings is in effect, because only one window can have focus at a time.
  • But (at present) we don't care about bindings when the Welcome window or Preferences window is in focus. The only focus change that matters is the "active player" (or "last active player" if there are no player windows open, or if some other lesser window is in focus).
  • Some bindings (well, the dynamic bindings coming from mpv) require each player to have a stack of input sections, while other bindings are application-wide. I found what I think is a simple enough solution which reconciles the two scopes.
  • Each player keeps its own stack of sections like mpv demands (an InputSectionStack), and when a given player is active, all of the current bindings will be determined from that player's stack.
  • But in every player's stack we will include static "shared" sections for those application-wide bindings. The player doesn't treat the shared section differently than any of its other sections. But the shared section can be altered from other parts of the application without needing to interact with any specific player. A change to one of the shared sections will take effect for all players, while a change to any of its non-shared sections will be confined to the player itself.
  • The InputSectionStack is itself contained within a PlayerInputConfig instance, which is attached to a single PlayerCore instance.
  • The idea of 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

  • Although a player's 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.
  • Then the set of enabled keys is assembled into a dictionary for efficient lookup, which is the end goal of all of this. But it's useful to hold on to the candidate list because it will be shown in the Key Bindings table, which will make it a lot easier to understand what's going on.
  • Both the lookup dictionary, and the list of candidate bindings are encapsulated in an instance of AppInputConfig.
  • When the focus moves from one player to another, or there is a change to one of the active player's input sections, a new instance of 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.
  • The static variable AppInputConfig.current holds the most authoritative, up-to-date version, which will be consulted to do key lookups for user input.
  • The factory method 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:
  • The focus changed from one player to another, or
  • Something or someone made a change to any of the InputSections in the current active player, including adding, removing, or changing bindings; or adding, removing, or replacing a section.
  • The logic for assembling an InputSectionStack into an AppInputConfig is encapsulated in the class AppInputConfigBuilder.
  • The bindingCandidateList field of an AppInputConfig is the same list of bindings that is shown in the Key Bindings table in in the UI, and its resolverDict serves as the primary dictionary for key lookups in the player window.

InputSectionStack

Bindings are grouped into InputSections, An InputSection 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 a PlayerInputConfig, which includes an InputSectionStack. The first 4 sections in every InputSectionStack will be the following, which are shared for every player:

// bottom of stack
default       (strong)
Video Filters (strong)
Audio Filters (strong)
IINA Plugins  (weak)
// top of stack

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’s InputSectionStack 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:

  1. the end of the list if its section is “strong”, or
  2. the front of the list if its section is “weak”.

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 for Shift+w when it starts up. When the user presses it, an OSD is displayed and additional bindings are set. One of those bindings is ESC, and if the user presses that, the OSD is closed and mp.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).

Dynamic-binding-example

Frontend notes

Inline table editing

Apple added a built-in cell editor and double-click action handling (doubleAction) to NSTableView 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 the EditableTableView, EditableTextField, and CellEditTracker classes, with EditableTableViewDelegate 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 or Enter), and provides support for navigation between editors via Tab and Enter, 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 for Undo and Redo, 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 the EditableTableView 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 the TableUIChange 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 an NSTableView was non-trivial and there's need to credit Giles Hammond for use of his RemainingRemovalTracker class. It's kind of amazing that Apple didn't build this functionality into NSTableView 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 getting BindingTableState.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 via TableUIChange.

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.

  1. User executes an action on the table.
  2. 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.
  3. The controller then calls a method on the table's state (in this case, ConfigTableState) with the requested operation. ConfigTableState prepares the changes needed, and then calls its state manager (ConfigTableStateManager) to carry out the update.
  4. The state manager, having both "current" and "new" states, is able to do the following:
  5. Register for an undo operation. If the user chooses to "undo", all that's needed is to swap the states so that "new" is the current state, and what was "current" will then be "new".
  6. Compute the table animation. 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 resulting TableUIChange object can be further customized to specify row selection or a completion handler to be run after the update.
  7. Commits / saves the necessary changes to the data. For this table, ConfigTableStateManager stores the updated list of configs and the current config to the IINA preferences. It then sets ConfigTableState.current to the new state.
  8. Now's also a good time to notify other components which are interested in the changes. In this case, if the table selection changes (i.e. a new config file is selected), a new sequence is kicked off where the new file is loaded and BindingTableStateManager is notified that an update is needed for its state. It triggers updates to the shared "default" section, which triggers a new build of AppInputConfig, which it waits for and uses to finally compute changes to its own table.
  9. At this point, the config table's appearance is not yet updated to include the changes, but the state has been. It's important to remember that the NSTableView can query NSTableViewDataSource 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.
  10. The datastore finally sends the TableUIChange object to the table via an asynchronous notification.
  11. The table executes the TableUIChange when it receives the notification, which does the animations and updates the table's UI state.

Feedback appreciated

  • Need to find custom icons to use for Key Bindings items to replace SF Symbols. They aren't available for older versions of MacOS, and they're also not the greatest. I'm currently using powerplug.fill for IINA plugins, applescript.fill for Lua bindings, camera.filters for saved filters, and menubar.rectangle to indicate a bound menu item. Also any of the above will be overridden with exclamationmark.circle if another binding overrides it.
  • Maybe different colors, and/or style changes? With the new color in the Key Bindings table, I felt like I needed to add some to the Configs table because it was starting to look drab. But I'm not happy that the current colors are a little bit off compared to the tab icons on the left, but it's hard to match with those because they are using an alpha blend with whatever's behind the window, and aren't 100% constant.
  • Needs testing with more Lua scripts to know for sure that everything is behaving like it would in mpv.
  • Should shortcuts for video & audio filters be editable from the Key Bindings Preferences? Should they become another entry in input.conf?

@svobs svobs force-pushed the pr-snowball branch 2 times, most recently from 3ee008f to 2390dce Compare January 7, 2023 02:15
@svobs
Copy link
Contributor Author

svobs commented Jan 29, 2023

Updated: fixed an issue where trying to load a config file which didn't exist could result in infinite table refresh

@svobs
Copy link
Contributor Author

svobs commented Jan 31, 2023

Fixed a minor bug which caused in-line editor to be opened at the same time popup editor was opened

svobs added 4 commits March 16, 2023 01:45
…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
…o their defaults after their key bindings were removed.
svobs added 2 commits March 17, 2023 02:42
…acters (and also case handling in general!), and also fix errors handling '#' and '+'
@konkrotte
Copy link

Is this abandoned?

@svobs
Copy link
Contributor Author

svobs commented Feb 27, 2024

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.

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.

2 participants