-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
Comments
@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
edit: pinging @z0al here for fact checking |
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! |
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 @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
I get something identical to the actual output you got which confirms it's how 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"
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. |
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:
|
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. |
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
Expected:
Actual
The text was updated successfully, but these errors were encountered: