Skip to content
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

services.aerospace.settings.on-window-detected renders TOML incorrectly #1271

Closed
bassforce86 opened this issue Jan 15, 2025 · 5 comments
Closed

Comments

@bassforce86
Copy link

bassforce86 commented Jan 15, 2025

There was an MR that made some great headway on making this more "nix" friendly #1208. However the final formatted toml doesn't quite fit the spec defined in the aerospace docs.

Related to: #1142

If I was to guess, this might be a small oversight in the filterAttrsRecursive fn that doesn't capture that the "if" attributes should be inlined within the current object instead of creating a new set.

Nix settings

on-window-detected = [
  {
    "if".app-name-regex-substring = "finder";
    run = "layout floating";
  }
  {
    "if".app-id = "com.mitchellh.ghostty";
    run = "move-node-to-workspace 1";
  }
  {
    "if".app-id = "com.google.Chrome";
    run = "move-node-to-workspace 2";
  }
  {
    "if".app-id = "com.tinyspeck.slackmacgap";
    run = "move-node-to-workspace 3";
  }
  {
    "if".app-id = "com.apple.mail";
    run = "move-node-to-workspace 5";
  }
  {
    "if".app-id = "us.zoom.xos";
    run = "move-node-to-workspace 6";
  }
  {
    "if".app-id = "net.whatsapp.WhatsApp";
    run = "move-node-to-workspace 7";
  }
  {
    "if".app-id = "com.cisco.secureclient.gui";
    run = "move-node-to-workspace 8";
  }
];

Expected:

[[on-window-detected]]
if.app-name-regex-substring = "finder"
run = "layout floating"

[[on-window-detected]]
if.app-id = "com.mitchellh.ghostty"
run = "move-node-to-workspace 1"

[[on-window-detected]]
if.app-id = "com.google.Chrome"
run = "move-node-to-workspace 2"

[[on-window-detected]]
if.app-id = "com.tinyspeck.slackmacgap"
run = "move-node-to-workspace 3"

[[on-window-detected]]
if.app-id = "com.apple.mail"
run = "move-node-to-workspace 5"

[[on-window-detected]]
if.app-id = "us.zoom.xos"
run = "move-node-to-workspace 6"

[[on-window-detected]]
if.app-id = "net.whatsapp.WhatsApp"
run = "move-node-to-workspace 7"

[[on-window-detected]]
if.app-id = "com.cisco.secureclient.gui"
run = "move-node-to-workspace 8"

Actual

[[on-window-detected]]
run = "layout floating"

[on-window-detected.if]
app-name-regex-substring = "finder"

[[on-window-detected]]
run = "move-node-to-workspace 1"

[on-window-detected.if]
app-id = "com.mitchellh.ghostty"

[[on-window-detected]]
run = "move-node-to-workspace 2"

[on-window-detected.if]
app-id = "com.google.Chrome"

[[on-window-detected]]
run = "move-node-to-workspace 3"

[on-window-detected.if]
app-id = "com.tinyspeck.slackmacgap"

[[on-window-detected]]
run = "move-node-to-workspace 5"

[on-window-detected.if]
app-id = "com.apple.mail"

[[on-window-detected]]
run = "move-node-to-workspace 6"

[on-window-detected.if]
app-id = "us.zoom.xos"

[[on-window-detected]]
run = "move-node-to-workspace 7"

[on-window-detected.if]
app-id = "net.whatsapp.WhatsApp"

[[on-window-detected]]
run = "move-node-to-workspace 8"

[on-window-detected.if]
app-id = "com.cisco.secureclient.gui"
@thuvasooriya
Copy link
Contributor

thuvasooriya commented Jan 15, 2025

@bassforce86 afaik this doesn't affect the aerospace behavior at the end right?

I noticed this as well during testing. but since aerospace seems to process this well and produce the same behavior, i assumed they were functionally identical.

If there were some edge cases where this fails behaviorally then please let me know, that would be worth investigating.

my two cents on why we couldn't match the documented version.

The end result depends on how nix translates the inherent config data structure we create into TOML, afaik we have no control over it other than creating a custom TOML generator in nix. and as mentioned in #1215 (comment) there are some inherent issues in conversion which we cleverly bypass through usage of strings.

Some solutions

  1. Write a custom TOML generator (is it worth it to make it look like docs for a generated file?)
  2. change the if keyword to something else in the docs (kinda far fetched)
  3. one i use which is just importing TOML as config as mentioned here (cause i'm an ocd person too) services.aerospace.settings.on-window-detected has the wrong type #1142 (comment)

edit:
and afaik filterAttrsRecursive was implemented only after solid types were introduced to sort out the null problem and stop introducing blank config lines without arguments (which actually affected aerospace behavior). So it shouldn't affect how the final toml is rendered. I might be missing something if so please enlighten me.

pinging @z0al here for fact checking filterAttrsRecursive implementation

@bassforce86
Copy link
Author

bassforce86 commented Jan 15, 2025

It does functionally work, but looking over how TOML works (outside of aerospace as it uses a toml parser itself) These "if" tables aren't actually supposed to be valid. It's more by luck than design that it works, duplicate table name are only support in table arrays e.g:

[[table]] 

[[table]]

is valid and will parse deterministically

[table]

[table]

is discouraged and (according to the spec) should actually produce errors.


Just a note on the filter fn, My nix knowledge is nowhere near yours so please don't take my guess as criticism, it really is just a guess based on the output!

@z0al
Copy link
Contributor

z0al commented Jan 15, 2025

this might be a small oversight in the filterAttrsRecursive fn that doesn't capture that the "if" attributes should be inlined

This doesn't have anything to do with the filtering logic as it's there only to remove null values and doesn't alter the TOML output.

The formatting of the TOML output is handled by the json2toml CLI used in nixpkgs.

@bassforce86 If we convert your expected TOML output to JSON we get this:

Expand for JSON output
{
  "on-window-detected": [
    {
      "if": {
        "app-name-regex-substring": "finder"
      },
      "run": "layout floating"
    },
    {
      "if": {
        "app-id": "com.mitchellh.ghostty"
      },
      "run": "move-node-to-workspace 1"
    },
    {
      "if": {
        "app-id": "com.google.Chrome"
      },
      "run": "move-node-to-workspace 2"
    },
    {
      "if": {
        "app-id": "com.tinyspeck.slackmacgap"
      },
      "run": "move-node-to-workspace 3"
    },
    {
      "if": {
        "app-id": "com.apple.mail"
      },
      "run": "move-node-to-workspace 5"
    },
    {
      "if": {
        "app-id": "us.zoom.xos"
      },
      "run": "move-node-to-workspace 6"
    },
    {
      "if": {
        "app-id": "net.whatsapp.WhatsApp"
      },
      "run": "move-node-to-workspace 7"
    },
    {
      "if": {
        "app-id": "com.cisco.secureclient.gui"
      },
      "run": "move-node-to-workspace 8"
    }
  ]
}

I think we can agree that the JSON output is correct semantically.

Next let's save that JSON to a file and call json2toml

nix-shell -p remarshal
json2toml output.json output.toml

I get something identical to the actual output you got which confirms it's how json2toml chooses to format the output:

Expand for output
[[on-window-detected]]
run = "layout floating"

[on-window-detected.if]
app-name-regex-substring = "finder"

[[on-window-detected]]
run = "move-node-to-workspace 1"

[on-window-detected.if]
app-id = "com.mitchellh.ghostty"

[[on-window-detected]]
run = "move-node-to-workspace 2"

[on-window-detected.if]
app-id = "com.google.Chrome"

[[on-window-detected]]
run = "move-node-to-workspace 3"

[on-window-detected.if]
app-id = "com.tinyspeck.slackmacgap"

[[on-window-detected]]
run = "move-node-to-workspace 5"

[on-window-detected.if]
app-id = "com.apple.mail"

[[on-window-detected]]
run = "move-node-to-workspace 6"

[on-window-detected.if]
app-id = "us.zoom.xos"

[[on-window-detected]]
run = "move-node-to-workspace 7"

[on-window-detected.if]
app-id = "net.whatsapp.WhatsApp"

[[on-window-detected]]
run = "move-node-to-workspace 8"

[on-window-detected.if]
app-id = "com.cisco.secureclient.gui"

These "if" tables aren't actually supposed to be valid.

TOML syntax is too complex for my small brain so I never read the spec and probably never will :)

However, pasting the output to TOML Lint doesn't report any issue so there is that.

@thuvasooriya
Copy link
Contributor

thuvasooriya commented Jan 16, 2025

Thankyou @bassforce86 for the info about the specs and Thankyou @z0al for pointing out the actual reason. Since the existing implementation is behaviorally valid for aerospace and TOML linters and formatters see no problem with it, (and as aerospace is still in beta and the config might change in the future as well) I think this is not worth the effort.

I'd say that this is a nix issue (because of the json2toml) and should be tackled there if it actually is not according to specs.

takeaway for the brave who are willing to tackle this:

  • you might want to look into how json2toml in nix handles "if" (since we cannot use if directly due to it being a reserved keyword in nix, we use a string) and try fixing it there.

@bassforce86
Copy link
Author

Thank you both for your investigations and insight! I might have a look into the json2toml parser and see whether we can apply some configuration to create an exception for the "if" sets. But given the time this has already taken away from you both - AND given that it does functionally work, I think we can probably close this off as a no-op, certainly for nix-darwin.

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

No branches or pull requests

3 participants