Focus first empty field in credentials dialog#43
Conversation
When some credentials are already stored, put the keyboard focus on the
first empty field so that e.g. a saved username and password let the user
type the OTP code straight away without clicking. A field counts as empty
both when it was never stored (None) and when it was stored blank ('').
grzegorz-gutowski
left a comment
grzegorz-gutowski
left a comment
There was a problem hiding this comment.
Thanks! This is a very nice idea and a clean solution!
| if user_input.value is not None: | ||
| entry.set_text(user_input.value) | ||
| default_store = True | ||
| if not entry.get_text() and focus_entry is None: |
There was a problem hiding this comment.
I think this would read better:
if user_input.value is not None:
...
elif focus_entry is None:
focus_entry = entryThere was a problem hiding this comment.
Thanks for your feedback! I tried the elif but it actually breaks one case, so I ended up not using it. Here is why.
The tricky case is a field that was saved empty. It does not come back as None, it comes back as an empty string ''.
With the elif that field goes into the first branch, because '' is not None evaluates to True, so the elif never runs for it and the focus skips right over it. That was the problem when only the username or only the password was saved.
There are two ways to make the elif work. One is to change that first condition from is not None to a plain truthiness check like if user_input.value:, so the empty string also falls through into the elif. The catch is that same first line also sets default_store, so changing its condition changes that behaviour too, and there are no tests around it.
So to avoid touching that existing line, I went with a separate if instead of an elif. A separate if always runs no matter what the first one did, so it still catches the empty field, and the is not None line stays exactly as it was.
If you would rather keep the proposed elif I am happy to do that, I would just switch the first condition to the truthiness check and accept that small change to default_store. Either way the test covers the four cases (nothing saved, only user, only password, both saved) and they all pass.
Happy to go with whatever you think fits best.
There was a problem hiding this comment.
I see the point. My gut tells me: we shouldn't be storing empty values. And if an empty value comes back from the safe storage it should be treated as if there was nothing there.
If you don't mind, as we are already here, I'd like to change the logic to the one suggested by you:
if user_input.value:
entry.set_text(user_input.value)
default_store = True
elif focus_entry is None:
focus_entry = entryThere was a problem hiding this comment.
Done, pushed the elif version. And you are right that empty values shouldn't really be
stored in the first place, this at least makes sure an empty one is treated as empty.
Reads cleaner than two separate ifs, and `if user_input.value:` treats both None and an empty string '' as empty, so a field stored blank still counts as empty when picking which one to focus.
2 participants
When some credentials are already stored, put the keyboard focus on the first empty field so that e.g. a saved username and password let the user type the OTP code straight away without clicking. A field counts as empty both when it was never stored (None) and when it was stored blank ('').