Android GL maps #5797
Android GL maps #5797
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
biasandroid/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
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
nit: I suggest switching to
LatLong
Thanks for fixing 🙂
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
Temporary ignore during development?
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 = 2500android/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):
Previously, albin-mullvad wrote…
This is identical or very similar to the desktop shared, right? Can we add a comment with a link to that one perhaps?
desktop shader*
There was a problem hiding this comment.
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):
Previously, Pururun (Jonatan Rhodin) wrote…
Magical number
Fixed
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 208 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
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?
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):
Previously, Pururun (Jonatan Rhodin) wrote…
See comment above about debug ui features.
Removed
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 196 at r2 (raw file):
Previously, albin-mullvad wrote…
Can this variable be clarified a bit?
This was very much VIP, but fixed now.
android/lib/map/build.gradle.kts line 12 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Maybe nitpick, but I guess we don't need a runner if we don't have any tests?
Done.
android/lib/map/build.gradle.kts line 44 at r3 (raw file):
Previously, albin-mullvad wrote…
Bad file ending. Maybe we should add an automatic check for that if it's not already checked?
Done.
android/lib/map/src/androidTest/java/net/mullvad/mullvadvpn/lib/map/ExampleInstrumentedTest.kt line 17 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Should probably be removed
Done.
android/lib/map/src/main/AndroidManifest.xml line 4 at r2 (raw file):
Previously, albin-mullvad wrote…
Bad file ending. Also, the manifest tag can be single line:
<manifest ... />
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 40 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Would this be considered a log that could potentially leak data?
Used while developing, removed
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 45 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Since this is a composable, should it start with a capital letter? Not sure about the rules exactly here.
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 49 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This whole function is good :)
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/GlobeColors.kt line 13 at r2 (raw file):
Previously, albin-mullvad wrote…
Do we need backing fields for these or can they be "plain" getters?
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):
Previously, albin-mullvad wrote…
Bad file ending
Done.
There was a problem hiding this comment.
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):
Previously, Pururun (Jonatan Rhodin) wrote…
Would it not be better to call it something like fallbackLatLng instead of gothenburgLatLng?
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 201 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Maybe we should use named parameters everywhere here to make it clearer what the different locations are used for?
Done.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 249 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Why was this removed?
Irrelevant since last push I believe
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/ConnectScreen.kt line 347 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This should probably be a constant somewhere, and be call fallback or default position.
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 99 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Magical numbers
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 108 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Magical number
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 112 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Magical number
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 88 at r3 (raw file):
Previously, albin-mullvad wrote…
Would be nice to not define these constants as part of the
Mapitself, but rather include some type of configuration structure.
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):
Previously, Pururun (Jonatan Rhodin) wrote…
Magical numbers
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):
Previously, Pururun (Jonatan Rhodin) wrote…
Magic number
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):
Previously, albin-mullvad wrote…
nit: what does this empty line represent? It's probably most clear to either skip it or to add comments for each "section"
Fixed
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 61 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Could vp be replaced by something more verbose?
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 36 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
These colors can be defined right?
Fixed
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/MapConfig.kt line 8 at r3 (raw file):
Previously, albin-mullvad wrote…
Some magic colors here. I guess these should be defined somewhere?
Fixed
android/lib/map/src/main/res/raw/fragment_shader.glsl line 1 at r3 (raw file):
Previously, albin-mullvad wrote…
This is identical or very similar to the desktop shader, right? Can we add a comment with a link to that one perhaps?
Removed and added comment in code.
android/lib/map/src/main/res/raw/vertex_shader.glsl line 1 at r1 (raw file):
Previously, albin-mullvad wrote…
desktop shader*
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):
Previously, albin-mullvad wrote…
Can probably be removed?
Gone!
|
Fixes: @#5581 |
Rawa
force-pushed
the
android-gl-maps
branch
from
7d909de
to
6de3588
Compare
There was a problem hiding this comment.
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 0fandroid/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:
dependsOnandroid/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/Map.kt line 88 at r3 (raw file):
Previously, Rawa (David Göransson) wrote…
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.
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):
Previously, Rawa (David Göransson) wrote…
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.
Fair enough 👍
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):
Previously, Rawa (David Göransson) wrote…
Moved them out to companion object, do we a even have a better idea where to place them?
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_CALLSThere was a problem hiding this comment.
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)
Rawa
force-pushed
the
android-gl-maps
branch
from
6de3588
to
14686d4
Compare
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
Not pretty, but I guess I can blame
ktfmt? 😆
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):
Previously, albin-mullvad wrote…
Is there any risk of this floating point being zero?
Fixed!
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/data/LocationMarkerColors.kt line 8 at r2 (raw file):
Previously, albin-mullvad wrote…
Sounds good! 👍
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLSurfaceView.kt line 15 at r9 (raw file):
Previously, albin-mullvad wrote…
Should any of these perhaps be disabled or used depending on build type?
True! Good catch!
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
Gothenburg coordinates? Would perhaps be nice to clarify as a comment or as part of the name in that case?
Clarified with comment
Rawa
force-pushed
the
android-gl-maps
branch
from
14686d4
to
175e3f3
Compare
There was a problem hiding this 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):
Previously, Rawa (David Göransson) wrote…
Fixed!
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?
Rawa
force-pushed
the
android-gl-maps
branch
from
175e3f3
to
99f0476
Compare
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
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 🤔
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):
Previously, albin-mullvad wrote…
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 🤔
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):
Previously, albin-mullvad wrote…
typo: should the last
$valuebe part of this error message?
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):
Previously, Pururun (Jonatan Rhodin) wrote…
I guess we can put into some kind of compose-ui module?
Managed to remove it completely
There was a problem hiding this comment.
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):
Previously, Rawa (David Göransson) wrote…
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.
Nice 👍
Rawa
force-pushed
the
android-gl-maps
branch
from
99f0476
to
87ed9e3
Compare
There was a problem hiding this comment.
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):
Previously, Rawa (David Göransson) wrote…
Moved them out to companion object, do we a even have a better idea where to place them?
I think that is fine
There was a problem hiding this comment.
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:
animatedCameraPositionandroid/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:
shadowandroid/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:
whiteandroid/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:
numEdgesandroid/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:
removeandroid/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? 🤔
There was a problem hiding this comment.
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)
Rawa
force-pushed
the
android-gl-maps
branch
2 times, most recently
from
901f0c7
to
6dd7e64
Compare
There was a problem hiding this comment.
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):
Previously, Pururun (Jonatan Rhodin) wrote…
For clarity we could use
AlphaVisibleandAlphaInvisiblehere
Fixed
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt line 26 at r16 (raw file):
Previously, albin-mullvad wrote…
Capitalize
Done.
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/MapGLRenderer.kt line 47 at r16 (raw file):
Previously, albin-mullvad wrote…
nit: remove empty line?
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):
Previously, albin-mullvad wrote…
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?
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):
Previously, albin-mullvad wrote…
Potential divide by zero (float)
Fixed
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/Globe.kt line 105 at r16 (raw file):
Previously, albin-mullvad wrote…
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"
Added comments
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/internal/shapes/LocationMarker.kt line 57 at r16 (raw file):
Previously, albin-mullvad wrote…
Would be nice to move these so that all
vals are grouped
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):
Previously, albin-mullvad wrote…
Potential divide by zero
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):
Previously, albin-mullvad wrote…
Maybe a dumb question: can this
elseoccur? Aren't all the cases already covered? 🤔
It should not be able to occur but kotlin does not understand that 😞
There was a problem hiding this comment.
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):
Previously, albin-mullvad wrote…
Would be nice to not pass along resources down to the renderer and globe.
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):
Previously, albin-mullvad wrote…
Might be a good idea to introduce some type of factory if we need to instantiate the globe (if we can't inject it)
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!
There was a problem hiding this comment.
Reviewed 5 of 5 files at r17, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @albin-mullvad)
Rawa
force-pushed
the
android-gl-maps
branch
from
6dd7e64
to
75b8eb6
Compare
Rawa
force-pushed
the
android-gl-maps
branch
from
75b8eb6
to
db41792
Compare
There was a problem hiding this comment.
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):
Previously, Rawa (David Göransson) wrote…
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?
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):
Previously, Rawa (David Göransson) wrote…
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!
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):
Previously, Rawa (David Göransson) wrote…
I removed the rings as a class value and reorganized them a bit, is it more clear now?
Yes, looks good!
android/lib/model/src/main/kotlin/net/mullvad/mullvadvpn/model/Latitude.kt line 51 at r16 (raw file):
Previously, Rawa (David Göransson) wrote…
It should not be able to occur but kotlin does not understand that 😞
Indeed 😢
This PR introduces a map view that renders maps in OpenGL
This change is