Conversation
Julius-Babies
left a comment
There was a problem hiding this comment.
Small changes, someone else needs to approve this since I started this pr and therefore cannot approve it.
| provider, | ||
| 1000L, // 1 Sekunde | ||
| 1f, // 1 Meter | ||
| 1000L, |
There was a problem hiding this comment.
So-called "Magic-numbers" should be defined using a constant at a somewhat top level given the importance of those. See https://en.wikipedia.org/wiki/Magic_number_(programming)?useskin=vector
There was a problem hiding this comment.
Also include the unit in the constant name. I suggest using Kotlin's const-keyword (see https://kotlinlang.org/docs/properties.html#compile-time-constants)
There was a problem hiding this comment.
This file seems rather large, consider refactoring some components into useful separate files enhancing reusability and testability in the future. However, this is not necessary yet so feel free to just skip this :)
…on detection radius from 200m to 250m for better user experience - Fix S-Bahn line announcements to avoid Gleis prefix - Improve platform name handling to remove redundant Gleis prefixes - Fix German text for eine Minute delay announcement - Refactor line announcement logic for different transport types: S-Bahn: S1 in Richtung X an Gleis Y, Trains: Gleis RE1 in Richtung X an Gleis Y, Bus/Tram: Linie 62 in Richtung X an Steig Y
…object with location-related constants - Add provider-specific time and distance thresholds - Add sensor-related constants - Improve code maintainability and readability - Make configuration values easier to modify
…ffixes to constant names (METERS, MILLIS, NANOS) - Use Kotlin const keyword for compile-time constants - Remove redundant comments as units are now in names - Update all constant references throughout the code - Improve code readability and maintainability
Refactor MainActivity to use Compose and MVVM architecture. Adjust Play/Pause button to take up 50% of screen height and optimize station departure display to show only next departure per line. Add proper delay indicators and improve loading states. Fix location permission handling and station distance calculation.
My main changes:Demo.mp4TTS works perfectly fine, but sound doesn’t seem to work in the markup, I guess. |
|
Should I simplify the files a bit more? It’s quite a lot of code. |
|
Would you mind testing if the app works on your device too? I’ve only tried it on my old Android phone so far. |
Next time, split these changes into multiple tickets and pull requests (PRs). I saw something related to translations in the changes that has nothing to do with the initial issue #22. It's really hard to review these massive PRs, that's why it takes so long.
If it works on your device, it should also work on newer ones. If you're able to test it on an emulator with a more recent version of Android, feel free to do so. If the app works on there too, I'll merge this and we forget that this big PR ever existed ;) |
|
Okay I will test it and will look that everything works fine. :) |
|
Can we merge this as an unstable version? |
| val plannedTime = extractLocalTimeFromDateString(it.scheduleTime) | ||
| val actualTime = extractLocalTimeFromDateString(realTime) | ||
| (plannedTime.hour - actualTime.hour) * 60 + (plannedTime.minute - actualTime.minute) | ||
| (actualTime.hour - plannedTime.hour) * 60 + (actualTime.minute - plannedTime.minute) |
There was a problem hiding this comment.
Maybe we should switch to kotlinx.datetime to get access to the Duration class which allows something like (actualTime - plannedTime).inWholeMinutes.
If you want to work on something, you should check the library out: https://github.com/Kotlin/kotlinx-datetime
This would be a required step to make the app work on iOS, since the LocalDate class we are currently using is a Java-Class which we are unable to utilize when targeting non-Android platforms. Feel free to create a master ticket for Kotlin Multiplatform for iOS (see https://kotlinlang.org/docs/multiplatform/kmp-overview.html) and a child ticket for the replacement of java DateTime API with the Kotlin alternative.
| emptyList<Station>() | ||
| } else { | ||
| Log.d("DVBSource", "JSON content length: ${content.length}") | ||
| Log.d("DVBSource", "First 100 characters: ${content.take(100)}") |
There was a problem hiding this comment.
We don't need to log this
| Station(it.properties.number, it.properties.name, it.geometry.coordinates[0], it.geometry.coordinates[1], emptyList()) | ||
| Log.d("DVBSource", "Starting to load stations from raw resource") | ||
| return try { | ||
| val inputStream = context.resources.openRawResource(R.raw.stops) |
There was a problem hiding this comment.
This should be deferred to the VVO-Api in the long term, so we can also get rid of the batch script which generates the json. Feel free to create a ticket.
There was a problem hiding this comment.
Do we need a dedicated activity for this? We have a loading indicator on the main activity
There was a problem hiding this comment.
Either complete or remove this file


I will review the changes now