Fix monitor preferences

[alex-ds13]

This PR reworks the way the postload and the reload functions apply the monitor configs to the monitors. Previously it was looping through the monitor configs and applying them to the monitor with the index corresponding to the config's index.

However this isn't correct, since the user might set the preferred indices for 3 monitors (like monitor A, B and C), with the preferred index set to 0 for A, 1 for B and 2 for C, but if only monitors A and C are connected then komorebi would apply config 0 to A and config 1 to C, which is wrong it should be 2 for C.

This PR changes the way the configs are applied on those functions.
Now it loops through the existing monitors (already in order), then checks if the monitor has a preferred config index, if it does it uses that one, if it doesn't then it uses the first monitor config that isn't a preferred index for some other monitor and that hasn't been used yet.

For the situation above it means that it would still apply config 2 to monitor C. And in case there aren't any display_index_preferences set it will still apply the configs in order.

It also changes the way the layout is stored from the current monitor, by making sure to set it to None if the workspace is not tiling. Previously any floating workspace would be restored from cache with a tiling BSP layout.

Related to this discussion on Discord

However this entire code is based on the premise that the device_id is always the same for the same devices. However there have been comments on discord stating that this isn't true, however I've not been able to confirm this and the users have not given any proof of this happening (they might simply be confusing it since it's a weird string similar between monitors...)

I think the changes from this PR should fix most issues that people have been having with multiple monitors (specially with 3 or more monitors). But it's probably better to not merge this until we can get some users to test it.

[LGUG2Z]

However this entire code is based on the premise that the device_id is always the same for the same devices. However there have been comments on discord stating that this isn't true, however I've not been able to confirm this and the users have not given any proof of this happening (they might simply be confusing it since it's a weird string similar between monitors...)

Now that we have proof, I dug even further and found a hardware serial number ID which I managed to get out of WMI, I have a commit on a branch with this integrated into komorebi which I'll merge to master after this week's nightly tagged release goes out: 70a1280

This commit also updates the komorebic monitor-info command to return an ordered list with more info, including the serial number ID if it's available:

[
  {
    "id": 1804802517,
    "name": "DISPLAY1",
    "device": "DEL4310",
    "device_id": "DEL4310-5&1a6c0954&0&UID209155",
    "serial_number_id": "9SZGCP3",
    "size": {
      "left": 0,
      "top": 0,
      "right": 5120,
      "bottom": 2160
    }
  }
]

With what now looks to be a truly unique ID which persists across sessions and restarts (proof here on Discord?), the changes proposed in this PR should hopefully help stabilize monitor related madness once and for all 🤞

[alex-ds13]

@LGUG2Z What should we do now in regards to the display_index_preferences? Should we change it to take the serial_id instead of the device_id or should we deprecate that config to give a warning to the user and create a new one that uses the serial_id and make that one the preferred one to use going forward?!

[LGUG2Z]

Let's keep the same config option but attempt to match against both serial_id_number and device_id (as a fallback) for the next few versions before eventually phasing out device_id

[alex-ds13]

Let's keep the same config option but attempt to match against both serial_id_number and device_id (as a fallback) for the next few versions before eventually phasing out device_id

I've made the changes to allow using both serial_number_id or device_id on display_index_preferences. But I still need to think about the idea I have to fix all the multiple monitors issues. For once there are a few questions that might need to be answered first:

Imagine a setup with 3 monitors from left to right monitors A, B and C. If you set your display_index_preferences like this:

"display_index_preferences": {
  "0": "serial_number_A",
  "1": "serial_number_B",
  "2": "serial_number_C"
}

You expect each monitor to have the correct config from the map above even if one of them is not connected (this should be easy to fix, I hope...).

Now the hard part is, imagine that monitor B is disconnected (or went to sleep like some monitors can do) do you expect all commands that take a monitor index to obey the map above, or should monitor C now have index 1?
Like if you had some keybinds to send a window to monitor A, B or C using index 0, 1 and 2 respectively you probably still want that if you press the keybind to send a window to monitor C when monitor B is disconnected to work, but currently it won't, since after monitor B disconnects the window manager monitors will have only 2 monitors with indices 0 and 1, so index 2 doesn't exist.

Same thing for all workspace_rules and initial_workspace_rules, when they are saved they are saved with the monitor index from the vector enumeration, if you disconnect monitor B, now any rule for monitor B will get sent to monitor C instead (since that one will become index 1 which was the index of monitor B)

If the answer to all the questions above is that the indices shouldn't change and the commands should still work as expected and the rules should work as expected, then I fear my idea won't be enough and we'll have to change the monitors from a Vec to a HashMap I don't see many more possibilities here.

P.S. BTW my idea was to create a HashMap of user index (the ones on config) to actual monitors index. And this map would be updated when there was a disconnect or reconnect. This would be used for getting the correct config and the bar could also use it to apply to the correct monitors. But using this on every place like the commands that have a monitor index or all the events or rules that take a monitor index as well, even though it is possible, it would be quite a hassle and in that case it would probably be better to just change the Vec to an HashMap altogether...

@LGUG2Z Please tell me which idea do you think is better or if you have a better one?! I'm happy to try pursuing any idea. Also I can't guarantee that any of these ideas (even the change from Vec to HashMap) will actually be able to fix the issues with multiple monitors, but I sure can try!

[LGUG2Z]

I think what is in this PR is good to merge once we get a few people to sanity test it

I think we spoke last week on Discord about the idea of having users define possible monitor combinations, including specifying which combinations of monitors should be considered mutually exclusive. I think this idea is worth exploring more in the spirit of "make invalid states unrepresentable"

This is definitely a more long-term project, but take a look at what I've roughly doodled out here with comments:

{
  // workspaces are declared independently of monitors
  "workspaces": {
    // the key here is the name
    "primary_one": { "layout": "BSP" },
    // the value here is the same as the Workspace struct
    "primary_two": { "layout": "BSP" },
    "vertical_one": { "layout": "Rows" },
    "vertical_two": { "layout": "Rows" },
    "laptop_one": { "layout": "BSP" },
    "laptop_two": { "layout": "BSP" },
    "work_one": { "layout": "BSP" },
    "work_two": { "layout": "BSP" },
  },
  // locations encompass valid combinations of monitor serial ids
  "locations": {
    "work": {
      "monitors": {
        // workspaces are bound to monitors per-location
        "s333-laptop": {
          "workspaces": ["laptop_one", "laptop_two"],
        },
        // the key here is the serial_id
        "s444-work-external": {
          "workspaces": ["work_one", "work_two"],
        },
      },
      // valid monitor combinations must have their index preferences declared
      "combinations": [
        {
          "serials": ["s444-work-external", "s333-laptop"],
          "index_preferences": [0, 1],
        },
        {
          "serials": ["s444-work-external"],
          "index_preferences": [0],
        },
        {
          "serials": ["s333-laptop"],
          "index_preferences": [0],
        },
      ],
      // explicit behaviours for what to do when a display is disconnected
      "disconnect_behaviours": {
        "s333-laptop": {
          // not sure what the full range of actions here would be
          "SendWorkspacesTo": "s444-work-external",
        },
        "s444-work-external": {
          // maybe if the destination is also not available they can be stored
          // in a cache indexed on ws name
          "SendWorkspacesTo": "s333-laptop",
        },
      },
      // explicit behaviours for what to do when a display is reconnected
      "connect_behaviours": {
        "s444-work-external": {
          "RetrieveWorkspaces": ["work_one", "work_two"]
        }
        "s333-laptop": {
          "RetrieveWorkspaces": ["laptop_one", "laptop_two"],
        },
      },
    },
    "home": {
      "monitors": {
        "s111-ultrawide": {
          "workspaces": ["primary_one", "primary_two"],
        },
        "s222-vertical": {
          "workspaces": ["vertical_one", "vertical_two"],
        },
        "s333-laptop": {
          "workspaces": ["laptop_one", "laptop_two"],
        },
      },
      "combinations": [
        {
          "serials": ["s111-ultrawide", "s222-vertical", "s333-laptop"],
          "index_preferences": [0, 1, 2],
        },
        {
          "serials": ["s111-ultrawide", "s333-laptop"],
          "index_preferences": [0, 1],
        },
        {
          "serials": ["s111-ultrawide", "s222-vertical"],
          "index_preferences": [0, 1],
        },
        {
          "serials": ["s111-ultrawide"],
          "index_preferences": [0],
        },
        {
          "serials": ["s333-laptop"],
          "index_preferences": [0],
        },
      ],
      "disconnect_behaviours": {
        "s111-ultrawide": "Pause",
        "s222-vertical": {
          "SendWorkspacesTo": "s111-ultrawide",
        },
        "s333-laptop": {
          "SendWorkspacesTo": "s111-ultrawide",
        },
      },
      "connect_behaviours": {
        "s111-ultrawide": "Unpause",
        "s222-vertical": {
          "RetrieveWorkspaces": ["vertical_one", "vertical_two"],
        },
        "s333-laptop": {
          "RetrieveWorkspaces": ["laptop_one", "laptop_two"],
        },
      },
    },
  },
}

I think if people want to opt in to this kind of of advanced behaviour, komorebi should go all-in on requiring a fully declarative definition of what should happen in different scenarios; there is no way to satisfy what everyone's different definition of "it should just work" is, so instead we should make replicable configurations available both for users to be explicit about their expectations, and for developers to be able to validate and test.

[alex-ds13]

I think what is in this PR is good to merge once we get a few people to sanity test it

I think we spoke last week on Discord about the idea of having users define possible monitor combinations, including specifying which combinations of monitors should be considered mutually exclusive. I think this idea is worth exploring more in the spirit of "make invalid states unrepresentable"

This is definitely a more long-term project, but take a look at what I've roughly doodled out here with comments:

I think if people want to opt in to this kind of of advanced behaviour, komorebi should go all-in on requiring a fully declarative definition of what should happen in different scenarios; there is no way to satisfy what everyone's different definition of "it should just work" is, so instead we should make replicable configurations available both for users to be explicit about their expectations, and for developers to be able to validate and test.

This is interesting, but I find it would be too much and too confusing for most users.

As for this PR, currently it doesn't fix much. It simply changes the display_index_preferences to take both serial and device ids and makes the monitor reconciliator and the static config postload and reload able to handle both as well. And the only fix it actually does is to make sure that the specified config is actually the one used for the wanted monitor.

But I feel that most issues that people with multiple monitors are facing are actually related to the issues mentioned on my comment above where the indices of commands and rules get mixed up when some monitor goes to sleep.

I guess we can look into those issues in another PR and keep this one just as "apply monitor config preferences correctly"...

[alex-ds13]

@LGUG2Z So I've added a few things more with the last 3 commits:

• Now the workspace_rules intended for a monitor that gets disconnected will be removed, so that the WindowManager doesn't try to send them to the wrong monitor.
• I've created a map of indices which maps the user intended index (from config's display_index_preferences) to the actual index that that monitor has on the WindowManager ring of monitors. This map is always recreated by the load_monitor_information function which happens on start and on monitor disconnect/reconnect. (currently this map is only being used by the komorebi-bar)
• The komorebi-bar now detects if it's monitor has been disconnect and sets a new field (disabled) so that the EGUI update function returns early without drawing anything at all. It also detects the reconnection of the monitor to re-enable the bar again and start showing up on the re-connected monitor. (This was tested by me and seems to be working fine!)
• The komorebi-bar now uses the new monitor_usr_idx_map to get the actual index of the monitor that the bar should be drawn to. This should make it so, in cases where a user has a setup with 3 monitors, like A, B and C, if they disconnect the monitor B (or it goes to sleep), the bar for monitor B should be disabled and stop rendering and the bar from monitor C should get it's monitor_index internally changed from 2 to 1 so that it is able to get the proper info from monitor C like it was intended to do! (This I have only tested with 2 monitors and the bar seems to be disabled and re-enabled has expected, however the situation with 3 or more monitors I've not been able to test since I don't have a 3rd monitor, would really like some feedback on that before setting this PR as ready to merge...)