aerospace: add `workspace-to-monitor-force-assignment` option and fix `on-window-detected` type #1208

thuvasooriya · 2024-12-02T02:04:58Z

trying to fix #1142

this might be redundant or even extra since i found out later that we already have #1168
fixes the error for most of my use cases not sure if there are any other issues.

let me know if there is some changes that i should make for this to be merged.

trying to fix LnL7#1142

thuvasooriya · 2024-12-02T02:41:05Z

This one solves the issue #1142 and implements [workspace-to-monitor-force-assignment]
@Enzime

Enzime

Can you remove all the formatting changes?

Can you update the tests as well?

Enzime · 2024-12-03T03:28:49Z

tests/services-aerospace.nix

@@ -31,6 +31,6 @@ in
      ${config.out}/user/Library/LaunchAgents/org.nixos.aerospace.plist`



Can you update the settings above and use the new option you added and set on-window-detected maybe with the example value?

Enzime · 2024-12-03T03:30:49Z

tests/services-aerospace.nix

@@ -31,6 +31,6 @@ in
      ${config.out}/user/Library/LaunchAgents/org.nixos.aerospace.plist`

    echo >&2 "checking config in $conf"
-    if [ `cat $conf | wc -l` -eq "27" ]; then echo "aerospace.toml config correctly contains 27 lines"; else return 1; fi
+    if [ `cat $conf | wc -l` -eq "29" ]; then echo "aerospace.toml config correctly contains 29 lines"; else return 1; fi


Can you replace this check with something that actually checks the contents of $conf to see that it is setting the settings correctly like:

nix-darwin/tests/services-skhd.nix

Line 20 in c6b65d9

grep "alt + shift - r : chunkc quit" ${config.out}/etc/skhdrc

i was doubtful of the initial implementation of the testing as well, unfortunately I didn't know much about it, I'll try this weekend to replicate the skhd one.

z0al · 2024-12-05T19:23:03Z

Works locally for me. Thanks @thuvasooriya for taking care of this

z0al · 2024-12-05T19:24:53Z

modules/services/aerospace/default.nix

@@ -72,9 +72,37 @@ in
              description = "Default orientation for the root container.";
            };
            on-window-detected = lib.mkOption {
-              type = listOf str;
+              type = types.listOf (types.attrsOf types.anything);


Is there any reason for types.anything instead of a proper sub module?

I did try to come up with something more restricted (not much really) in #1168 but don't have much knowledge about aerospace and possible values of this toml table so we kind of backed up to accept everything. Just leaving it here in case we want to play with it a bit.

callbackConf = with lib.types; oneOf [ bool int float str path attrs ] // { description = "AeroSpace config callback (bool, int, float, str, path or attrs)"; }; ... on-window-detected = lib.mkOption { type = listOf (attrsOf (either callbackConf (listOf callbackConf))); ...

don't have much knowledge about aerospace and possible values of this toml table

Looking at the code & docs I believe:
1.run command is an array of strings and it's required.
2. if.app-id, if.workspace, if.window-title-regex-substring and if.app-name-regex-substring are all optional and of type string
3. if.during-aerospace-startup is a boolean and optional
4. check-further-callbacks is optional and of type boolean

I will leave it to @thuvasooriya if they want to add proper typing here. Otherwise, I can follow up with a PR once this is merged.

Thankyou for the valuable PR @talhaHavadar, and @z0al for taking the time to go through the docs. This was my first Nix contribution ever so I didn't want to mess around too much.
My initial solution before I saw #1168 was

type = listOf (attrsOf (oneOf [ bool str attrs ]));

which was not rigid in terms of types but then I changed it according to the suggestion made by @Enzime in #1168.
I'll see if I can come up with anything better. Thanks again.

thuvasooriya@49c1990
~~let me know if this can be merged to this PR @Enzime @z0al~~
Tried to whip something up from the above doc description, ~~not sure if it handles all the edge cases. but works with the test cases and my config~~

edit: submodule types cause some issues with aerospace on-window-detected due to null values not available in TOML, the redundant default values of other if fields are conflicting or rather manipulating the functionality. Further testing and information about the actual types and handling of fields in aerospace is required to implement this properly.

I think this PR is in a good working condition. I think @z0al can try to crack the proper types when time allows. Not sure how much of a help this thuvasooriya@49c1990 can be but hope it helps.

@Enzime let me know if any further changes are required to this PR. Thanks.

You can try to filter the config of null values before passing the settings totoml.generate

This is what i was looking for.. Thx!
I was going to do this but then thought, maybe I'm just dumb and a better way exists in nix. Guess this is the way to go.

I'm a nix noob myself but that's what I see being done in nixpkgs.

unfortunately replicating the filter is somewhat troublesome for me.
my guess is that the list of submodules is not playing nicely with filterAttrs.
I tried the following and still get the above error: cannot convert data to TOML,
meaning the null is not filtered.

I tried the default functions, filterAttrs, filterAttrsRecursive
Then I tried to implement a custom function to iterate over each item in the submodule list. Which doesn't work either. Maybe there is some obvious mistake that I'm overlooking.
Any help or direction would be appreciated. @Enzime @z0al

filterOptionalFields = value: if lib.isAttrs value then let filteredAttrs = lib.mapAttrs (_: v: filterOptionalFields v) value; nonNullAttrs = lib.filterAttrs (_: v: v != null) value; in filteredAttrs // nonNullAttrs else if lib.isList value then lib.filter (x: x != null) (map filterOptionalFields value) else value; # filterOptionalFields = lib.filterAttrsRecursive (_: v: v != null); configFile = format.generate "aerospace.toml" (filterOptionalFields cfg.settings);

Ah, I see. My bad. I didn't know that filterAttrsRecursive doesn't handle the case of lists.

Here is a custom implementation that works. I took the original filterAttrsRecursive code and added a condition for lists + changed the predicate signature to accept only values.

# Code needs proper formatting filterAttrsRecursive = pred: set: lib.listToAttrs ( lib.concatMap (name: let v = set.${name}; in if pred v then [ (lib.nameValuePair name ( if lib.isAttrs v then filterAttrsRecursive pred v else if lib.isList v then ( map (i: if lib.isAttrs i then filterAttrsRecursive pred i else i) (lib.filter pred v) ) else v )) ] else [ ] ) (lib.attrNames set) ); filterNulls = filterAttrsRecursive (v: v != null); configFile = format.generate "aerospace.toml" (filterNulls cfg.settings);

Let's hope that Enzime has a trick up their sleeve so we don't have to keep this custom function :)

P.S. Don't forget to change all optional attributes to default to null even things like check-further-callbacks as Aerospace behaves differently when it's set to false vs left out.

cool! it works! thanks.
merged the changes, and yes every optional is set to null by default.

cleanup n if fiz checking final

z0al · 2024-12-06T21:22:36Z

tests/services-aerospace.nix

+    grep 'check-further-callbacks = false' $conf
+    grep 'run = "move-node-to-workspace m"' $conf
+    grep 'app-id = "Another.Cool.App"' $conf
+    grep 'during-aerospace-startup = false' $conf


It would be great if you also check that unspecified options don't appear in the config e.g. the window-title-regex-substring.

I don't think default null values have a way of somehow entering TOML files, since they have the concept of null = omitted and all. Hence I'm not sure how to feasibly test that in this specific implementation. maybe we check that check-further-callbacks = false is not present before the line run = "layout floating" just for the example test case made. and for the other string optionals the what string values should be considered an exception when taken as default since we are not providing "".

I could just check window-title-regex-substring and workspace doesn't exist in the whole config in this specific example but that seems kind of misleading isn't it.

Maybe I'm missing something, further direction on how should I implement this would be great. Thanks again.

I think it's fine to do a negative assertion for fields that don't show up in the config file, you can also be explicit about the value by setting them to null intentionally rather than relying on the default value of the option

You can see how I did them here: fd6660c

thanks for the hint!
I'll try to tackle that when I get some time.

thuvasooriya · 2024-12-17T13:37:41Z

@Enzime just made a small change to detect window-title-regex-substring not being present, and added example configs for all the other callback examples. if we have to do this properly then we should check how the whole file is output in order to check the presence or absence of certain troublemakers.

This works flawless for now. Let me know if you want anything else for this to be merged.

thuvasooriya added 3 commits December 2, 2024 07:28

fix

c38f61a

trying to fix LnL7#1142

testing requested changes

a07067c

adding workspace to monitor force assignment

a8d66d7

Enzime requested changes Dec 2, 2024

View reviewed changes

Enzime changed the title ~~fix #1142~~ aerospace: add workspace-to-monitor-force-assignment option and fix on-window-detected type Dec 2, 2024

thuvasooriya added 2 commits December 2, 2024 12:05

remove formatting

c38aa86

tests pass

2d1035a

Enzime requested changes Dec 3, 2024

View reviewed changes

z0al mentioned this pull request Dec 5, 2024

fix(aerospace): remove unusable on-window-detected option #1215

Closed

thuvasooriya added 4 commits December 5, 2024 20:55

proper tests

4684eb3

Merge branch 'master'

78e9b7a

undo formatting

7648a84

tests for on-window-detected and workspace-to-monitor-force-assignment

cea6074

z0al reviewed Dec 5, 2024

View reviewed changes

z0al mentioned this pull request Dec 5, 2024

aerospace: fix on-window-detected #1168

Open

thuvasooriya added 3 commits December 6, 2024 07:37

testing submodules

49c1990

cleanup n if fiz checking final

toml null field aerospace callback issue

206cafa

custom null filter for submodule list

89fb96c

z0al reviewed Dec 6, 2024

View reviewed changes

check for no presense of window-regex and if.workspace config check

9c52a30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aerospace: add `workspace-to-monitor-force-assignment` option and fix `on-window-detected` type #1208

aerospace: add `workspace-to-monitor-force-assignment` option and fix `on-window-detected` type #1208

thuvasooriya commented Dec 2, 2024

thuvasooriya commented Dec 2, 2024

Enzime left a comment

Enzime Dec 3, 2024

Enzime Dec 3, 2024

thuvasooriya Dec 3, 2024

z0al commented Dec 5, 2024

z0al Dec 5, 2024

talhaHavadar Dec 5, 2024

z0al Dec 5, 2024

thuvasooriya Dec 6, 2024

thuvasooriya Dec 6, 2024 •

edited

Loading

thuvasooriya Dec 6, 2024 •

edited

Loading

z0al Dec 6, 2024

thuvasooriya Dec 6, 2024 •

edited

Loading

z0al Dec 6, 2024

thuvasooriya Dec 6, 2024

z0al Dec 6, 2024

thuvasooriya Dec 6, 2024 •

edited

Loading

Enzime Dec 8, 2024

thuvasooriya Dec 10, 2024

thuvasooriya commented Dec 17, 2024

		@@ -31,6 +31,6 @@ in
		${config.out}/user/Library/LaunchAgents/org.nixos.aerospace.plist`

aerospace: add workspace-to-monitor-force-assignment option and fix on-window-detected type #1208

Are you sure you want to change the base?

aerospace: add workspace-to-monitor-force-assignment option and fix on-window-detected type #1208

Conversation

thuvasooriya commented Dec 2, 2024

thuvasooriya commented Dec 2, 2024

Enzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

z0al commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thuvasooriya Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

thuvasooriya Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thuvasooriya Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thuvasooriya Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thuvasooriya commented Dec 17, 2024

aerospace: add `workspace-to-monitor-force-assignment` option and fix `on-window-detected` type #1208

aerospace: add `workspace-to-monitor-force-assignment` option and fix `on-window-detected` type #1208

thuvasooriya Dec 6, 2024 •

edited

Loading

thuvasooriya Dec 6, 2024 •

edited

Loading

thuvasooriya Dec 6, 2024 •

edited

Loading

thuvasooriya Dec 6, 2024 •

edited

Loading