-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Auto-Type: Allow actions to fail and be retried #6357
Auto-Type: Allow actions to fail and be retried #6357
Conversation
73ba867
to
092cc4f
Compare
092cc4f
to
58ff316
Compare
@@ -28,8 +28,61 @@ class AutoTypeExecutor; | |||
class KEEPASSXC_EXPORT AutoTypeAction | |||
{ | |||
public: | |||
class Result |
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 feel like this should just be a QPair<AutoTypeAction::ResultEnum, QString>
where the result enum is "Ok, Retry, Failed" and the QString contains the error message (optional). The below is a lot of boiler plate to provide the same end result.
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.
Kind of but it's not very ergonomic to use enums and QPairs and the code isn't self explanatory compared to the boilerplate version.
IIRC I would need to return something like this:
return qMakePair(AutoTypeActionResult_Failed, tr("Well duh."));
It doesn't look very pretty to use a QPair as a return type either so it makes declarations more verbose as well. Also need to remember returning an Ok looks a bit weird as you need to create an empty string. The enum values need to be globally unique unless namespaced.
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.
Doesn't the enum value get wrapped in the class namespace? So it would be AutoTypeAction::Ok
I am not against the current version it is just verbose. We can keep it.
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.
Indeed, the enum would actually work like that but it would reserve the words inside the class. Not necessarily an issue.
Though as discussed on IRC and here I'll keep this implementation for now and we'll refactor later if we feel the urge to either use a more global result class or just get rid of it.
AutoTypeActions are required to return a result object with information if they can be retried or not. An error string is also provided to show a user friendly message why said action did not succeed if even after retries it keeps failing. This is a prerequisite for waiting for modifier keys to be released on X11.
58ff316
to
d2c6958
Compare
AutoTypeActions are required to return a result object with information if they can be retried or not. An error string is also provided to show a user friendly message why said action did not succeed if even after retries it keeps failing.
This is a prerequisite for waiting for modifier keys to be released on X11 and can be used to add friendly error messages to any executor.
We want to merge this before #6351 so it can be refactored to make use of the retry logic here.
Only X11 is actually implemented and others are just stubbed in.
Testing strategy
Manually.
Type of change