-
Notifications
You must be signed in to change notification settings - Fork 321
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
Extend "seach --sync" command with timing tunables. #143
base: master
Are you sure you want to change the base?
Conversation
…t and waiting time before attempt tunables.
…w (Just a logical conclusion that came after rethinking the overal behaviour).
@@ -58,7 +58,9 @@ int cmd_search(context_t *context) { | |||
"--title DEPRECATED. Same as --name.\n" | |||
"--all Require all conditions match a window. Default is --any\n" | |||
"--any Windows matching any condition will be reported\n" | |||
"--sync Wait until a search result is found.\n" | |||
"--sync [T [A]] Wait T * A seconds until a search result is found.\n" | |||
" T - sync waiting time till next attempt, default: 0.5\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer keeping --sync a no_argument and maybe instead providing this functionality with --sync-timeout?
Additionally, I think having users say "check every T
seconds" feels like an unnecessary configuration detail -- I think we can choose the right check interval ourselves in the code and have users provide how long they want to wait for (--sync --sync-timeout 5
for 5 second timeout) ?
Alternately, I am open to being convinced that --sync [optional_timeout]
is better than two separate flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case --sync works with no arguments as well as with arguments. Arguments are not mandatory.
What kind of benefit is there to split those in two separate flags? Are there any different future flags intended for --sync?
As for timing argument(s)... At first when i wanted to implement timing, i thought too, to have it in one argument, just time in seconds, but when was looking on the code, that the check is done with simple loop not select() type of function and there was a TODO comment, that it would be nice to let it be tunable i thought that that is nice to have it this way, since somebody already needs it also. Another thing which i realized later was, when using already this in scripts was, that it was really convenient, because the --sync did not make xdotool sleep the whole time till rechecking, but it could recheck on the intervals specified in command line, thus in cases if window popped up earlier than total duration, script could continue execution at that point, thus in overall execution time would be smaller.
I don't know about other users and their use cases, but for mine it turned out to work really nice, because when scripting xdotool, i know exactly what the script is written for and characteristics for the executed application, thus can make longer waiting time till next attempt and not burn unnecessarily CPU cycles. OTOH nowadays CPU cycles are not so important and maybe not for xdotool, maybe users don't care if total script execution is 25 seconds or 30, it really is hard to tell.
As far as for --sync flag. With this patch:
- if just --sync is specified (works as was before, no flags needed),
- --sync 5, for example, would wait 5 seconds in total and the flag would work the same way you suggest and user does not have to care of details,
- --sync 5 10 would be for more precise configured use cases.
One drawback for two separate flags, that came into my mind now, is code reuse. It would be copy/paste the same code and harder to manage changes afterwards. |
This modification extends xdotool "seach --sync" command with execution total time, attempt count and waiting time before next attempt tunables. Unfortunately i need this functionality and TODO was left in code too, thus two rabbits with one shot.
Code should be fine even for future use cases, when there is some non-blocking way of how to seach for windows by these complex criteria until they are found (this seems unlikely to happen, but anyways this change does not disturb that).
I'm not too good in writing comments and manual pages, so maybe some brush-up is needed on that.
Change should not leave any implication on the old scripts, because the default values are the same as values which were used before.