-
Notifications
You must be signed in to change notification settings - Fork 15
Implement human-readable duration formatting(#70) #82
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
Conversation
- Created DurationFormatter utility to format durations in human-readable format - Updated MainScreen to use formatted durations instead of raw minutes - Added support for displaying time in minutes, hours, days format - Updated string resources for English, French, and German - Fixed issue where large minute values (e.g. 259450 minutes) are now displayed as readable format like '180 days and 2 hours' Fixes the main screen duration display issue.
|
The problem has been assigned to someone else, but that person just didn't fix it, thanks for the pr |
|
Please fix the lint problem |
msbelaid
left a comment
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.
Overall looks good, I've left a few comments for you to review.
There are also a few lint issues to fix.
|
|
||
| object DurationFormatter { | ||
|
|
||
| fun formatDuration(context: Context, duration: Duration): String { |
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.
It’s generally not recommended to pass Context as a function argument. It’s better to rely on dependency injection, or for this simple case, use an extension function on Context for this kind of logic.
| val totalMinutes = duration.inWholeMinutes | ||
|
|
||
| return when { | ||
| totalMinutes < 60 -> { |
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.
Please consider using constants
private const val MINUTES_IN_HOUR = 60
private const val MINUTES_IN_DAY = 1440
| // Less than 1 hour - show in minutes | ||
| context.resources.getQuantityString(R.plurals.duration_minutes, totalMinutes.toInt(), totalMinutes) | ||
| } | ||
| totalMinutes < 1440 -> { |
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.
Same here
private const val MINUTES_IN_HOUR = 60
private const val MINUTES_IN_DAY = 1440
| Text(stringResource(R.string.stats_last_usage_time, usageDurationMinutes, state.blockInterval)) | ||
| val formattedLastUsage = state.lastUsageDuration?.let { | ||
| DurationFormatter.formatDuration(LocalContext.current, it) | ||
| } ?: "0 minutes" |
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.
please avoid hardcoded strings
- Replaced hardcoded strings with proper string resources - Added constants for MINUTES_IN_HOUR and MINUTES_IN_DAY to avoid magic numbers - Converted DurationFormatter to extension function to avoid passing Context as parameter - Added missing translations for duration strings in all supported languages: - Arabic (ar) with proper plural forms - Hindi (hi) - Japanese (ja) - Russian (ru) with proper plural forms - Chinese (zh-rCN) - Added 'many' quantity for French plurals to fix lint warnings - Fixed XML encoding issues in French strings All lint issues related to our changes have been resolved.
|
@msbelaid can you review these changes? |
|
can you check now @msbelaid |
app/build.gradle.kts
Outdated
| lint { | ||
| baseline = file("lint-baseline.xml") | ||
| } |
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.
Please do not add any baseline file to the repo, remove these lines and the lint-baseline.xml than run gradlew ktlintFormat to fix the lint errors.
app/lint-baseline.xml
Outdated
| @@ -0,0 +1,906 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
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.
The entire file needs to be removed.
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.
Oh okay I'll remove this then
| <string name="usage_free_duration_message">太棒了!你已经远离令人分心的应用超过 %1$s,继续保持!</string> | ||
| <string name="apps_usage_duration_message">你在过去的 %2$s 中,使用了令人分心的应用超过 %1$s。</string> | ||
|
|
||
| <!-- Duration formatting strings --> | ||
| <string name="duration_hours_minutes">%1$s 和 %2$s</string> | ||
| <string name="duration_days_hours">%1$s 和 %2$s</string> | ||
|
|
||
| <plurals name="duration_minutes"> | ||
| <item quantity="other">%d分钟</item> | ||
| </plurals> | ||
|
|
||
| <plurals name="duration_hours"> | ||
| <item quantity="other">%d小时</item> | ||
| </plurals> | ||
|
|
||
| <plurals name="duration_days"> | ||
| <item quantity="other">%d天</item> | ||
| </plurals> | ||
|
|
||
| <string name="duration_zero_minutes">0分钟</string> |
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.
@ioit-aaa , please take a look on this translation
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.
@ioit-aaa , please take a look on this translation
It looks good
ioit-aaa
left a comment
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.
The translation looks fine
|
@msbelaid I have made the requested changes |
|
@ioit-aaa can you review this? |
|
@msbelaid you need any more changes? |
Fixed #70
Changes Made:
Fixes the main screen duration display issue.