Skip to content

Conversation

@ohassine
Copy link
Member

@ohassine ohassine commented Dec 29, 2025

BugWPB-22171 [Android] Files upload time format doesn't follow the device settings


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

For cells files, time should follow device format. If system is using 12h or 24h format, the app should follow this.

Needs releases with:

  • GitHub link to other pull request

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.

Notes (Optional)

Specify here any other facts that you think are important for this issue.

Attachments (Optional)

Attachments like images, videos, etc. (drag and drop in the text box)


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@yamilmedina
Copy link
Contributor

@claude review

@github-actions
Copy link
Contributor

github-actions bot commented Dec 29, 2025

Pull Request Review - Time Format Following Device Settings

Summary

This PR fixes an issue where the Cells feature was not respecting the device's 12h/24h time format settings. The changes introduce new methods to use java.text.DateFormat for time formatting, which automatically adapts to the system's locale and time format preferences.


Code Quality & Best Practices

Strengths

  1. Proper Approach: Using java.text.DateFormat.getTimeInstance() is the correct way to respect system time format preferences (12h/24h)
  2. Consistent Timezone Handling: All new methods properly set timeZone = java.util.TimeZone.getDefault()
  3. Backward Compatibility: Existing API preserved - cellFileDateTime() still works, just with improved implementation
  4. Separation of Concerns: New cellTimeFormat() and cellDateFormat() methods allow for flexible reuse

Issues & Concerns

1. Inconsistent Removal of cellFileDateTimeFormat (core/ui-common/src/main/kotlin/com/wire/android/util/DateAndTimeParsers.kt:105-106)

The old cellFileDateTimeFormat formatter was removed, but there is an inconsistency in the codebase.

Impact: Medium - The code works but cleanup could be more complete.

Recommendation: Verify all references are consistent.


2. Hardcoded Date Pattern (core/ui-common/src/main/kotlin/com/wire/android/util/DateAndTimeParsers.kt:171-172)

While the time format now respects device settings, the date format is still hardcoded using SimpleDateFormat with pattern MMM dd, yyyy.

Impact: High - This does not fully respect locale preferences

  • US format: Dec 29, 2025
  • Some locales prefer: 29 Dec 2025 or 2025-12-29

Recommendation: Consider using DateFormat.getDateInstance() for dates as well to fully respect locale preferences.


Performance Considerations

No major concerns

  • The new approach using DateFormat and SimpleDateFormat is standard and performant
  • Instance creation on each call is normal for formatters
  • No loops or heavy computations introduced

Security Concerns

No security issues identified

  • Date/time formatting is safe
  • No user input parsing or external data handling
  • Proper timezone handling prevents timezone-related vulnerabilities

Test Coverage

Missing Tests

The existing test file (DateAndTimeParsersTest.kt) does not include tests for the new functions:

  • No tests for cellFileTime()
  • No tests for cellDateFormat()
  • No tests for the refactored cellFileDateTime()

Recommendation: Add tests to verify:

  1. 12h format (e.g., 11:30 PM) in US locale
  2. 24h format (e.g., 23:30) in locales that use it
  3. Date formatting with and without year
  4. Combined date-time formatting

Summary of Recommendations

Must Fix

  • Add unit tests for new date/time formatting functions covering different locales and formats

Should Consider

  • Consider using DateFormat.getDateInstance() for dates to fully respect locale preferences (not just time)
  • Verify all other time display locations in Cells feature respect device format

Nice to Have

  • Remove unnecessary empty line in cellFileDateTime() function
  • Add documentation comments explaining that these functions respect device locale/time format

Verdict

Overall Assessment: Approve with Recommendations

The core fix is solid and addresses the issue correctly. The use of DateFormat.getTimeInstance() is the right approach for respecting device time format settings. However, the PR would benefit from:

  1. Adding comprehensive tests
  2. Considering full locale support for date formatting (not just time)

The code is production-ready but would be more robust with the recommended improvements.


Review completed by Claude Code

…evice-settings' into time-format-doesn't-follow-the-device-settings
@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.72%. Comparing base (7541389) to head (564b757).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
...ure/cells/ui/versioning/VersionHistoryViewModel.kt 33.33% 1 Missing and 1 partial ⚠️
...kotlin/com/wire/android/util/DateAndTimeParsers.kt 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4502      +/-   ##
===========================================
- Coverage    48.73%   48.72%   -0.01%     
===========================================
  Files          563      563              
  Lines        19493    19516      +23     
  Branches      3252     3253       +1     
===========================================
+ Hits          9499     9510      +11     
- Misses        8997     9008      +11     
- Partials       997      998       +1     
Files with missing lines Coverage Δ
...kotlin/com/wire/android/util/DateAndTimeParsers.kt 81.25% <93.75%> (+2.46%) ⬆️
...ure/cells/ui/versioning/VersionHistoryViewModel.kt 83.05% <33.33%> (-0.99%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7541389...564b757. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ohassine ohassine changed the title fix(cells): Time format doesn't follow the device settings fix(cells): Time format doesn't follow the device settings (WPB-22171) Dec 30, 2025
@ohassine ohassine requested a review from yamilmedina December 30, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants