Don't reload the UI when the keyboard layout changes

[zadjii-msft]

Alrighty, this is a silly solution to the problem. Go read #11522.

What if, we just didn't reload the UI for a settings reload?

Terminal refreshes the appearance of panes, profiles, tabs, etc, in TerminalPage::_RefreshUIForSettingsReload. That's only called in TerminalPage::SetSettings, when we specify that we didn't need to reload the UI. Currently, we only don't reload the UI on an initial load of the settings (that makes sense, there's no UI to "reload" yet.)

So what if we did all of TerminalPage::SetSettings, but without the UI reload? We update the keybindings, the command palette, the current language, but not the actual content of the tabs.

before

after

Closes #11522?

---

A caveat from discussion:

This might have some weird irreproducible bugs in it. Consider

1. Open two terminal windows
2. Install a WSL distro
3. Switch the KB layout

Both these windows will reload the settings, find that the new settings (& dynamic profile) are different, and will try to write the settings to the disk. One will win? Maybe? Both will probably end up with a full UI refresh, for determining that the settings file changed.

But is this fine anyways? We're pretty sure there's a backlog item for "reload the settings on a distro install" anyways. And the value add of this fix is probably worth more than the very rare new edge case where this still happens.

[DHowett]

You know, one of the things that weirds me out about this is that we don't need to reload the settings at all... just re-apply their effects.

Would it be possible to only reprocess the key bindings from the original model objects? That would address the bulk of the issue and make for a more targeted fix...

[zadjii-msft]

Would it be possible to only reprocess the key bindings from the original model objects?

@lhecker might need to back me up on this - could we just call ActionMap::_RefreshKeyBindingCaches(), and have that reload the keybindings for the new kbd layout? I assumed that we couldn't do it trivially - I guess I assumed the layering effects would be lost by this point, and we'd need to re-parse the settings to get the right layering.

Now that I'm re-reading ActionMap::_PopulateKeyBindingMapWithStandardCommands, that seems like it'd work without a whole reparse

[lhecker]

_RefreshKeyBindingCaches() is not the most important thing that needs to be reloaded when the keyboard layout changes. The KeyChord instances need to be refreshed, as they translate scan-codes to vkeys via MapVirtualKeyW and those are then used for the Hash() they return. In other words, yeah this is 100% doable, but it needs a bit more work than just calling _RefreshKeyBindingCaches().

[zadjii-msft]

Hmm. Updating the KeyChords seems... hard. The ctor will evaluate the SC->VK on construction, and store that the same as if we parsed the VK directly. So we can't just make a new _KeyMap map, take all the old KeyChords and just re-evaluate them.

Not sure what there is to do here. I can try to look if there's a way to build a fresh Settings object and just steal the actions map? Maybe that would work?

[zadjii-msft]

I could theoretically make changes to AppLogic::ReloadSettings:

• AppLogic::_TryLoadSettings returns out the new Settings rather than placing it in _settings.
• then, it would:

  if (SUCCEEDED(_settingsLoadedResult) && keybindingsOnly) {
    _settings.CopyActionsFrom(newSettingsObject);
    _page->UpdateKeyboardSettings()
    return;
  }
  // the rest of the handling
  

  • CopyActionsFrom(other) does basically what it would sound like, just replace the globals.ActionsMap with the new one
  • UpdateKeyboardSettings would just _UpdateCommandsForPalette(); CommandPalette().SetActionMap(_settings.ActionMap()); to update any labels, etc.

Does that seem like it's sensible? How egregious of a layering violation does that feel like?

[DHowett]

I think this is the problem:

The ctor will evaluate the SC->VK on construction

The thing consuming the settings model object here should be on the hook for this.

If we made that change, would it be easier to do "reevaluate all SC/VK" as a follow-on?

[DHowett]

(Feel free to veto me)

[zadjii-msft]

This is super stale. Like 1.5 years is too long, and there have been major rewrites since. We need to surely just start fresh