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

Profile dialog #677

Merged
merged 10 commits into from
Jul 26, 2020
Merged

Profile dialog #677

merged 10 commits into from
Jul 26, 2020

Conversation

krkk
Copy link
Contributor

@krkk krkk commented Apr 19, 2020

Added uploading user avatar, changing display name, and viewing and renaming devices.

preview

Closes #28
Contributes to #268 – I have no idea how to reveal an access token here.

@KitsuneRal KitsuneRal added the enhancement A feature or change request for Quaternion label Jun 25, 2020
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.

This is fantastic, thanks! Just a few nitpicks.

Comment on lines 800 to 803
this, [=]
{
auto* dlg = new ProfileDialog(c->user(), this);
dlg->setModal(false);
dlg->setAttribute(Qt::WA_DeleteOnClose);
dlg->reactivate();
});
Copy link
Member

Choose a reason for hiding this comment

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

You might have seen summon() used in other places to achieve the same effect and also provide lazy initialisation:

Suggested change
this, [=]
{
auto* dlg = new ProfileDialog(c->user(), this);
dlg->setModal(false);
dlg->setAttribute(Qt::WA_DeleteOnClose);
dlg->reactivate();
});
this, [this,c,dlg=QPointer<ProfileDialog>{}]() mutable
{
summon(dlg, c->user(), this);
});

Being pre-C++17, other places use static declaration instead of an initialisation in the capture block and making the lambda mutable - the C++17 way is slightly cleaner because it destructs the dialog when the lambda is destructed (in this case - when the connection is logged out). Ideally it should destruct the dialog with deleteLater() but I'm yet to commit the facility class to do that.

Comment on lines 49 to 51
//auto mainLayout = new QFormLayout;
//profileWidget->setLayout(mainLayout);

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//auto mainLayout = new QFormLayout;
//profileWidget->setLayout(mainLayout);

topLayout->addLayout(essentialsLayout);
}

//mainLayout->addRow(topLayout);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//mainLayout->addRow(topLayout);

connect(devicesJob, &BaseJob::success, this, [=] {
m_deviceTable->setRowCount(devicesJob->devices().size());

for (int i = 0; i < devicesJob->devices().size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = 0; i < devicesJob->devices().size(); i++) {
for (int i = 0; i < devicesJob->devices().size(); ++i) {

if (!m_avatarUrl.isEmpty())
m_user->setAvatar(m_avatarUrl);

for (auto d: m_devices) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (auto d: m_devices) {
for (const auto& deviceIt = m_devices.begin(); it != m_devices.end(); ++it) {

m_user->setAvatar(m_avatarUrl);

for (auto d: m_devices) {
auto list = m_deviceTable->findItems(m_devices.key(d), Qt::MatchExactly);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto list = m_deviceTable->findItems(m_devices.key(d), Qt::MatchExactly);
auto list = m_deviceTable->findItems(deviceIt.key(), Qt::MatchExactly);

Comment on lines 161 to 162
if (!list.isEmpty() && newName != d)
m_user->connection()->callApi<Quotient::UpdateDeviceJob>(m_devices.key(d), newName);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!list.isEmpty() && newName != d)
m_user->connection()->callApi<Quotient::UpdateDeviceJob>(m_devices.key(d), newName);
if (!list.isEmpty() && newName != deviceIt.value())
m_user->connection()->callApi<Quotient::UpdateDeviceJob>(deviceIt.key(), newName);

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 a ton!

@KitsuneRal KitsuneRal merged commit c793651 into quotient-im:master Jul 26, 2020
@krkk krkk deleted the profile-dialog branch July 26, 2020 22:30
@KitsuneRal KitsuneRal mentioned this pull request Aug 8, 2020
6 tasks
@KitsuneRal KitsuneRal mentioned this pull request Mar 12, 2021
@KitsuneRal KitsuneRal linked an issue Mar 12, 2021 that may be closed by this pull request
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
None yet
Development

Successfully merging this pull request may close these issues.

Identities UI Change/set avatar
2 participants