minimize window if already focused #41
Conversation
I won't use this personally but if you find it useful for others, you may grant this. If a single window is already open and in focus - minimize it instead if m flag is present.
|
FYI, I've packaged jumpapp for NixOS: NixOS/nixpkgs#57893 |
There was a problem hiding this comment.
Thanks for the PR! (I'm slow to respond to these things, sorry, but I still check in from time-to-time)
I like the behavior that everyone decided on, which is to minimize if it's the only matching open window.
I haven't manually tested the change, but the new code makes sense to me.
In order to make sure this feature doesn't break as a result of future changes, can we add unit tests for the new functionality? The test file is here. I added some pointers below, but if you have any questions feel free to ask. Also if you get stuck, I can write the tests myself, but it'll probably be a few weeks before I find an opportunity to do it.
jumpapp
Outdated
| @@ -85,6 +87,9 @@ jumpapp() { | |||
| if [[ -n "$list" ]]; then | |||
| printf 'Matched Windows [%d]\n' ${#windowids[@]} | |||
| list_matching_windows "$classid" "${pids[@]}" | print_windows | |||
| elif [[ -n "$focusOrMinimize" ]] && [[ ${#windowids[@]} -eq "1" ]] \ | |||
| && [[ "$(get_active_windowid)" -eq "${windowids[0]}" ]]; then | |||
| xdotool getactivewindow windowminimize | |||
There was a problem hiding this comment.
To be able to unit test this, instead of directly calling:
xdotool getactivewindow windowminimize
You could declare a new function near the bottom of the file:
minimize_active_window() {
xdotool getactivewindow windowminimize
}
Then call it from here. That would make it easy to unit test this.
| @@ -10,6 +10,7 @@ Otherwise, launch COMMAND (with opitonal ARGs) to start the application. | |||
| Options: | |||
| -r -- cycle through windows in reverse order | |||
| -f -- force COMMAND to launch if process found but no windows found | |||
| -m -- if a single window is already open and in focus - minimize it | |||
|
Added a test! The testing setup is awesome: it was very easy to figure out how to add tests, despite the fact that I've never tested bash scripts before :) |
|
btw, #27 (comment) suggest making |
|
Tests look good. Change looks good 👍 I thought I'd find the time to test the change but I never seem to find the free time. I've merged it without testing it myself, but I figure on the low chance there's some issue, someone can report it. Thanks again for the PR. |
closes #12tested on plasma, seems to work
EDIT:
closes #27
based on #12