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

Show spinner when consolidating logs for problem reports #6255

Conversation

rablador
Copy link
Collaborator

@rablador rablador commented May 15, 2024

The problem report view freezes when pressing the "view app logs" button, if many logs need to be consolidated. We should add a spinner to let the user know that everything's alright.


This change is Reviewable

Copy link

linear bot commented May 15, 2024

@rablador rablador self-assigned this May 15, 2024
@rablador rablador added the iOS Issues related to iOS label May 15, 2024
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)


ios/MullvadVPN/View controllers/ProblemReport/ProblemReportReviewViewController.swift line 79 at r1 (raw file):

    private func loadLogs() {
        let presentation = AlertPresentation(

it creates a delay for log presentation which doesn't guarantee the consilating is done.is it enough?

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/MullvadVPN/View controllers/ProblemReport/ProblemReportReviewViewController.swift line 79 at r1 (raw file):

Previously, mojganii wrote…

it creates a delay for log presentation which doesn't guarantee the consilating is done.is it enough?

It should wait until it's done, not just a delay.

Copy link
Collaborator Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/MullvadVPN/View controllers/ProblemReport/ProblemReportReviewViewController.swift line 79 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It should wait until it's done, not just a delay.

Yes, that's the idea, to wait until logs have been consolidated.

@mojganii mojganii requested a review from pinkisemils May 24, 2024 10:49
Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pinkisemils and @rablador)


ios/MullvadVPN/View controllers/ProblemReport/ProblemReportReviewViewController.swift line 79 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Yes, that's the idea, to wait until logs have been consolidated.

ok, you locked view and interaction somehow.

Copy link
Collaborator Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)


ios/MullvadVPN/View controllers/ProblemReport/ProblemReportReviewViewController.swift line 79 at r1 (raw file):

Previously, mojganii wrote…

ok, you locked view and interaction somehow.

The spinner cannot be dismissed manually, so the user has to wait. For better or worse.

@pinkisemils pinkisemils force-pushed the show-spinner-when-consolidating-logs-for-problem-reports-ios-686 branch from 282cef3 to 4aca24c Compare May 29, 2024 15:06
@pinkisemils pinkisemils force-pushed the show-spinner-when-consolidating-logs-for-problem-reports-ios-686 branch from 4aca24c to a5c3ec4 Compare May 29, 2024 15:28
@pinkisemils pinkisemils merged commit 4bfc949 into main May 29, 2024
6 of 7 checks passed
@pinkisemils pinkisemils deleted the show-spinner-when-consolidating-logs-for-problem-reports-ios-686 branch May 29, 2024 15:28
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

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

Successfully merging this pull request may close these issues.

None yet

3 participants