waycrate / swhkd

Sxhkd clone for Wayland (works on TTY and X11 too)
https://git.sr.ht/~shinyzenith/swhkd
BSD 2-Clause "Simplified" License
666 stars 47 forks source link

[Bug / Feature] stack for commands #219

Open tsankuanglee opened 11 months ago

tsankuanglee commented 11 months ago

Version Information:

Describe the bug:

Commands chained by && seem to be executed together, disregarding mode stack changes. Example:

super + d
    @enter display

mode display swallow
b
    @escape && @enter display_selection && @enter brightness_selection && set_brightness.sh
endmode

mode display_selection swallow oneoff
{1,2,3}
    save_selection_to_file.sh {left,middle,right}
endmode

mode brightness_selection swallow oneoff
{1,2,3,4,5}
    save_brightness_to_file.sh {0,25,50,75,100}%
endmode

Expected behavior: With @escape && @enter display_selection && @enter brightness_selection && set_brightness.sh, we expect when mode display_selection is popped, we continue to the next command @enter brightness_selection.

Actual behavior: As soon as b fires @escape && @enter display_selection && @enter brightness_selection && set_brightness.sh, everything is executed right away, including the set_brightness.sh script, while we are still in the mode display_selection.

To Reproduce: See example above.

Additional information: Since we don't have the syntax like sxhkd as I mentioned in https://github.com/waycrate/swhkd/issues/67 , we can't exactly do a command with more than two expansions, e.g.

{1,2,3}; {b,c,t}; {1,2,3}
    display_setting.sh {left,middle,right} {brightness,contrast,temperature} {low,mid,high}

With the mode design, I can only get around it by temporarily saving a state for later use.

That said, mode actually provides a more clear and reusable design, if this bug/feature is addressed. For example, I can re-use the display_selection mode for not only brightness, but contrast, color temperature, etc., by entering that mode, and then continue with the next action.

Shinyzenith commented 11 months ago

@EdenQwQ can you provide your input on this?

EdenQwQ commented 11 months ago

@tsankuanglee Could you please check out #220 and let us know if the change meets your request

tsankuanglee commented 11 months ago

@EdenQwQ , I have not got a the chance to test the code yet (will probably be a few days before I can find time to do that), but from the description of that PR, yes, it addresses exactly the problem I'm describing here. Thank you so much!

By the way, the example you gave in your PR is much more concise and clear!

EdenQwQ commented 11 months ago

@EdenQwQ , I have not got a the chance to test the code yet (will probably be a few days before I can find time to do that), but from the description of that PR, yes, it addresses exactly the problem I'm describing here. Thank you so much!

By the way, the example you gave in your PR is much more concise and clear!

Thanks! We won't merge it without your feedback, but take your time!

tsankuanglee commented 11 months ago

Just an update that I'm still on this. I'm getting some weird rust index out of bounds errors:

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 18446744073709551615', swhkd/src/daemon.rs:385:68

I'm also seeing this error with the release version, so I suspect it's something on my end. I'll resolve it and test your PR and report back.

EdenQwQ commented 11 months ago

Just an update that I'm still on this. I'm getting some weird rust index out of bounds errors:

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 18446744073709551615', swhkd/src/daemon.rs:385:68

I'm also seeing this error with the release version, so I suspect it's something on my end. I'll resolve it and test your PR and report back.

It's weird because I though I've solved this issue with the latest commit in that PR. Could you run git pull in command_stack_devel branch and confirm that you're synced with the branch? I'm not sure why this also happens with the release version, if that still happens please provide the method to trigger the bug. Sorry for the inconvenience.

tsankuanglee commented 11 months ago

@EdenQwQ thanks for the prompt. I created #222 for the index out of bound problem that happened on the release version.

To clarify, when I got the error with your PR (yes, I did pull the latest commit), I was using the correct config (as opposed to the incorrect one mentioned in #222), but with an extra @escape (which is semantically wrong, I know). I suspect we need to check for an empty stack before pop.

alt + ctrl + shift + super + w
    @enter web

mode web swallow oneoff
c
    @escape && touch /tmp/test
endmode
tsankuanglee commented 11 months ago

@EdenQwQ By the way, here's an illegal config that I accidentally used (see the last line: no endmode). This same config causes different behavior for the release version and your latest PR commit:

Not sure whether that's a useful data point for you to debug.

alt + ctrl + shift + super + w
    @enter web

mode web swallow oneoff
c
    @escape && touch /tmp/test
escape
Shinyzenith commented 11 months ago

@EdenQwQ can you look into this dear?