fdw / rofi-rbw

Rofi frontend for Bitwarden
https://github.com/fdw/rofi-rbw/
MIT License
192 stars 25 forks source link

Added option typer arguments #52

Closed jacob-horton closed 2 years ago

jacob-horton commented 2 years ago

Similar to selector arguments, I added the option to define arguments for the typer. This allows for setting options such as the delay between key presses.

I also updated the README to include ydotool and describe the usage of --typer-args

fdw commented 2 years ago

Hey, thank you! 🙂

You mentioned delay between key presses. Is this something we could set by default? Do you have any other use cases in mind? I'm just asking because configuration always adds complexity, and if we can avoid it with good defaults, that's often preferable.

jacob-horton commented 2 years ago

Hi, Yeah, no worries, the only use case I had in mind was the delay between key presses. I only went with allowing any arguments because it would be more consistent with what was done with --selector-args? But I am more than happy to reduce the complexity to just control the delay between key presses :slightly_smiling_face:

I am unsure what a sensible default would be, though. xdotool and ydotool have a default of 12ms, but wtype has a default of 0ms. I am unsure why 12ms was chosen, and cannot seem to find anything on Google that explains it. Personally, I use xdotool and would like the delay to be 0ms (as this prevents me accidentally hitting enter, to log in, before the password is typed). I do not see why the default would not be 0ms, unless it causes some issues? What would you suggest is a sensible default?

fdw commented 2 years ago

I agree, if we offer this parameter, it's better to make it generic for all kinds of arguments.

I could imagine that there are problems if you set it to 0, effectively sending everything without even a tiny break. Have you tried this, and did it work? If there are no problems, I think we can just set it to 0 everywhere.

jacob-horton commented 2 years ago

I have tried it as 0, and so far there have been no problems. I am just concerned about edge cases that I have missed.

Also, I am a bit confused how we would make it generic for any typer argument, but also have a default for just the delay? Would you like one option for key delay, and one for generic arguments? Sorry, this is my first proper pull request.

I have also tried using ydotool, but there is currently an issue with it, which means the key-delay doesn't work. I will have a look at trying to fix this though.

Edit: the issue was not what I thought, the issue was actually options being swapped (see here). I have managed to fix this and will submit a PR to them shortly after doing some testing.

Second edit: I have submitted the PR to fix both issues, but there has been no activity on ydotool since Feb, so I'm not sure if/when it will be merged.

fdw commented 2 years ago

Sorry, this is my first proper pull request.

Awesome, I feel honored! And there are no stupid questions 🙂

Also, I am a bit confused how we would make it generic for any typer argument, but also have a default for just the delay? Would you like one option for key delay, and one for generic arguments?

If a general delay of 0 works, my plan would be to hardcode it and not offer any arguments to the typer, to reduce complexity (until someone needs it 😉).

I have tried it as 0, and so far there have been no problems. I am just concerned about edge cases that I have missed.

This is one of the cases where we can't try everything, so I'd be fine with merging a hardcoded 0 and waiting for bug reports.

jacob-horton commented 2 years ago

Perfect, I just pushed the update to only offer the key delay parameter with a default of 0 (I wrote the code while we were discussing, so it's showing up earlier in the comments)

fdw commented 2 years ago

I haven't forgotten this PR, but I want to think a bit more if the added complexity with a new parameter is worth it, or if we should just hardcode 0 into the typers.

jacob-horton commented 2 years ago

No worries, thank you

fdw commented 2 years ago

My current impression is that the delay is not a problem for most people. Still, we could reduce it to avoid the problems you described (and to make it faster).

Since this is also the only use case for --typer-args, that argument would add complexity for little benefit. --key-delay is a bit less complex, but still a new argument. So, before we add this complexity, I'd like to try hardcoding a delay of 0 (or a low value like 2). I expect this to solve the mentioned problems and work for everyone while having a simpler UX. Only if that introduces new problems would I want to make the delay configurable.

So, do you want to hardcode 0 in this PR? I'm also not against keeping the previous commit (for --typer-args and for --key-delay) in case we'd need one of them later.

jacob-horton commented 2 years ago

That sounds good, I'll hardcode the 0 now. I'll also keep the branch, so I keep the previous commits

jacob-horton commented 2 years ago

Would you prefer I use a constant for the hardcoded 0 and pass this through to the type_characters method, or just have the 0 hardcoded into the run parameters?

If using a constant is better, how would you like me to define it? I'm not too sure on conventions - would I use upper case with underscores? And where would I define it - in the main method, the RofiRbw class or just in the rofi_rbw.py file?

Edit: another option is to keep key_delay as a parameter (like I would do with the constant), but just pass 0 directly in to the type_characters method without defining the constant

fdw commented 2 years ago

Having it directly in the run parameters should be clear enough. Since --key-delay is right in front of it, it's easy enough to understand.

jacob-horton commented 2 years ago

I have hardcoded it into the run parameters now :slightly_smiling_face:

fdw commented 2 years ago

Thanks a lot! 🙂