Skip to content
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

"exec" command not working with [command_criteria] #3903

Closed
randoragon opened this issue Jan 12, 2020 · 12 comments · Fixed by #3905
Closed

"exec" command not working with [command_criteria] #3903

randoragon opened this issue Jan 12, 2020 · 12 comments · Fixed by #3905
Labels
4.17 bug reproducible A bug that has been reviewed and confirmed by a project contributor

Comments

@randoragon
Copy link

randoragon commented Jan 12, 2020

**

I'm submitting a…

[x] Bug
[ ] Feature Request
[ ] Documentation Request
[ ] Other (Please describe in detail)

Current Behavior

When writing a bindsym with [command_criteria], whenever I use the "exec" command, it ignores the criteria and runs 100% of the time. Using any other command than "exec" doesn't cause this problem, which leads me to believe there's something specifically wrong with the "exec" command.

Expected Behavior

The "exec" command runs only for windows narrowed down by the [command_criteria].

Reproduction Instructions

In my case, these two sets of bindsyms showed the error:

bindsym $mod+a [con_id=__focused__ floating] exec "i3-msg border normal"
bindsym $mod+b [con_id=__focused__ tiling]   exec "i3-msg border none"

The above two commands run regardless of whether the focused window is floating or tiled.
The expected behavior is for $mod+a to only work on floating windows, and $mod+b on tiled windows.

bindsym $mod+c [con_id=__focused__ floating] border normal
bindsym $mod+d [con_id=__focused__ tiling]   border none

The above two commands abide by the criteria, i.e. $mod+c only works on floating windows, and $mod+d only on tiled windows.

(The use of "border normal" and "border none" is completely arbitrary, it's just to illustrate which bindsyms get executed and which don't)

Environment

Output of i3 --moreversion 2>&-:

i3 version: 4.17-239-gd21c3a09
https://pastebin.com/1veHqCzK
Logfile URL: https://logs.i3wm.org/logs/5673511062339584.bz2
- Linux Distribution & Version: Arch Linux, Kernel x86_64 Linux 5.4.10-arch1-1
- Are you using a compositor (e.g., xcompmgr or compton): yes, picom
@Airblader
Copy link
Member

My previous comment was actually not correct. This does appear to be a bug because exec commands do not check for window matches at all. This usually makes sense as executing something per matched window is pointless, but here it decides between not executing at all. This we should handle.

@Airblader Airblader reopened this Jan 12, 2020
@Airblader Airblader added the reproducible A bug that has been reviewed and confirmed by a project contributor label Jan 12, 2020
@Airblader
Copy link
Member

Airblader commented Jan 12, 2020

So essentially the issue here is that cmd_exec in src/commands.c does not use HANDLE_EMPTY_MATCH or owindows at all, since it doesn't need a window to operate on.

I guess the question here is whether it really makes sense to execute the command for each matched window (as we do for commands in general) or whether we should deviate from normal behavior and make it a once-or-never kind of logic.

In terms of principle of least surprise we should execute n times, but this could actually break things for some users who unknowingly match multiple windows. Although the same argument can be made for empty matches, I guess.

Airblader added a commit to Airblader/i3-original that referenced this issue Jan 12, 2020
We currently do not evaluate match criteria for the exec command
since generally executing the same command multiple times is
unlikely to make sense.

However, it does make sense when the match is empty and this should
prevent the command from running, which currently does not happen.

For consisteny we execute the command as many times as there are
matched criteria, but print a warning if it matches more than one
container.

fixes i3#3903
@Airblader
Copy link
Member

@randoragon Could you give #3905 a try to see if it fixes the issue?

@randoragon
Copy link
Author

@Airblader To give some context to how I managed to find this "bug" and what I was originally trying to achieve with those quirky binds:
I wrote a couple of scripts for my desktop for enhanced floating window movement and resizement, but then found out those scripts don't work with tiling windows. Instead of separating the movement of tiling/floating windows into different keybindings which seemed annoying, I decided to find a way to apply a "conditional statement" and use the same binding for both. The conditional looked as follows:

bindsym $mod+Shift+h [con_id=__focused__ tiling] move left, [con_id=__focused__ floating] exec windowmove x -20

So, to sum up, I never intended for the "exec" command to run for multiple windows, I agree that it's debatable whether or not that's a desirable or useful behavior to begin with. But if you're planning on explicitly making the "exec" command an exception from the general criteria rules, It would be cool to see it documented somewhere in the i3 docs, in case other users stumble upon it and get confused (I know I did).

@Airblader
Copy link
Member

We're not making it an exception, with the PR it will behave just like any other command. Your use-case is valid and absolutely should work.

@randoragon
Copy link
Author

@randoragon Could you give #3905 a try to see if it fixes the issue?

Hmm, I've tried cloning your i3-original repository and following the compilation from source steps, but on the actual make step I'm getting an error: "make: *** No targets specified and no makefile found. Stop.". What am I missing? I'm positive I have everything in terms of dependencies, because I'm running the AUR version of i3.

@orestisfl
Copy link
Member

Run every command exactly like in the script and report its output if it still doesn't work.

@randoragon
Copy link
Author

Sorry, I'm new at this sort of thing, what script? I've tried running ./configure and following that with make && sudo make install, but that just showered me with errors. I can also see compile and install-sh scripts, but I'm unsure of how I should use them, or if I should in the first place.

@orestisfl
Copy link
Member

I mean all the commands in the wiki page you linked:

autoreconf --force --install
rm -rf build/
mkdir -p build && cd build/
../configure --prefix=/usr --sysconfdir=/etc --disable-sanitizers
make

@randoragon
Copy link
Author

Alright, I made the mistake of skipping the second last step, because I wrongly assumed it to be directed at only sanitizer-free release versions. Problem solved.

@randoragon
Copy link
Author

randoragon commented Jan 14, 2020

Alright, I've tested the fix from PR, works perfectly fine for me. Thank you for your help guys.

Airblader added a commit to Airblader/i3-original that referenced this issue Jan 16, 2020
We currently do not evaluate match criteria for the exec command
since generally executing the same command multiple times is
unlikely to make sense.

However, it does make sense when the match is empty and this should
prevent the command from running, which currently does not happen.

For consisteny we execute the command as many times as there are
matched criteria, but print a warning if it matches more than one
container.

fixes i3#3903
@Airblader
Copy link
Member

Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.17 bug reproducible A bug that has been reviewed and confirmed by a project contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants