Add filter for ownership and providers

[MaryamShaghaghi]

In this screen, users can select ownership and providers to customize their experience. The selected preferences take effect in the 'Select Location' screen upon clicking 'Apply'.

---

This change is 

[Pururun]

Reviewed 25 of 25 files at r1, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions (waiting on @MaryamShaghaghi)

---

android/app/build.gradle.kts line 4 at r1 (raw file):

import com.android.build.gradle.internal.cxx.configure.gradleLocalProperties
import com.android.build.gradle.internal.tasks.factory.dependsOn
import org.gradle.configurationcache.extensions.capitalized

This should be removed.

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt line 25 at r1 (raw file):

    @Test
    fun testFilterCells() {

Maybe this can be called defaultState or something to make it a bit clearer what we are testing here.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/button/ApplyButton.kt line 25 at r1 (raw file):

@Composable
fun ApplyButton(

⭐

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 44 at r1 (raw file):

@Composable
internal fun CheckboxCell(

Really nice cell.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 66 at r1 (raw file):

    ) {
        Box(
            modifier = Modifier.size(24.dp).background(Color.White, RoundedCornerShape(4.dp)),

We want to avoid hardcoded dp values font sizes and colors. This goes for the whole Cell.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 39 at r1 (raw file):

@Composable
fun FilterCell(

Nice cell

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 59 at r1 (raw file):

            text = stringResource(id = R.string.filtered),
            color = MaterialTheme.colorScheme.onPrimary,
            style = MaterialTheme.typography.headlineSmall.copy(fontSize = 12.sp),

We should avoid hardcoded font sizes.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 42 at r1 (raw file):

@Composable
fun MullvadFilterChip(text: String, onRemoveClick: () -> Unit) {
    FilterChip(

Same as above, we want to avoid hardcoded values in the compose components.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 103 at r1 (raw file):

                            .padding(end = Dimens.titleIconSize),
                    textAlign = TextAlign.Center,
                    style = MaterialTheme.typography.headlineSmall.copy(fontSize = 20.sp),

Hardcoded value

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 108 at r1 (raw file):

            }
        },
        bottomBar = {

Nice use of bottomBar

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 112 at r1 (raw file):

                modifier =
                    Modifier.fillMaxWidth()
                        .padding(top = 18.dp)

Hardcoded value

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 119 at r1 (raw file):

                                    backgroundColor
                                } else {
                                    backgroundColor.copy(alpha = 0.5f)

Hardcoded value

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 137 at r1 (raw file):

        },
    ) { contentPadding ->
        Column(

Is there any reason why we have column and then a LazyColumn?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayFilterState.kt line 7 at r1 (raw file):

data class RelayFilterState(
    val selectedOwnership: Ownership?,

This class should have a defined default state, either through adding default arguments in the constructor or a provided default function or INITIAL

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SelectLocationUiState.kt line 18 at r1 (raw file):

        val selectedProvidersCount: Int?,
    ) : SelectLocationUiState {
        val hasFilter: Boolean = (selectedProvidersCount != null || selectedOwnership != null)

⭐

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/FilterViewModel.kt line 76 at r1 (raw file):

    fun setSelectedProvider(checked: Boolean, provider: Provider) {
        selectedProviders.value =
            if (checked) {

⭐

---

android/app/src/main/res/layout/fragment_compose.xml line 5 at r1 (raw file):

    android:layout_width="match_parent"
    android:layout_height="match_parent">

This filed should be completely restored

[Rawa]

Nice work! I've attached some minor comments but generally very good!

Reviewed 1 of 5 files at r2, all commit messages.
Reviewable status: 21 of 25 files reviewed, 24 unresolved discussions (waiting on @MaryamShaghaghi and @Pururun)

---

android/app/build.gradle.kts line 4 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This should be removed.

I think it was just moved and java.util.* was replaced by properties, but it looks like a new line was added in the top of the file.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 33 at r2 (raw file):

@Preview
@Composable
fun CheckboxCellPreview() {

Preview should be private :)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 59 at r2 (raw file):

            modifier
                .clickable { onCheckedChange(!checked) }
                .wrapContentHeight()

Is wrapContentHeight really needed, I believe it is default?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 61 at r2 (raw file):

            style = MaterialTheme.typography.headlineSmall.copy(fontSize = 12.sp),
        )
        Spacer(modifier = Modifier.size(ButtonDefaults.IconSpacing))

It should be possible to set something along the lines of horizontalArrangement = spacedBy(ButtonDefaults.IconSpacing), then you don't have to manually insert spacing between all elements. It also removes the possibility of accidentally adding duplicate spacing (e.g if selectedProviderFilter is null there would be two spacers.)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 80 at r2 (raw file):

}

private fun Ownership.getStringResources(): Int =

Writing getStringResource is a bit Java-esk, personally I think it would be nicer to say just stringResource

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 108 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Nice use of bottomBar

Indeed, nice work!

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 119 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Hardcoded value

Why do we add an alpha to the button here? To show it as disabled? If so I think it can be part of the button instead using ButtonColors.

---

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

@Preview
@Composable
fun PreviewFilterScreen() {

Previews should have the private modifier

---

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

                    painter = painterResource(id = R.drawable.icon_back),
                    contentDescription = null,
                    modifier = Modifier.size(Dimens.titleIconSize).clickable { onBackClick() },

.clickable(::onBackClick) should be possible and a bit better.

---

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

                    .background(backgroundColor)
                    .fillMaxWidth()
                    .fillMaxHeight(),

.fillMaxWidth().fillMaxHeight() -> .fillMaxSize()

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/SelectLocationUiState.kt line 7 at r2 (raw file):

import net.mullvad.mullvadvpn.relaylist.RelayItem

sealed interface SelectLocationUiState {

😍

[MaryamShaghaghi]

Thanks David. We did that with the big help of all of you :)

Reviewable status: 5 of 25 files reviewed, 23 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/app/build.gradle.kts line 4 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I think it was just moved and java.util.* was replaced by properties, but it looks like a new line was added in the top of the file.

We need more information,
you mean that we should remove this line?
import org.gradle.configurationcache.extensions.capitalized

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt line 25 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe this can be called defaultState or something to make it a bit clearer what we are testing here.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 66 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We want to avoid hardcoded dp values font sizes and colors. This goes for the whole Cell.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 33 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Preview should be private :)

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 59 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Is wrapContentHeight really needed, I believe it is default?

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 59 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

We should avoid hardcoded font sizes.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/FilterCell.kt line 61 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

It should be possible to set something along the lines of horizontalArrangement = spacedBy(ButtonDefaults.IconSpacing), then you don't have to manually insert spacing between all elements. It also removes the possibility of accidentally adding duplicate spacing (e.g if selectedProviderFilter is null there would be two spacers.)

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/component/FilterChip.kt line 42 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Same as above, we want to avoid hardcoded values in the compose components.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 103 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Hardcoded value

We need more information. Which part needs to be changed?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 112 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Hardcoded value

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 119 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Why do we add an alpha to the button here? To show it as disabled? If so I think it can be part of the button instead using ButtonColors.

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 137 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Is there any reason why we have column and then a LazyColumn?

Done.

---

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

Previously, Rawa (David Göransson) wrote…

Previews should have the private modifier

Done.

---

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

Previously, Rawa (David Göransson) wrote…

.clickable(::onBackClick) should be possible and a bit better.

We need help to change this

---

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

Previously, Rawa (David Göransson) wrote…

.fillMaxWidth().fillMaxHeight() -> .fillMaxSize()

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayFilterState.kt line 7 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This class should have a defined default state, either through adding default arguments in the constructor or a provided default function or INITIAL

Done.

---

android/app/src/main/res/layout/fragment_compose.xml line 5 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This filed should be completely restored

Done.

[Pururun]

Nice, just need to look through the dimensions a little bit more.

Reviewed 2 of 5 files at r2, 15 of 18 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @MaryamShaghaghi and @Rawa)

---

android/app/build.gradle.kts line 4 at r1 (raw file):

Previously, MaryamShaghaghi (Maryam Shaghaghi) wrote…

We need more information,
you mean that we should remove this line?
import org.gradle.configurationcache.extensions.capitalized

This whole file should be restored and not part of this PR.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 66 at r1 (raw file):

Previously, MaryamShaghaghi (Maryam Shaghaghi) wrote…

Done.

Instead of using dimensions here, you could use MaterialTheme.shapes.small directly

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 103 at r1 (raw file):

Previously, MaryamShaghaghi (Maryam Shaghaghi) wrote…

We need more information. Which part needs to be changed?

The font size, but it seems you have already done this?

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/RelayFilterState.kt line 7 at r1 (raw file):

Previously, MaryamShaghaghi (Maryam Shaghaghi) wrote…

Done.

Nice

---

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

    val verticalSpacer: Dp = 1.dp,
    val smallIconSize: Dp = 16.dp,
    val checkBoxSize: Dp = 24.dp,

Consider these values and if they really should be used.
Most are not standard material design values (4, 8, 12, 16 etc) and some probably have quite similar values you can use.
For example we have a several already defined dimensions that have padding and spacing.

[MaryamShaghaghi]

Reviewable status: 23 of 25 files reviewed, 9 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt line 66 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Instead of using dimensions here, you could use MaterialTheme.shapes.small directly

Done.

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Consider these values and if they really should be used.
Most are not standard material design values (4, 8, 12, 16 etc) and some probably have quite similar values you can use.
For example we have a several already defined dimensions that have padding and spacing.

Yes we can see these, but is it ok to use them? We just think that the naming is not matching for our case. val searchIconSize: Dp = 24.dp, val switchIconSize: Dp = 24.dp,

[MaryamShaghaghi]

Reviewable status: 20 of 25 files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt line 103 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

The font size, but it seems you have already done this?

Done.

---

android/app/build.gradle.kts line 4 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This whole file should be restored and not part of this PR.

Done.

[Pururun]

Reviewed 2 of 2 files at r6, 6 of 6 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Rawa]

Reviewed 19 of 19 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved