-
Notifications
You must be signed in to change notification settings - Fork 354
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
Improve animation duration calculation #5991
Improve animation duration calculation #5991
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Rawa)
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt
line 100 at r1 (raw file):
} private fun Float.toAnimationDuration() =
This is just a preference, but since we are doing some kind of conversion from float to int here, could we set the return type explicitly here so that if we do for some reason change this function and the return type becomes int it will show an error immediately?
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
import kotlin.math.sqrt import kotlin.test.assertTrue
Use jupiter assertTrue
ad1a721
to
10a50f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @Pururun)
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt
line 100 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
This is just a preference, but since we are doing some kind of conversion from float to int here, could we set the return type explicitly here so that if we do for some reason change this function and the return type becomes int it will show an error immediately?
Good point! I'll add it.
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
Use jupiter assertTrue
🙏 Thanks!
There was a problem hiding this 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 r1, 4 of 5 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @Pururun and @Rawa)
android/lib/map/src/main/kotlin/net/mullvad/mullvadvpn/lib/map/CameraAnimation.kt
line 100 at r3 (raw file):
} private fun Float.toAnimationDuration(): Int =
I suggest renaming to toAnimationDurationMillis
to avoid any risk of confusion
Code quote:
toAnimationDuration
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
🙏 Thanks!
Was this detected by Detekt? Otherwise it might be a good idea to such a rule
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 23 at r3 (raw file):
@Test fun `ensure seppDistance respects lateral value is respected`() {
typo?
Code quote:
respects lateral value is respected
10a50f8
to
c88e120
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 6 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
Previously, albin-mullvad wrote…
Was this detected by Detekt? Otherwise it might be a good idea to such a rule
Unfortunately not :( , I'll add an issue to add a Konsist rule:
https://linear.app/mullvad/issue/DROID-804/add-konsist-rule-to-block-usage-of-kotlintestasserttrue
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 23 at r3 (raw file):
Previously, albin-mullvad wrote…
typo?
Yes! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @Pururun)
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
Previously, Rawa (David Göransson) wrote…
Unfortunately not :( , I'll add an issue to add a Konsist rule:
https://linear.app/mullvad/issue/DROID-804/add-konsist-rule-to-block-usage-of-kotlintestasserttrue
Nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @Pururun)
android/lib/model/src/test/kotlin/net/mullvad/mullvadvpn/model/LatLongTest.kt
line 4 at r1 (raw file):
Previously, albin-mullvad wrote…
Nice 👍
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
c88e120
to
9d35783
Compare
This PR implements a new algorithm to evaluate the distance between two location that is then used to decide which type of animation we are using.
This change is