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

Migrate problem report & view logs to compose #5200

Merged
merged 8 commits into from
Oct 4, 2023

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Sep 27, 2023

Migrate report problem view to compose

  • Move some fullscreen logic to dialogs
  • Removes old views
  • Reworks MullvadProblemReport
  • Adds alternative to add color to scroll view (used in view logs)

Caveats:

  • View log uses column and not lazy column because it is not selectable in a LazyColumn

Fixes: DROID-60


This change is Reviewable

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

linear bot commented Sep 27, 2023

DROID-58 Migrate problem report view to compose

Migrate to compose and MVVM.

Make sure to fix the following scrolling issue when migrating to compose:

b3b19e04-3b8a-407a-b276-67d7a7e3bf19

@Rawa Rawa changed the title Migrate problem report view to compose droid 58 Migrate problem report & view logs to compose Sep 27, 2023
@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch from 6ad2bd4 to b58fb5c Compare September 27, 2023 12:14
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.

Reviewed 20 of 25 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 31 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 45 at r2 (raw file):

                Image(
                    painter = painterResource(id = R.drawable.icon_alert),
                    contentDescription = "Remove",

This should be translated no?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 46 at r2 (raw file):

                    painter = painterResource(id = R.drawable.icon_alert),
                    contentDescription = "Remove",
                    modifier = Modifier.width(50.dp).height(50.dp)

Please avoid hardcoded dimensions


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 53 at r2 (raw file):

            Text(
                text = stringResource(id = R.string.confirm_no_email),
                color = colorResource(id = R.color.white),

Please use compose colors, preferably from the theme.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 54 at r2 (raw file):

                text = stringResource(id = R.string.confirm_no_email),
                color = colorResource(id = R.color.white),
                fontSize = dimensionResource(id = R.dimen.text_small).value.sp,

Please use compose text sizes.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 63 at r2 (raw file):

                colors =
                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.red),

Compose colors!


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 64 at r2 (raw file):

                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.red),
                        contentColor = Color.White

If possible please use theme colors.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 75 at r2 (raw file):

                colors =
                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),

Compose colors.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 76 at r2 (raw file):

                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),
                        contentColor = Color.White

Theme colors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 82 at r2 (raw file):

            )
        },
        containerColor = colorResource(id = R.color.darkBlue)

Compose colors.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 83 at r2 (raw file):

                Text(
                    text = stringResource(id = R.string.sending),
                    color = colorResource(id = R.color.white),

Please use compose colors instead.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 84 at r2 (raw file):

                    text = stringResource(id = R.string.sending),
                    color = colorResource(id = R.color.white),
                    fontSize = dimensionResource(id = R.dimen.text_small).value.sp,

Please use compose dimensions instead.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 97 at r2 (raw file):

                dismissOnBackPress = false,
            ),
        containerColor = colorResource(id = R.color.darkBlue)

Compose colors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 121 at r2 (raw file):

                Image(
                    painter = painterResource(id = R.drawable.icon_success),
                    contentDescription = "Remove",

Translated?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 122 at r2 (raw file):

                    painter = painterResource(id = R.drawable.icon_success),
                    contentDescription = "Remove",
                    modifier = Modifier.width(50.dp).height(50.dp)

Hardcoded


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 130 at r2 (raw file):

                text =
                    buildAnnotatedString {
                        withStyle(SpanStyle(color = colorResource(id = R.color.green))) {

Compose color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 135 at r2 (raw file):

                        append(" ")

                        withStyle(SpanStyle(color = colorResource(id = R.color.white))) {

Compose Color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 139 at r2 (raw file):

                        }
                    },
                fontSize = dimensionResource(id = R.dimen.text_small).value.sp,

Compose font size


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 148 at r2 (raw file):

                colors =
                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),

Compose colors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 149 at r2 (raw file):

                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),
                        contentColor = Color.White

Theme color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 155 at r2 (raw file):

            )
        },
        containerColor = colorResource(id = R.color.darkBlue)

Compose color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 180 at r2 (raw file):

                    painter = painterResource(id = R.drawable.icon_fail),
                    contentDescription = null,
                    modifier = Modifier.width(50.dp).height(50.dp)

Hardcoded values


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 187 at r2 (raw file):

            Text(
                text = stringResource(id = R.string.failed_to_send_details),
                color = colorResource(id = R.color.white),

Compose colors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 188 at r2 (raw file):

                text = stringResource(id = R.string.failed_to_send_details),
                color = colorResource(id = R.color.white),
                fontSize = dimensionResource(id = R.dimen.text_small).value.sp,

Compose font size


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 197 at r2 (raw file):

                colors =
                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),

Compose color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 198 at r2 (raw file):

                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.blue),
                        contentColor = Color.White

Theme color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 209 at r2 (raw file):

                colors =
                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.green),

Compose color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 210 at r2 (raw file):

                    ButtonDefaults.buttonColors(
                        containerColor = colorResource(id = R.color.green),
                        contentColor = Color.White

Theme color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 216 at r2 (raw file):

            )
        },
        containerColor = colorResource(id = R.color.darkBlue)

Compose color


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/TextFieldColors.kt line 11 at r2 (raw file):

@Composable
fun mullvadWhiteTextFieldColors(): TextFieldColors =
    TextFieldDefaults.colors(

We should probably create a linear issue to determine the colors text field colors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt line 51 at r2 (raw file):

            withContext(dispatcher) {
                true
                //                sendProblemReport(

What is this function suppose to do?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ViewLogsViewModel.kt line 25 at r2 (raw file):

            // which would make the loading way faster but then the SelectionContainer is broken and
            // text would not be copyable.
            delay(500)

Should we not use the constant here?

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.

Reviewed 10 of 25 files at r1.
Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/MinLoadingConstant.kt line 0 at r1 (raw file):
Can this file be named something more generic so that we can include other loading constants in the same file?

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.

Don't forget to update the change log

Reviewable status: all files reviewed, 32 unresolved discussions (waiting on @Rawa)

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch 2 times, most recently from d0bc74a to 1ac53ac Compare September 27, 2023 13:38
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: 18 of 28 files reviewed, 32 unresolved discussions (waiting on @albin-mullvad and @Pururun)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 45 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should be translated no?

Removed it, we seemingly don't have a good contentDescription for a warning dialog.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 46 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Please avoid hardcoded dimensions

Good catch, fixed


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 53 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Please use compose colors, preferably from the theme.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 54 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Please use compose text sizes.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 63 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors!

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 64 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

If possible please use theme colors.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 75 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 76 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Theme colors

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemNoEmailDialog.kt line 82 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 83 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Please use compose colors instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 84 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Please use compose dimensions instead.

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 97 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 121 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Translated?

Removed.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 122 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Hardcoded

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 130 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 135 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose Color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 139 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose font size

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 148 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 149 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Theme color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 155 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 180 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Hardcoded values

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 187 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose colors

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 188 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose font size

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 197 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 198 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Theme color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 209 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 210 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Theme color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/ReportProblemStateDialog.kt line 216 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Compose color

Done.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/textfield/TextFieldColors.kt line 11 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We should probably create a linear issue to determine the colors text field colors

Agree, https://linear.app/mullvad/issue/DROID-370/determine-compose-textfieldcolors


android/app/src/main/kotlin/net/mullvad/mullvadvpn/dataproxy/MullvadProblemReport.kt line 51 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

What is this function suppose to do?

Woops. Fixed


android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/ViewLogsViewModel.kt line 25 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Should we not use the constant here?

Fixed


android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/MinLoadingConstant.kt line at r1 (raw file):

Previously, albin-mullvad wrote…

Can this file be named something more generic so that we can include other loading constants in the same file?

Maybe "TimingConstant" ?

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch from 1ac53ac to be158ba Compare September 27, 2023 13:41
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.

Reviewed 1 of 6 files at r5.
Reviewable status: 18 of 28 files reviewed, 32 unresolved discussions (waiting on @Pururun and @Rawa)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/MinLoadingConstant.kt line at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe "TimingConstant" ?

Yes, AnimationConstant or TimingConstant works! 👍

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.

Reviewed 2 of 4 files at r3, 1 of 1 files at r4, 4 of 6 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)


android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/dimensions/Dimensions.kt line 56 at r7 (raw file):

    val verticalSpace: Dp = 20.dp,
    val verticalSpacer: Dp = 1.dp,
    val dialogIconSize: Dp = 48.dp

Dimens should be in alphabetical order.

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch 2 times, most recently from 4774454 to 110bbdc Compare September 27, 2023 14:46
@linear
Copy link

linear bot commented Sep 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.

:lgtm:

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


android/lib/theme/src/main/kotlin/net/mullvad/mullvadvpn/lib/theme/dimensions/Dimensions.kt line 56 at r7 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Dimens should be in alphabetical order.

Done

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch 5 times, most recently from c9956ea to f88de14 Compare October 2, 2023 12:40
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.

Reviewed 24 of 29 files at r11, 33 of 33 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch from f88de14 to b4ebffa Compare October 2, 2023 14:16
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.

Reviewed 3 of 3 files at r13, all commit messages.
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.

:lgtm:

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

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 25 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Rawa)


-- commits line 3 at r13:
There is a new file in this commit that removed later(MinLoadingConstant.kt), any reason for it?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/LoginScreen.kt line 197 at r13 (raw file):

            visualTransformation = accountTokenVisualTransformation(),
            enabled = uiState.loginState is Idle,
            colors = mullvadWhiteTextFieldColors(),

Is this related to this PR?


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt line 211 at r13 (raw file):

@Composable
private fun ColumnScope.SendingContent() {

Will we do content through extensions?


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

                CircularProgressIndicator(
                    modifier =
                        Modifier.padding(Dimens.mediumPadding).align(Alignment.CenterHorizontally)

Is not it better to show spinner in screen center

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: all files reviewed, 4 unresolved discussions (waiting on @sabercodic)


-- commits line 3 at r13:

Previously, sabercodic wrote…

There is a new file in this commit that removed later(MinLoadingConstant.kt), any reason for it?

We renamed it to TimingConstants.kt after review.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/LoginScreen.kt line 197 at r13 (raw file):

Previously, sabercodic wrote…

Is this related to this PR?

Yes, we reused the textfield colors in another ProblemReport so I broke it out to a separate file to reuse the colors from the login screen.


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt line 211 at r13 (raw file):

Previously, sabercodic wrote…

Will we do content through extensions?

The reason is to keep the ColumnScope and not have to redefine the column in each of the states, since it is private to this file I think it is cleanest way to do it, rather than redefining the Column with padding etc. 3 times.


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

Previously, sabercodic wrote…

Is not it better to show spinner in screen center

How do you mean? Center vertically as well?

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch from b4ebffa to a610d26 Compare October 3, 2023 08:28
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: 57 of 58 files reviewed, 4 unresolved discussions (waiting on @Pururun and @sabercodic)


android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ReportProblemScreen.kt line 211 at r13 (raw file):

Previously, Rawa (David Göransson) wrote…

The reason is to keep the ColumnScope and not have to redefine the column in each of the states, since it is private to this file I think it is cleanest way to do it, rather than redefining the Column with padding etc. 3 times.

Idea is to keep it more in line of context receivers (We are in the context of a Column), rather than thinking of it as an extension function of ColumnScope. Later the syntax would be more like this:

@Composable
context(ColumnScope)
fun SendingContent() {
    ...
}

Does it make sense?

Copy link
Contributor

@sabercodic sabercodic 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 r14, 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 83 at r13 (raw file):

Previously, Rawa (David Göransson) wrote…

How do you mean? Center vertically as well?

yes

@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch 2 times, most recently from 8b1cc6c to 1a980f4 Compare October 4, 2023 07:19
@Rawa Rawa force-pushed the migrate-problem-report-view-to-compose-droid-58 branch from 1a980f4 to c83c55f Compare October 4, 2023 07:19
Copy link
Contributor

@sabercodic sabercodic 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 r15, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

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: all files reviewed, 1 unresolved discussion (waiting on @sabercodic)


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

Previously, sabercodic wrote…

yes

The loading are top aligned in the Split tunnels page, so I assumed that it was so. There was no design for showing the loading bar.

Copy link
Contributor

@sabercodic sabercodic 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: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@sabercodic sabercodic left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@Pururun Pururun merged commit 53b7f1c into main Oct 4, 2023
15 checks passed
@Pururun Pururun deleted the migrate-problem-report-view-to-compose-droid-58 branch October 4, 2023 08:32
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.

4 participants