Reviewed 39 of 39 files at r1, 4 of 18 files at r2.
Reviewable status: 29 of 43 files reviewed, 20 unresolved discussions (waiting on @Rawa)

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

        timeLeft = uiState.daysLeftUntilExpiry
    ) {
        var bias by remember { mutableFloatStateOf(0.5f) }

Magical number

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

            modifier = Modifier.padding(top = it.calculateTopPadding()),
            uiState.animateMap,
            uiState.location?.toLatLng() ?: gothenburgLatLng,

Would it not be better to call it something like fallbackLatLng instead of gothenburgLatLng?

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

            uiState.animateMap,
            uiState.location?.toLatLng() ?: gothenburgLatLng,
            marker = uiState.tunnelRealState.toMarker(uiState.location),

Maybe we should use named parameters everywhere here to make it clearer what the different locations are used for?

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

        ) {
            val (divider) = createRefs()
            Divider(

I guess this is is one kind of debug line to use for testing? If we want to keep it maybe we should hide it behind a flag?

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

                onClickDismissNewDevice = onDismissNewDeviceClick,
            )
            Slider(

See comment above about debug ui features.

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

            if (
                uiState.tunnelRealState is TunnelState.Connecting ||
                    uiState.tunnelRealState is TunnelState.Disconnecting

Why was this removed?

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

fun GeoIpLocation.toLatLng() = LatLng(Latitude(latitude.toFloat()), Longitude(longitude.toFloat()))

val gothenburgLatLng = LatLng(Latitude(57.7065f), Longitude(11.967f))

This should probably be a constant somewhere, and be call fallback or default position.

android/lib/map/build.gradle.kts line 12 at r1 (raw file):

    defaultConfig {
        minSdk = Versions.Android.minSdkVersion
        testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"

Maybe nitpick, but I guess we don't need a runner if we don't have any tests?

android/lib/map/src/androidTest/java/net/mullvad/mullvadvpn/lib/map/ExampleInstrumentedTest.kt line 17 at r1 (raw file):

 */
@RunWith(AndroidJUnit4::class)
class ExampleInstrumentedTest {

Should probably be removed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 40 at r1 (raw file):

            )
        }
    Log.d("MullvadMap", "CameraLocation: ${mapViewState.cameraLatLng}")

Would this be considered a log that could potentially leak data?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 45 at r1 (raw file):

@Composable
fun animatedMapViewState(

Since this is a composable, should it start with a capital letter? Not sure about the rules exactly here.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 49 at r1 (raw file):

    marker: Marker?,
    percent: Float,
): MapViewState {

This whole function is good :)

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 99 at r1 (raw file):

                        } else {
                            durationMillis = duration
                            1.25f at duration / 3 using EaseInOut

Magical numbers

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 108 at r1 (raw file):

    LaunchedEffect(marker?.type) {
        launch { secureZoomAnimation.animateTo(targetValue = marker?.type.toZoom(), tween(2000)) }

Magical number

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 112 at r1 (raw file):

    return MapViewState(
        zoom = secureZoomAnimation.value * zoomOutMultiplier.value * 0.9f,

Magical number

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/RememberPrevious.kt line 13 at r1 (raw file):

import androidx.compose.runtime.remember

// TODO this file was copied for now and should be removed/broken out to a new module

I guess we can put into some kind of compose-ui module?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):

    val centerColor: Color,
    val ringBorderColor: Color = Color.White,
    val shadowColor: Color = Color.Black.copy(alpha = 0.55f),

Magical numbers

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/Marker.kt line 5 at r1 (raw file):

import net.mullvad.mullvadvpn.model.LatLng

data class Marker(val latLng: LatLng, val type: MarkerType, val size: Float = 0.02f)

Magic number

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 61 at r1 (raw file):

        val vp = viewMatrix.copyOf()

Could vp be replaced by something more verbose?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 36 at r1 (raw file):

    val globeColors: GlobeColors =
        GlobeColors(
            landColor = Color(0.16f, 0.302f, 0.45f),

These colors can be defined right?

Reviewed 12 of 39 files at r1, 1 of 18 files at r2.
Reviewable status: 29 of 43 files reviewed, 26 unresolved discussions (waiting on @Pururun and @Rawa)

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

        timeLeft = uiState.daysLeftUntilExpiry
    ) {
        var bias by remember { mutableFloatStateOf(0.5f) }

Can this variable be clarified a bit?

Code quote:

bias

android/config/detekt.yml line 764 at r2 (raw file):

    excludeCommentStatements: false
    excludeRawStrings: true
    ignoreAnnotated: ['Test']

Temporary ignore during development?

Code quote:

ignoreAnnotated: ['Test']

android/lib/map/src/main/AndroidManifest.xml line 4 at r2 (raw file):

<manifest xmlns:android="http://schemas.android.com/apk/res/android">

</manifest>

Bad file ending. Also, the manifest tag can be single line: <manifest ... />

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/GlobeColors.kt line 13 at r2 (raw file):

    val landColorArray = landColor.toFloatArray()
    val oceanColorArray = oceanColor.toFloatArray()
    val contourColorArray = contourColor.toFloatArray()

Do we need backing fields for these or can they be "plain" getters?

Code quote:

    val landColorArray = landColor.toFloatArray()
    val oceanColorArray = oceanColor.toFloatArray()
    val contourColorArray = contourColor.toFloatArray()

android/lib/map/src/main/res/raw/vertex_shader.glsl line 9 at r2 (raw file):

    gl_Position = uProjectionMatrix * uModelViewMatrix * vec4(aVertexPosition, 1.0);
    vColor = uColor;
}

Bad file ending

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/LatLng.kt line 6 at r2 (raw file):

import kotlin.math.sqrt

data class LatLng(val latitude: Latitude, val longitude: Longitude) {

nit: I suggest switching to LatLong

Code quote:

LatLng

Reviewed 7 of 39 files at r1, 3 of 18 files at r2, 2 of 10 files at r3.
Reviewable status: 28 of 43 files reviewed, 28 unresolved discussions (waiting on @Pururun and @Rawa)

android/lib/map/build.gradle.kts line 44 at r3 (raw file):

    implementation(Dependencies.AndroidX.lifecycleRuntimeKtx)
}

Bad file ending. Maybe we should add an automatic check for that if it's not already checked?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/ShaderComposable.kt line 1 at r3 (raw file):

package net.mullvad.mullvadvpn.lib.map.internal

Can probably be removed?

android/lib/map/src/main/res/raw/vertex_shader.glsl line 1 at r1 (raw file):

attribute vec3 aVertexPosition;

This is identical or very similar to the desktop shared, right? Can we add a comment with a link to that one perhaps?

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/LatLng.kt line 6 at r2 (raw file):

Thanks for fixing 🙂

Reviewable status: 28 of 43 files reviewed, 31 unresolved discussions (waiting on @Pururun and @Rawa)

android/config/detekt.yml line 764 at r2 (raw file):

Seems fixed now

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 88 at r3 (raw file):

const val SHORT_ANIMATION_MILLIS = 1700
const val MIN_ANIMATION_MILLIS = 1300
const val MAX_ANIMATION_MILLIS = 2500

Would be nice to not define these constants as part of the Map itself, but rather include some type of configuration structure.

Code quote:

const val SECURE_ZOOM = 1.25f
const val UNSECURE_ZOOM = 1.35f
const val DISTANCE_DURATION_SCALE_FACTOR = 20
const val SHORT_ANIMATION_MILLIS = 1700
const val MIN_ANIMATION_MILLIS = 1300
const val MAX_ANIMATION_MILLIS = 2500

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/MapConfig.kt line 8 at r3 (raw file):

    val globeColors: GlobeColors =
        GlobeColors(
            landColor = Color(0.16f, 0.302f, 0.45f),

Some magic colors here. I guess these should be defined somewhere?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/Constants.kt line 5 at r3 (raw file):

internal const val VERTEX_COMPONENT_SIZE = 3
internal const val COLOR_COMPONENT_SIZE = 4

nit: what does this empty line represent? It's probably most clear to either skip it or to add comments for each "section"

android/lib/map/src/main/res/raw/fragment_shader.glsl line 1 at r3 (raw file):

varying lowp vec4 vColor;

This is identical or very similar to the desktop shader, right? Can we add a comment with a link to that one perhaps?

android/lib/map/src/main/res/raw/vertex_shader.glsl line 1 at r1 (raw file):

desktop shader*

Reviewable status: 27 of 46 files reviewed, 31 unresolved discussions (waiting on @albin-mullvad and @Pururun)

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

Fixed

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

It was used for creating the view, it has been removed now.

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

Removed

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

This was very much VIP, but fixed now.

android/lib/map/build.gradle.kts line 12 at r1 (raw file):

Done.

android/lib/map/build.gradle.kts line 44 at r3 (raw file):

Done.

android/lib/map/src/androidTest/java/net/mullvad/mullvadvpn/lib/map/ExampleInstrumentedTest.kt line 17 at r1 (raw file):

Done.

android/lib/map/src/main/AndroidManifest.xml line 4 at r2 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 40 at r1 (raw file):

Used while developing, removed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 45 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 49 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/GlobeColors.kt line 13 at r2 (raw file):

I'm not sure how much it would impact performance but since we use land, ocean and contour color every time we draw I felt i was unnecessary to convert it to FloatArray each single Draw.

android/lib/map/src/main/res/raw/vertex_shader.glsl line 9 at r2 (raw file):

Done.

Reviewable status: 17 of 48 files reviewed, 29 unresolved discussions (waiting on @albin-mullvad and @Pururun)

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

Done.

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

Done.

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

Irrelevant since last push I believe

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

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 99 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 108 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 112 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 88 at r3 (raw file):

I've fixed some of these remarks. Map module now provides a default camera animation with contains the last 4 properties, the secure/unsecure zoom has now been removed and replaced with baseZoom which the user of the library can provide. If someone want to do other animations we they can always implement their own animation.

Do you still feel we should make the animation part more open/flexible? I'm open for discussion/ideas.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):

Moved them out to companion object, do we a even have a better idea where to place them?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/Marker.kt line 5 at r1 (raw file):

Moved them out to companion object, do we a even have a better idea where to place them?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/Constants.kt line 5 at r3 (raw file):

Fixed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 61 at r1 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 36 at r1 (raw file):

Fixed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/MapConfig.kt line 8 at r3 (raw file):

Fixed

android/lib/map/src/main/res/raw/fragment_shader.glsl line 1 at r3 (raw file):

Removed and added comment in code.

android/lib/map/src/main/res/raw/vertex_shader.glsl line 1 at r1 (raw file):

Removed and added comment in code.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/ShaderComposable.kt line 1 at r3 (raw file):

Gone!

Amazing! 🤩

Reviewed 1 of 10 files at r3, 2 of 7 files at r6, 11 of 24 files at r7, 3 of 6 files at r8, 9 of 13 files at r9.
Reviewable status: 31 of 49 files reviewed, 23 unresolved discussions (waiting on @Pururun and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 270 at r9 (raw file):

                            )
                                1f
                            else 0f

Not pretty, but I guess I can blame ktfmt? 😆

Code quote:

                            if (
                                uiState.tunnelRealState is TunnelState.Connecting ||
                                    uiState.tunnelRealState is TunnelState.Disconnecting
                            )
                                1f
                            else 0f

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 277 at r9 (raw file):

                            val offsetY = it.positionInParent().y + it.size.height / 2
                            progressIndicatorBias =
                                offsetY / it.parentLayoutCoordinates?.size?.height?.toFloat()!!

Is there any risk of this floating point being zero?

Code quote:

it.parentLayoutCoordinates?.size?.height?.toFloat()!!

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/AnimationConstant.kt line 19 at r9 (raw file):

const val UNSECURE_ZOOM = 1.20f
const val SECURE_ZOOM_ANIMATION_MILLIS = 2000
val fallbackLatLong = LatLong(Latitude(57.7065f), Longitude(11.967f))

Gothenburg coordinates? Would perhaps be nice to clarify as a comment or as part of the name in that case?

android/lib/map/build.gradle.kts line 1 at r9 (raw file):

import com.android.build.gradle.internal.tasks.factory.dependsOn

Probably no longer needed (?)

Code quote:

dependsOn

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 88 at r3 (raw file):

Nice, sounds good! We don't need to make it more open or flexible at this point imo

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/GlobeColors.kt line 13 at r2 (raw file):

Fair enough 👍

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):

Sounds good! 👍

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 15 at r9 (raw file):

        setEGLContextClientVersion(2)

        debugFlags = DEBUG_CHECK_GL_ERROR or DEBUG_LOG_GL_CALLS

Should any of these perhaps be disabled or used depending on build type?

Code quote:

debugFlags = DEBUG_CHECK_GL_ERROR or DEBUG_LOG_GL_CALLS

Reviewed 1 of 7 files at r6, 1 of 13 files at r9, 1 of 2 files at r10, 4 of 4 files at r12, all commit messages.
Reviewable status: 38 of 49 files reviewed, 23 unresolved discussions (waiting on @Pururun and @Rawa)

Reviewable status: 35 of 49 files reviewed, 20 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 270 at r9 (raw file):

Yes, ktfmt :( but I've fixed by doing some refactoring

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 277 at r9 (raw file):

Fixed!

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 15 at r9 (raw file):

True! Good catch!

Reviewable status: 35 of 49 files reviewed, 20 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/constant/AnimationConstant.kt line 19 at r9 (raw file):

Clarified with comment

Reviewed 1 of 39 files at r1, 1 of 2 files at r10, 1 of 1 files at r11, 3 of 3 files at r13, 5 of 6 files at r14, all commit messages.
Reviewable status: 40 of 49 files reviewed, 21 unresolved discussions (waiting on @Pururun and @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 277 at r9 (raw file):

The following can still potentially (if height=0f) be zero causing a division by zero (float)? Or perhaps I'm missing something. Even though it might be mostly theoretical issue I believe it could be nice to somehow safe-guard 🤔

Code snippet:

it.size.height.toFloat()

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 216 at r12 (raw file):

    }
        """
                .trimIndent()

Just a thought: maybe it would make sense to move the shader code to some separate class/file to avoid cluttering this file? Also would make it possible to share between the markers and globe if the shaders are the identical 🤔

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 9 at r14 (raw file):

    init {
        require(value in LATITUDE_RANGE) {
            "Latitude, $value, must be between $MIN_LATITUDE_VALUE and $MAX_LATITUDE_VALUE $value"

typo: should the last $value be part of this error message?

Reviewable status: 40 of 49 files reviewed, 21 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 277 at r9 (raw file):

Yeah, it would be if the view is for some reason height of 0 pixels. Not sure if this can happen but added an if statement to check it.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 216 at r12 (raw file):

They are indeed identical for the fragmentshader code but the vertexshader are different (very slightly but still different, maybe they could be the same?). It would be a bit weird to separate only the fragmentshader code but not the vertexshader.

I'm also not sure exactly how they might change over time and they are separated in the desktop app thus I kept them separated. For now I feel they are the closest to the object that uses the shader, if they are the same for multiple objects I think it makes sense to start looking at refactoring it out from the object itself.

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 9 at r14 (raw file):

Nice find, I also clarified the error message a little bit.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/RememberPrevious.kt line 13 at r1 (raw file):

Managed to remove it completely

Reviewed 2 of 10 files at r15.
Reviewable status: 36 of 49 files reviewed, 19 unresolved discussions (waiting on @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 277 at r9 (raw file):

Nice 👍

Reviewed 1 of 5 files at r4, 1 of 7 files at r6, 1 of 24 files at r7, 1 of 13 files at r9, 1 of 6 files at r14, 7 of 10 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 245 at r16 (raw file):

                            top = Dimens.mediumPadding
                        )
                        .alpha(if (uiState.showLoading) 1f else 0f)

For clarity we could use AlphaVisible and AlphaInvisible here

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/Marker.kt line 5 at r1 (raw file):

I think that is fine

Reviewed 1 of 24 files at r7, 1 of 13 files at r9, 1 of 6 files at r14, 7 of 10 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @Pururun and @Rawa)

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt line 26 at r16 (raw file):

@Composable
fun animatedCameraPosition(

Capitalize

Code quote:

animatedCameraPosition

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 18 at r16 (raw file):

import net.mullvad.mullvadvpn.model.COMPLETE_ANGLE

internal class MapGLRenderer(private val resources: Resources) : GLSurfaceView.Renderer {

Would be nice to not pass along resources down to the renderer and globe.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 39 at r16 (raw file):

    override fun onSurfaceCreated(unused: GL10, config: EGLConfig) {
        globe = Globe(resources)

Might be a good idea to introduce some type of factory if we need to instantiate the globe (if we can't inject it)

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 47 at r16 (raw file):

        GLES20.glEnable(GLES20.GL_CULL_FACE)
        GLES20.glCullFace(GLES20.GL_BACK)

nit: remove empty line?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 54 at r16 (raw file):

    private val projectionMatrix = newIdentityMatrix()

    override fun onDrawFrame(gl10: GL10) {

nit: there are many empty lines in this function and it's a bit hard to intepret what they signify. Is it intentional or can it be cleaned up a bit? Maybe adding some comment would be another way of clarifying the code?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 106 at r16 (raw file):

        GLES20.glViewport(0, 0, width, height)

        val ratio: Float = width.toFloat() / height.toFloat()

Potential divide by zero (float)

Code quote:

height.toFloat()

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/Globe.kt line 105 at r16 (raw file):

            LAND_OCEAN_SCALE_FACTOR
        )
        drawBufferElements(

nit: it's a bit inconsistent with empty lines between some of the function calls. I suggest either making it consistent or adding comments if the lines are used as "code sections"

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 34 at r16 (raw file):

                colors.shadowColor,
                colors.shadowColor.copy(alpha = 0.0f),
            ), // shadow

nit: capitalize

Code quote:

shadow

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 41 at r16 (raw file):

                colors.ringBorderColor,
                colors.ringBorderColor,
            ), // white ring

nit: capitalize

Code quote:

white

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 57 at r16 (raw file):

    private data class AttribLocations(val vertexPosition: Int, val vertexColor: Int)

    private data class UniformLocation(val projectionMatrix: Int, val modelViewMatrix: Int)

Would be nice to move these so that all vals are grouped

Code quote:

    private data class AttribLocations(val vertexPosition: Int, val vertexColor: Int)

    private data class UniformLocation(val projectionMatrix: Int, val modelViewMatrix: Int)

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 157 at r16 (raw file):

        for (i in 1 until points) {

            val angle = (i.toFloat() / numEdges) * 2f * Math.PI

Potential divide by zero

Code quote:

numEdges

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 36 at r16 (raw file):

        private fun unwind(value: Float): Float {
            // remove all 360 degrees

Capitalize

Code quote:

remove

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 51 at r16 (raw file):

                partiallyUnwound < MIN_LATITUDE_VALUE ->
                    MIN_LATITUDE_VALUE - (partiallyUnwound % MIN_LATITUDE_VALUE)
                else -> error("Unwound value $partiallyUnwound is not within valid latitude range")

Maybe a dumb question: can this else occur? Aren't all the cases already covered? 🤔

Reviewed 1 of 5 files at r4, 1 of 7 files at r6.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @Pururun and @Rawa)

Reviewable status: 44 of 49 files reviewed, 9 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 245 at r16 (raw file):

Fixed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt line 26 at r16 (raw file):

Done.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 47 at r16 (raw file):

I grouped them because they are related, first we enable GL_CULL_FACE feature and then configure it, same for GL_BLEND. I've added comments to make it more clear.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 54 at r16 (raw file):

There are some intentions into the grouping of code, I've added comments to clarify

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 106 at r16 (raw file):

Fixed

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/Globe.kt line 105 at r16 (raw file):

Added comments

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 57 at r16 (raw file):

I removed the rings as a class value and reorganized them a bit, is it more clear now?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 157 at r16 (raw file):

Added a require higher up to make sure numEdges is above 2.

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 51 at r16 (raw file):

It should not be able to occur but kotlin does not understand that 😞

Reviewable status: 44 of 49 files reviewed, 9 unresolved discussions (waiting on @albin-mullvad and @Pururun)

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 18 at r16 (raw file):

I'd agree that it would be nice, but at the same time it sort of requires the resources to be able to load in it's assets. The alternative it to provide the 5 ByteArray already loaded into memory. Feels like that might be more odd?

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 39 at r16 (raw file):

I'm not sure how a factory would help in this case, would it be to avoid passing the resources? Where would we create the factory? And whom would be responsible for it? Lets discuss offline, I'm open for ideas!

Reviewed 5 of 5 files at r17, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @albin-mullvad)

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

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 18 at r16 (raw file):

As discussed offline we can let it be as-is for now.

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 39 at r16 (raw file):

As discuss, we'll keep it as-is for now

android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 57 at r16 (raw file):

Yes, looks good!

android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 51 at r16 (raw file):

Indeed 😢