Skip to content

[Bug] : Fix isMinimumTimeBeforeRecalcExceeded#755

Merged
ianthetechie merged 6 commits into
stadiamaps:mainfrom
radityagumay:bug/wrong_time
Dec 19, 2025
Merged

[Bug] : Fix isMinimumTimeBeforeRecalcExceeded#755
ianthetechie merged 6 commits into
stadiamaps:mainfrom
radityagumay:bug/wrong_time

Conversation

@radityagumay

@radityagumay radityagumay commented Dec 17, 2025

Copy link
Copy Markdown
Contributor

Description

Earlier the time comparison is using nanoseconds instead of seconds, the PR essentially to use TimeUnit.SECONDS to fix aggressive nano time usage.

In this PR i also extract the functionality into a function so that i can add a unit test to verify the logic.

Safety net

Providing unit tests to validate the logic < and > of 5 seconds

@ianthetechie

Copy link
Copy Markdown
Contributor

For future PRs, note that we use ktFmtFormat to avoid this sort of formatting noise 😅 https://stadiamaps.github.io/ferrostar/dev-env-setup.html#pr-checklist-3

@ianthetechie

Copy link
Copy Markdown
Contributor

Wow, good catch! I've done a clean-up pass and think this is good to merge + release after CI checks pass.

The original PR actually introduced a subtle bug that didn't preserve the original behavior in case there was no prior recalculation. This wasn't picked up in the new unit tests but was luckily caught an unrelated integration test failure designed to exercise that part of the code 😅

I've fixed the issue and made a few other tweaks:

  • Moved the unit tested function out to the top level to make the dependencies explicit + improve testability. It was not obvious that the minimum cooldown (not usually explicitly specified) defaults to 5 seconds.
  • Moved the tests out to a different package; unfortunately most stuff has to go in connected checks for the moment due to some shortcomings in the testing stack when it comes to the JNA bindings, but the new functions are pure Kotlin, so we can move them out.
  • Fixed the public API (breaking change) to use the Kotlin 1.6+ Duration API. This way it's impossible to get confused when setting the cooldown. We would have done that from the start, but I don't think the API was stable 3 years ago :)

@ianthetechie
ianthetechie merged commit a89da35 into stadiamaps:main Dec 19, 2025
14 checks passed
@radityagumay

Copy link
Copy Markdown
Contributor Author

Thank you ian!

return _lastRecalculationLocation?.let {
it.toAndroidLocation().distanceTo(location.toAndroidLocation()) >
minimumMovementBeforeRecalculation
} ?: true // Default to true if no prior automatic recalculation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is make sense, i am about to make a changes of this. thank you @ianthetechie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants