RT schedule UI improvements...#154
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on refining the user experience for real-time schedule displays by enhancing how real-time data is processed and presented. It includes updates to UI text formatting for schedule statuses, a significant refactoring of time change listeners for better maintainability, and improvements to the underlying data merging logic for real-time updates. Additionally, it incorporates minor fixes for clipboard functionality and logging clarity. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Qodana for JVMIt seems all right 👌 No new problems were found according to the checks applied ☁️ View the detailed Qodana report Contact Qodana teamContact us at qodana-support@jetbrains.com
|
There was a problem hiding this comment.
Code Review
The pull request refactors how UITimeUtils.TimeChangedReceiver listeners are handled in NewsListAdapter, NewsDetailsFragment, and ScheduleFragment. Instead of using direct lambdas, explicit UITimeUtils.TimeChangedReceiver.TimeChangedListener objects are now created and passed to the receiver. This change ensures that the WeakReference mechanism within the TimeChangedReceiver correctly maintains references to the listeners, preventing premature garbage collection and ensuring time change events are reliably processed. Additionally, the ScheduleViewModel was updated to correctly apply real-time schedule updates by removing the original timestamp and adding the real-time one, then re-sorting the list. Minor changes include adding onInitializationComplete() > to an ad log message, reordering conditions in POIManager.getStatus for readability, adjusting UISchedule to wrap early/late text in parentheses and simplify heading appending, updating the timestamps setter in ScheduleAdapter for clarity, changing for loop to forEach in ScheduleAdapter, and updating string resources for short late/early indicators to include + and - signs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces several UI improvements related to schedule display and refactors the handling of time change listeners across various fragments and adapters. A notable change is the revised logic in ScheduleViewModel.kt for applying real-time timestamp updates, which now ensures better accuracy and consistency by replacing old timestamps with new real-time data and re-sorting the list. One comment related to displaying early/late status text when a headsign is unavailable has been updated to reference an existing rule. Overall, these changes enhance the user experience and improve code maintainability.
I am having trouble creating individual review comments. Click here to see my feedback.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleViewModel.kt (231-232)
The change in real-time timestamp update logic, where old timestamps are removed and new real-time timestamps are added, followed by sorting, is a significant functional improvement. This ensures that the schedule always reflects the most accurate and up-to-date information, which is critical for a real-time transit application. This change enhances the reliability and correctness of the displayed schedule.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleViewModel.kt (249)
Explicitly sorting the scheduleTimestamps after applying real-time updates ensures that the displayed schedule is always in the correct chronological order, which is essential for user experience and data consistency.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleViewModel.kt (239-248)
The new approach of converting scheduleTimestamps to a mutable list, then iterating through rtTimestamps to remove existing entries and add new real-time ones, followed by sorting, is a more robust way to handle real-time updates. This ensures data integrity and proper ordering, which is crucial for accurate schedule display.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (247)
Replacing the for loop with timestamps.forEach { timestamp -> is a more idiomatic and concise way to iterate over collections in Kotlin, improving code style and readability.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (635)
Using EMPTY instead of Constants.EMPTY is consistent with the specific imports added earlier in the file, improving code consistency and reducing verbosity.
app-android/src/main/java/org/mtransit/android/ui/news/NewsListAdapter.kt (101-103)
Removing the onTimeChanged() private function is a clean-up that aligns with the refactoring of the timeChangedReceiver to use a named listener object. This reduces redundant code and improves clarity.
app-android/src/main/java/org/mtransit/android/ui/news/details/NewsDetailsFragment.kt (115-119)
Extracting the lambda into a named timeChangedListener object improves code readability and explicitly manages the WeakReference lifecycle, which is crucial for preventing memory leaks in Android development. This is a good practice for maintainability.
app-android/src/main/java/org/mtransit/android/ui/news/details/NewsDetailsFragment.kt (478-480)
Removing the onTimeChanged() private function is a clean-up that aligns with the refactoring of the timeChangedReceiver to use a named listener object. This reduces redundant code and improves clarity.
app-android/src/main/java/org/mtransit/android/ui/pref/main/MainPreferencesFragment.kt (223-227)
Removing the redundant token variable and directly using the it parameter in the let block makes the code more concise and readable without altering its functionality.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (34-35)
Using specific imports for EMPTY and SPACE from org.mtransit.commons.Constants improves code clarity by indicating exactly which constants are being used and reduces the overall scope of the import.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (48-49)
Adjusting the class declaration formatting improves code readability and aligns with common Kotlin style guidelines for multi-line declarations.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (125-129)
Refactoring the setter for timestamps to use newValue and curly braces for the if statement improves code readability and consistency with modern Kotlin style.
app-android/src/main/java/org/mtransit/android/ad/GlobalAdManager.kt (125)
Adding onInitializationComplete() > to the log message provides more context about where the log is originating from, which can be helpful for debugging and understanding the application flow.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (603-604)
Reordering the P1 and P2 constants, while minor, can improve logical grouping if there's an implicit order or relationship between them. This contributes to better code organization.
app-android/src/main/java/org/mtransit/android/ui/news/NewsListAdapter.kt (75-79)
Extracting the lambda into a named timeChangedListener object improves code readability and explicitly manages the WeakReference lifecycle, which is crucial for preventing memory leaks in Android development. This is a good practice for maintainability.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleAdapter.kt (643)
Changing the heading formatting from (P1) + it + (P2) to SPACE + it simplifies the output by removing parentheses and using a single space. This can improve the visual presentation of the heading.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleFragment.kt (117-124)
Extracting the lambda into a named timeChangedListener object improves code readability and explicitly manages the WeakReference lifecycle, which is crucial for preventing memory leaks in Android development. This is a good practice for maintainability.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleFragment.kt (124)
The creation of timeChangedReceiver now correctly uses the timeChangedListener object, ensuring proper WeakReference management and consistent behavior with the refactored listener.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleFragment.kt (283)
Calling timeChangedListener.onTimeChanged() directly instead of the removed onTimeChanged() function ensures consistency with the refactored listener implementation and maintains the intended behavior of forcing an update.
app-android/src/main/java/org/mtransit/android/ui/schedule/ScheduleViewModel.kt (20-23)
Removing unused imports for arrival, departure, and updateForRealTime cleans up the code and indicates that these functions are no longer directly utilized in this file, improving code maintainability.
app-android/src/main/java/org/mtransit/android/data/UISchedule.java (520)
Wrapping the earlyOrLateText with parentheses () when headSignSSB is null provides better visual grouping and clarity for the user, making it easier to distinguish this information. This aligns with the practice of using late/early status text as the primary headsign when a timestamp heading is not available.
References
- If a timestamp heading is not available, the late/early status text can be used as the primary headsign.
app-android/src/main/java/org/mtransit/android/data/UISchedule.java (517)
Changing the short parameter to true in getAbsoluteDepartureDiffString ensures a consistent short format for departure differences. This improves the uniformity of the displayed information.
app-android/src/main/java/org/mtransit/android/data/POIManager.java (237-238)
Reordering the conditions to check !this.status.isUseful() before this.lastFindStatusTimestampMs < 0L can slightly improve efficiency if isUseful() is a quick check and lastFindStatusTimestampMs is frequently positive. This prioritizes a potentially faster check, leading to minor performance gains in some scenarios.
app-android/src/main/res/values-fr/strings_schedule.xml (98)
Adding a + sign to the short version of "late" (R:+%dm) improves clarity and consistency in the French translation, making it easier for users to understand the direction of the time difference.
app-android/src/main/res/values-fr/strings_schedule.xml (100)
Adding a - sign to the short version of "early" (A:-%dm) improves clarity and consistency in the French translation, making it easier for users to understand the direction of the time difference.
app-android/src/main/res/values/strings_schedule.xml (92)
Adding a + sign to the short version of "late" (R:+%dm) improves clarity and consistency in the English translation, making it easier for users to understand the direction of the time difference.
app-android/src/main/res/values/strings_schedule.xml (94)
Adding a - sign to the short version of "early" (A:-%dm) improves clarity and consistency in the English translation, making it easier for users to understand the direction of the time difference.
Uh oh!
There was an error while loading. Please reload this page.