Skip to content
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

Add login warning dialog for logging into the same account #505

Merged
merged 2 commits into from
Feb 24, 2019
Merged

Add login warning dialog for logging into the same account #505

merged 2 commits into from
Feb 24, 2019

Conversation

krkk
Copy link
Contributor

@krkk krkk commented Feb 23, 2019

Closes #369

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on this. I have a few nitpicks, the biggest (and the only mandatory one) is about the for() declaration.

{
if (a->userId() == connection->userId())
{
Dialog warningDialog(tr("Logging in into a logged in account"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You actually don't need a Dialog here. A QMessageBox::warning() should cover all your needs, shouldn't it?

@@ -760,7 +761,26 @@ void MainWindow::showLoginWindow(const QString& statusMessage)
account.sync();

showFirstSyncIndicator();
addConnection(connection, dialog.deviceName());

for (auto a: connections)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, instead of an explicit for() it would be great to do a std::find():

Suggested change
for (auto a: connections)
const auto it = std::find(connections.cbegin(), connections.cend(),
[a] (Connection* c) { return a->userId() == c->userId(); });
if (it != connections.cend())

If you hate STL algorithms, at least enclose connections into qAsConst to prevent COW-detaching (see, e.g., https://stackoverflow.com/questions/35811053/using-c11-range-based-for-loop-correctly-in-qt).


for (auto a: connections)
{
if (a->userId() == connection->userId())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With std::find() this becomes unneeded, of course.

warningDialog.addWidget(label);

if (warningDialog.exec())
deviceName += "-" + connection->deviceId();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's really a nitpick but I believe you can break; after that (unless rewritten via std::find(), see above).

@@ -746,6 +746,7 @@ void MainWindow::showLoginWindow(const QString& statusMessage)
{
auto connection = dialog.releaseConnection();
AccountSettings account(connection->userId());
QString deviceName = dialog.deviceName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move it down, closer to where it's actually used? It's not C so we can declare variables anywhere in the block (and I prefer it to grouping variable definitions).

client/dialog.cpp Show resolved Hide resolved
…for logging into the same account
@KitsuneRal KitsuneRal added the enhancement A feature or change request for Quaternion label Feb 24, 2019
@KitsuneRal KitsuneRal merged commit 8b36a58 into quotient-im:master Feb 24, 2019
@krkk krkk deleted the login-warning-dialog branch February 24, 2019 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for Quaternion
Projects
Status: Version 0.0.9.4 - Done
Development

Successfully merging this pull request may close these issues.

2 participants