search

alex-courtis / way-displays

way-displays: Auto Manage Your Wayland Displays
MIT License
267 stars 13 forks source link

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

Closed PaideiaDilemma closed 7 months ago

PaideiaDilemma commented 8 months ago

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:

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.

alex-courtis commented 7 months ago

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

alex-courtis commented 7 months ago

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

alex-courtis commented 7 months ago

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 commented 7 months ago
  • 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 commented 7 months ago

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 commented 7 months ago
  • 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 commented 7 months ago

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 commented 7 months ago

Let's get the success cases nailed down first.

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

alex-courtis commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 cfbe233993f9a065f5f02e83b75dbf05dc65407b

PaideiaDilemma commented 7 months ago

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 commented 7 months ago

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.

PaideiaDilemma commented 7 months ago

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

Sounds good!

matthewwardrop commented 7 months ago

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

alex-courtis commented 7 months ago

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