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

Adopt Update dialogs to the new modal widget experience #11617

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erikjv
Copy link
Contributor

@erikjv erikjv commented Apr 25, 2024

Fixes: #11593

@erikjv erikjv self-assigned this Apr 25, 2024
@erikjv erikjv marked this pull request as draft April 25, 2024 16:56
@erikjv erikjv requested a review from TheOneRing April 25, 2024 16:56
@erikjv erikjv force-pushed the work/adopt-update-dialogs branch 2 times, most recently from 82d0c63 to 66e5494 Compare May 3, 2024 12:50
@erikjv erikjv marked this pull request as ready for review May 3, 2024 12:50
@erikjv
Copy link
Contributor Author

erikjv commented May 3, 2024

New message box:

Screenshot 2024-05-03 at 14 57 17

@erikjv
Copy link
Contributor Author

erikjv commented May 3, 2024

Update downloaded dialog:

Screenshot 2024-05-03 at 15 06 54

@TheOneRing
Copy link
Member

Hmm could we use the same icon, the app icon, for both?
Also I guess both should be centered and not aligned to top?

@erikjv erikjv force-pushed the work/adopt-update-dialogs branch from 66e5494 to 5fdd4fe Compare May 7, 2024 13:07
@erikjv
Copy link
Contributor Author

erikjv commented May 7, 2024

New screenshots:

Screenshot 2024-05-07 at 16 47 41 Screenshot 2024-05-07 at 16 49 22

QString txt = tr("<p>A new version of the %1 Client is available.</p>"
"<p><b>%2</b> is available for download. The installed version is %3.</p>")
.arg(Utility::escape(Theme::instance()->appNameGUI()),
Utility::escape(info.versionString()), Utility::escape(Version::versionWithBuildNumber().toString()));
auto *widget = new NoUrlWidget(ocApp()->gui()->settingsDialog(), txt);
widget->setAttribute(Qt::WA_DeleteOnClose);
Copy link
Member

Choose a reason for hiding this comment

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

Its not a dialog anymore, we should handle its deletion.

Q_OBJECT

public:
explicit NoUrlWidget(QWidget *parent, const QString &statusMessage);
Copy link
Member

Choose a reason for hiding this comment

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

What is the meaning of the class name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It comes from the method that previously showed the dialog: WindowsUpdater::showNoUrlDialog. It's called from WindowsUpdater::versionInfoArrived, when the info has an empty URL. 🤷‍♂️

{
delete _ui;
Q_EMIT accepted();
close();
Copy link
Member

Choose a reason for hiding this comment

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

We should rather delete the class in the caller than rely on delete on close.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The removal of the delete is because the _ui is now a QScopedPointer instead of a raw pointer.

Copy link
Member

@TheOneRing TheOneRing left a comment

Choose a reason for hiding this comment

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

Looks great, but please take a look on my comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt Update dialogs to the new modal widget experience
2 participants