#65 add ON_CHANGE_CMD to run a shell command in handle_success #162

PaideiaDilemma · 2024-04-02T21:20:30Z

Hi!

This tries to implement a configuration that allows users to execute an arbitrary command when display configurations change. The command gets executed in handle_success, so when the succeeded event gets fired and is processed by way-displays. This is also how kanshi implements it.

Here are some things to discuss:

The name "ON_CHANGE_CMD" as suggested in Feature: Run shell command on display change detected #65 is quite easy to understand so I went with that. But maybe something like "CHANGE_SUCCESS_CMD" or something different is preferable, because it is kind of a "AFTER_CHANGE_CMD" :D.
~~The test uses the filesystem because I could not come up with another good solution to check command execution~~. spawn_async is not tested at all, because it is hard to test. Is that fine?
Do we need IPC for this?

Additional note for hyprland:
Hyprland currently does not play well and way-displays needs to handle multiple redundant events, so the command might get executed a few times on a change. I might look into that when I have time. Only gets executed once on sway.
Havn't tried other compositors, but that's why I added the note in cfg.yaml.

Can't think of a use case to run a command when vrr changes

alex-courtis · 2024-04-06T01:04:32Z

This looks great! I test and review this on Monday.

alex-courtis · 2024-04-06T01:06:16Z

Looks like you might need to assert the new INFO message during the layout test: https://github.com/alex-courtis/way-displays/actions/runs/8529996247/job/23510275263?pr=162#step:6:332

Now we are not testing if the command actually gets executed.

alex-courtis · 2024-04-09T04:36:47Z

This tries to implement a configuration that allows users to execute an arbitrary command when display configurations change. The command gets executed in handle_success, so when the succeeded event gets fired and is processed by way-displays. This is also how kanshi implements it.

Definitely works. ON_CHANGE_CMD: 'notify-send "$(way-displays -g)"' does what you'd think.

valgrinds successfully for happy path.

Additional note for hyprland:
Hyprland currently does not play well and way-displays needs to handle multiple redundant events, so the command might get executed a few times on a change. I might look into that when I have time. Only gets executed once on sway.
Havn't tried other compositors, but that's why I added the note in cfg.yaml.

Yes, there's nothing we can do about that. We'll also get multiple events for multi-stage changes e.g. mode set, then vrr, then scale.

I think we can make use of that - notifications of successive changes and the fail.

alex-courtis · 2024-04-09T04:50:19Z

spawn_async is not tested at all, because it is hard to test. Is that fine?

That's completely fine, you've tested the invocations. It's definitely async and works.

ON_CHANGE_CMD: 'sleep 10'

alex-courtis · 2024-04-09T04:59:40Z

Do we need IPC for this?

Not quite sure what you're asking, however we could add feedback so that the user has something to go with.

Perhaps the ability to send a notify message like:

eDP-1 Changing:
  from:
    scale:     2.000 (2.213)
    size:      1280x720
    position:  0,0
    mode:      2560x1440@60Hz (59,998mHz) (preferred)
    VRR:       off
  to:
    scale:     1.000

We could dump that text to a file like /tmp/way-displays/change.20240409_145421.txt

The user could set something like this to retrieve that file:

ON_CHANGE_CMD: 'notify-send way-displays "$(cat ${WD_CHANGE_FILE})"'
# or
ON_CHANGE_CMD: 'notify-send way-displays "$(cat %%wd_change_file%%)"'

What do you think?

alex-courtis

This is looking fantastic! Please:

add to way-displays -g; adding it to info.c should propagate it everywhere
add CLI set and delete commands

alex-courtis · 2024-04-09T05:06:08Z

But maybe something like "CHANGE_SUCCESS_CMD" or something different is preferable, because it is kind of a "AFTER_CHANGE_CMD" :D.

Great potential there: CHANGE_FAILED_CMD CHANGE_CANCELLED_CMD.

Let's get the success cases nailed down first.

PaideiaDilemma · 2024-04-09T15:54:38Z

Do we need IPC for this?

I meant set and delete via the cli.
I will do that today :)

Regarding the suggestion about something like a WD_CHANGE_FILE.

I think that would be great and I would be down to implement that.
Do you want me to do that in this PR or in a separate one?

PaideiaDilemma · 2024-04-09T15:58:10Z

Let's get the success cases nailed down first.

Alright I agree. CHANGE_SUCCESS_CMD sounds good.
I will rename.

alex-courtis · 2024-04-14T05:02:33Z

Many thanks for the updates! I'll review them tomorrow or the day after.

FYI my availability for this project is Mon-Tue only.

alex-courtis · 2024-04-15T03:49:48Z

I think that would be great and I would be down to implement that.
Do you want me to do that in this PR or in a separate one?

Very sensible. This change is complete and can be merged.

alex-courtis · 2024-04-15T03:53:24Z

I spoke too soon; there is an odd memory leak reported. Looks like it's reported every loop, for libinput monitor and wl display connect.

I'll take a look...

valgrind --leak-check=full --show-leak-kinds=all --suppressions=/tmp/vg.supp way-displays > /tmp/wd.log 2>&1

wd.log

Bisected to cfbe233

alex-courtis

Looking good, works.

simplify spawn
add help / man

Do you want me to push changes for help / man?

src/process.c

src/layout.c

- only fork once - rename from spawn_async to spawn_sh_cmd

PaideiaDilemma · 2024-04-15T14:06:04Z

Thanks for the review.

add help / man

Do you want me to push changes for help / man?

Did that as well. Let me know if <shell command> is fine or not. I didn't just want to use <command>, since it is already used in the help text via "COMMAND" as in the command that way-displays should do.

alex-courtis · 2024-04-16T03:53:32Z

Did that as well. Let me know if <shell command> is fine or not. I didn't just want to use <command>, since it is already used in the help text via "COMMAND" as in the command that way-displays should do.

That works well - it's really clear what the user can execute.

alex-courtis

This is looking fantastic! I'm excited for all the opportunities this opens up.

Let's get that last child bit in and we're good to go.

We can release 1.11 and update the wiki, unless you'd prefer to add something else first.

src/process.c

Thanks to dwm :)

src/server.c

PaideiaDilemma · 2024-04-16T07:53:57Z

We can release 1.11 and update the wiki, unless you'd prefer to add something else first.

Sounds good!

alex-courtis

Works beautifully - immediate spawn, no zombies, no complaints from valgrind.

Many thanks for your contribution! Hopefully we can work together on more such functionality #164

src/server.c

matthewwardrop · 2024-04-20T04:35:05Z

Woohoo! Thanks for working on this @PaideiaDilemma ; and for your consideration @alex-courtis !

alex-courtis · 2024-04-20T04:37:33Z

Thank you... I added a complex command: https://github.com/alex-courtis/way-displays/blob/master/examples/cfg.yaml#L70

PaideiaDilemma added 9 commits March 29, 2024 17:16

add spawn_async function

bc91418

add ON_CHANGE_CMD config

7dbb2ed

spawn on_change_cmd in handle_success

f1a2a3d

adapt marshalling tests

afb57ba

isolate the spawed process a bit more

cfbe233

ignore adaptive sync changes

82191fa

Can't think of a use case to run a command when vrr changes

add a test for the change command

41cd743

add ON_CHANGE_CMD to cfg.yaml

e2f3456

sanitary fixes

e63e344

PaideiaDilemma added 2 commits April 7, 2024 12:14

wrap spawn_async and simplify the on_change_cmd test

76169f3

Now we are not testing if the command actually gets executed.

fix mem leak due to creating the default config twice

6a68451

alex-courtis requested changes Apr 9, 2024

View reviewed changes

PaideiaDilemma added 4 commits April 9, 2024 18:08

rename to CHANGE_SUCCESS_CMD

8a85133

add cli set and delete for CHANGE_SUCCESS_CMD

2b1e78d

add CHANGE_SUCCESS_CMD to print_cfg and print_cfg_commands

1351215

add cfg merge tests for change_success_cmd

32fa9a9

PaideiaDilemma requested a review from alex-courtis April 9, 2024 18:23

alex-courtis mentioned this pull request Apr 15, 2024

Additional CHANGE_SUCCESS_CMD Functionality #164

Open

alex-courtis requested changes Apr 15, 2024

View reviewed changes

src/process.c Outdated Show resolved Hide resolved

src/layout.c Show resolved Hide resolved

PaideiaDilemma added 2 commits April 15, 2024 15:57

revision spawn command

3ae2d9c

- only fork once - rename from spawn_async to spawn_sh_cmd

add CHANGE_SUCCESS_CMD to cli usage

b490c60

alex-courtis added 2 commits April 16, 2024 14:10

add CHANGE_SUCCESS_CMD to man

a0264d2

prepare for next minor release

afe995e

alex-courtis requested changes Apr 16, 2024

View reviewed changes

src/process.c Outdated Show resolved Hide resolved

src/process.c Outdated Show resolved Hide resolved

setup signal handlers in the server to avoid zombie processes

637b104

Thanks to dwm :)

PaideiaDilemma commented Apr 16, 2024

View reviewed changes

src/server.c Show resolved Hide resolved

alex-courtis approved these changes Apr 20, 2024

View reviewed changes

src/server.c Show resolved Hide resolved

add compound command example, unify man and cfg.yaml

7eefc79

alex-courtis merged commit 5eb61e0 into alex-courtis:master Apr 20, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#65 add ON_CHANGE_CMD to run a shell command in handle_success #162

#65 add ON_CHANGE_CMD to run a shell command in handle_success #162

PaideiaDilemma commented Apr 2, 2024 •

edited

Loading

alex-courtis commented Apr 6, 2024

alex-courtis commented Apr 6, 2024

alex-courtis commented Apr 9, 2024 •

edited

Loading

alex-courtis commented Apr 9, 2024

alex-courtis commented Apr 9, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis commented Apr 9, 2024

PaideiaDilemma commented Apr 9, 2024

PaideiaDilemma commented Apr 9, 2024

alex-courtis commented Apr 14, 2024

alex-courtis commented Apr 15, 2024

alex-courtis commented Apr 15, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

PaideiaDilemma commented Apr 15, 2024 •

edited

Loading

alex-courtis commented Apr 16, 2024

alex-courtis left a comment

PaideiaDilemma commented Apr 16, 2024

alex-courtis left a comment

matthewwardrop commented Apr 20, 2024

alex-courtis commented Apr 20, 2024

#65 add ON_CHANGE_CMD to run a shell command in handle_success #162

#65 add ON_CHANGE_CMD to run a shell command in handle_success #162

Conversation

PaideiaDilemma commented Apr 2, 2024 • edited Loading

alex-courtis commented Apr 6, 2024

alex-courtis commented Apr 6, 2024

alex-courtis commented Apr 9, 2024 • edited Loading

alex-courtis commented Apr 9, 2024

alex-courtis commented Apr 9, 2024 • edited Loading

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

alex-courtis commented Apr 9, 2024

PaideiaDilemma commented Apr 9, 2024

PaideiaDilemma commented Apr 9, 2024

alex-courtis commented Apr 14, 2024

alex-courtis commented Apr 15, 2024

alex-courtis commented Apr 15, 2024 • edited Loading

alex-courtis left a comment • edited Loading

Choose a reason for hiding this comment

PaideiaDilemma commented Apr 15, 2024 • edited Loading

alex-courtis commented Apr 16, 2024

alex-courtis left a comment

Choose a reason for hiding this comment

PaideiaDilemma commented Apr 16, 2024

alex-courtis left a comment

Choose a reason for hiding this comment

matthewwardrop commented Apr 20, 2024

alex-courtis commented Apr 20, 2024

PaideiaDilemma commented Apr 2, 2024 •

edited

Loading

alex-courtis commented Apr 9, 2024 •

edited

Loading

alex-courtis commented Apr 9, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

alex-courtis commented Apr 15, 2024 •

edited

Loading

alex-courtis left a comment •

edited

Loading

PaideiaDilemma commented Apr 15, 2024 •

edited

Loading