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

Fix view logs crash #5373

Merged
merged 2 commits into from
Nov 15, 2023
Merged

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Oct 27, 2023

Fixes crash that could happen if we render too many lines in the textfield.


This change is Reviewable

@linear
Copy link

linear bot commented Oct 27, 2023

DROID-460 View logs crash because of long log

We should replace the log view with a Lazy Column instead of a TextField due to long input in textfield may crash the app. This will have to disable the copy of the logs due to limitations in SelectableContainer.

@Rawa Rawa self-assigned this Oct 27, 2023
@Rawa Rawa added the Android Issues related to Android label Oct 27, 2023
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

This fixes the issue, however we no longer give the user the ability to download or copy the app logs. So I guess someone with more knowledge about how the users uses or are expected to use the app should confirm that we are not removing an expected feature.

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

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

That is correct, but considering that some users can't copy because it would crash immediately it might be our only way? We could also consider offering a copy button or similar to that.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Pururun Pururun left a 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 raise the issue with Matilda and propose to add a button that allows the user to share/download the logs.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

I've messages design now, to see how they feel about resolving this.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the view-logs-crash-because-of-long-log-droid-460 branch from 7667e9e to c146f8f Compare November 15, 2023 10:24
Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

After internal discussions we have now added a share button that has the option to copy and share the content of the logs.

Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @Pururun)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Nice improvement with the share button!

Reviewed 1 of 3 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ViewLogsScreen.kt line 71 at r2 (raw file):

                navigationIcon = { NavigateBackIconButton(onBackClick) },
                actions = {
                    IconButton(onClick = { shareLogs(uiState.allLines.joinToString("\n")) }) {

Do we need to make this call both here and in the shareLogs function or can we call it in a single place?

Code quote:

uiState.allLines.joinToString("\n")

@Rawa Rawa force-pushed the view-logs-crash-because-of-long-log-droid-460 branch from 97694a6 to 6f3044d Compare November 15, 2023 13:31
Copy link
Contributor Author

@Rawa Rawa 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: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ViewLogsScreen.kt line 71 at r2 (raw file):

Previously, albin-mullvad wrote…

Do we need to make this call both here and in the shareLogs function or can we call it in a single place?

Nice catch, my mistake, I was suppose to use uiState.text() I fixed this issue and refactored out shareLogs function

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ViewLogsScreen.kt line 71 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Nice catch, my mistake, I was suppose to use uiState.text() I fixed this issue and refactored out shareLogs function

👍

@albin-mullvad albin-mullvad force-pushed the view-logs-crash-because-of-long-log-droid-460 branch from 6f3044d to 2fb8caa Compare November 15, 2023 13:55
@albin-mullvad albin-mullvad merged commit 7050e43 into main Nov 15, 2023
9 checks passed
@albin-mullvad albin-mullvad deleted the view-logs-crash-because-of-long-log-droid-460 branch November 15, 2023 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants