-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: Keychain QML Item #17166
feat: Keychain QML Item #17166
Conversation
Jenkins BuildsClick to see older builds (78)
|
34e8850
to
453becd
Compare
453becd
to
5a88732
Compare
87aadf6
to
4e0e771
Compare
@caybro you don't have to review unless it's out of draft 🙂 |
f979fda
to
82d84f2
Compare
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.
Looks good in general, just some minor technical comments.
2575df8
to
64326e3
Compare
@@ -36,6 +36,7 @@ status-desktop.log | |||
nim_status_client.log | |||
*.csv | |||
.flatpak-builder |
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.
Is it related and necessary here?
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.
@micieslak It's not related.
But I get this shit every time I run the Storybook, and I have to clean it up before committing, quite annoying 😬
64326e3
to
bb19837
Compare
if (m_future.isRunning()) { | ||
return; |
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 we should print a warning at least at this point. Calling it when busy means some logical problems on the caller side and shouldn't be just muted.
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.
LGTM!
@caybro can you please take a look? |
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.
LGTM
Just a minor detail + pls squash the commits :)
{ | ||
Q_UNUSED(account); | ||
qWarning() << "Keychain::requestGetCredential is intended to be called only on MacOS."; | ||
emit getCredentialRequestCompleted(Keychain::StatusNotSupported, ""); |
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.
emit getCredentialRequestCompleted(Keychain::StatusNotSupported, ""); | |
emit getCredentialRequestCompleted(Keychain::StatusNotSupported, {}); |
@@ -0,0 +1,48 @@ | |||
#include "StatusQ/keychain.h" | |||
|
|||
#include <QDebug> |
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.
Not needed here?
@caybro I always do it with Github |
Required for #17085
Screen.Recording.2025-02-02.at.13.17.28.mov