After 24h fasting add small 'days + hours' underneath timer - #59
Conversation
|
Next request is adding similar info to the other timers: Fat Burn, Ketosis, Autophagy. |
Not sure what you mean by this. Could you please post this in the Issues area with a bit more info, as this is mainly for discussing the code changes specific to the PR. |
Sorry, I mean similar days/hours/minutes timers for the "Fat Burn", "Ketosis", and "Autophagy" timers. |
|
I think this static code tool is a bit too aggressive... but okay. I'll provide a few more fixes. |
|
Ok Static Code tool seems happy. I would prefer to have this merged over #58 Can we get this done? These have been dangling for like 2 months just based on simple indentation fixes etc. |
|
Wouldn't it make sense to take advantage of the touch event on the hh:mm:ss display - which currently does nothing - and use it to toggle the view between hours vs days? The small text alternative label below would still be useful in this case as it can be used to show the alternative view (hours vs days). E.g. the original screenshot would be the initial state, and tapping the view would swap between the two: 57:00:592 days, 9 hours after tapping: 2 days, 9 hours57:00:59 Wdyt? |
|
I need to check how this looks on a wide variety of screen sizes, you wouldn't believe the problems I've had with the fasting screen on very tiny or very large screens. But I'll get to it this week and hopefully get this merged! |
I would and do believe 😅; layouts can be really tricky to render properly in all viewports; been through it; maybe autosizing can help? e.g. app:autoSizeTextType="uniform"
app:autoSizeMinTextSize="12sp"
app:autoSizeMaxTextSize="100sp"
app:autoSizeStepGranularity="2spor programmatically textView.setAutoSizeTextTypeWithDefaults(TextView.AUTO_SIZE_TEXT_TYPE_UNIFORM)
// with custom bounds
textView.setAutoSizeTextTypeUniformWithConfiguration(
12, 100, 2, TypedValue.COMPLEX_UNIT_SP
)This works similar to used something similar in a project, not perfect and sometimes causes unexpected results but maybe can work if properly configured? although, I guess, autosizing is not really the answer to what should probably be composable layouts https://developer.android.com/develop/ui/compose/layouts/basics or custom layouts for each view port https://developer.android.com/develop/ui/views/layout/responsive-adaptive-design-with-views with resource qualifiers (e.g. |
|
I don't see how having it in small text underneath the already massive large font is going to be an issue on displays. Personally having it always active (rather than on touch which is easy to not know it exists) is more of a no-brainer to me. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 84 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements the 'days + hours' display requirement, but it includes a significant, unannounced visual redesign of the TimeLine component and extensive refactoring of the fasting screen. While the project is overall 'Up to Standards' according to Codacy, several implementation choices introduce technical debt and performance risks.
Key issues that should be addressed before merging:
- Localization (L10n): User-facing strings and pluralization logic are hardcoded in the
FastingViewModel, which violates Android best practices and prevents internationalization. - Performance: The new animated
TimeLineallocatesPathobjects inside theCanvasdraw loop, which will cause excessive garbage collection and potential frame drops on every frame of the blinking animation. - PR Description: The description fails to mention the complete overhaul of the timeline UI (transition to rhombuses and blinking states).
About this PR
- The PR description focuses only on the timer label, but the diff reveals a major visual redesign of the 'TimeLine' component (rhombus shapes, blinking animations) and extensive screen refactoring. Please update the PR description to accurately reflect these changes for future auditability.
Test suggestions
- Verify 'daysAndHoursText' is null for durations under 24 hours.
- Verify '1 day' formatting for exactly 24 hours.
- Verify '1 day, 1 hour' formatting for 25 hours.
- Verify pluralization for multiple days and hours (e.g., 50 hours results in '2 days, 2 hours').
- Verify the blinking animation (infinite transition) is active on the current phase in TimeLine.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify 'daysAndHoursText' is null for durations under 24 hours.
2. Verify '1 day' formatting for exactly 24 hours.
3. Verify '1 day, 1 hour' formatting for 25 hours.
4. Verify pluralization for multiple days and hours (e.g., 50 hours results in '2 days, 2 hours').
5. Verify the blinking animation (infinite transition) is active on the current phase in TimeLine.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| val daysAndHoursText = if (hours >= 24) { | ||
| val days = hours / 24 | ||
| val remainingHours = hours % 24 | ||
| if (remainingHours == 0L) { | ||
| if (days == 1L) "1 day" else "$days days" | ||
| } else { | ||
| val dayText = if (days == 1L) "1 day" else "$days days" | ||
| val hourText = if (remainingHours == 1L) "1 hour" else "$remainingHours hours" | ||
| "$dayText, $hourText" | ||
| } | ||
| } else null |
There was a problem hiding this comment.
🔴 HIGH RISK
Avoid hardcoding user-facing strings and manual pluralization logic in the ViewModel. This prevents internationalization and fails to support languages with complex plural rules. Refactor the daysAndHoursText logic to use Android string resources with plurals (quantity strings). Either pass the raw values to the UI or use a resource provider.
Try running this prompt in your IDE agent:
Refactor the
daysAndHoursTextlogic to use Android string resources with plurals instead of hardcoded English strings in theFastingViewModel.
| val rhombusPath = Path().apply { | ||
| moveTo(startX + slantOffsetPx, startY - barSizePx / 2) | ||
| lineTo(startX + phaseWidth + slantOffsetPx, startY - barSizePx / 2) | ||
| lineTo(startX + phaseWidth, startY + barSizePx / 2) | ||
| lineTo(startX, startY + barSizePx / 2) | ||
| close() | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Allocating Path objects inside the Canvas draw block during an animation causes excessive garbage collection and potential frame drops. Consider using Modifier.drawWithCache to move the calculation of rhombusPath and layout constants into an onDrawWithCache block, ensuring they are only recomputed when the component size changes.
| val phaseWidth = (size.width - (2 * paddingPx) - (Stages.phases.size - 1) * spacingPx) / Stages.phases.size | ||
| val totalWidth = (Stages.phases.size * phaseWidth) + ((Stages.phases.size - 1) * spacingPx) + slantOffsetPx | ||
| val startOffset = (size.width - totalWidth) / 2f |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The layout logic for calculating phase widths and offsets is duplicated between the tap detection block and the drawing block. Extract these calculations into a shared helper function or scope to ensure tap targets remain synchronized with the visuals.
Adds a small indication of how many days + hours you have been fasting after 24h.