Skip to content

Conversation

@ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ self-assigned this Nov 6, 2025
@ArnyminerZ ArnyminerZ requested a review from a team as a code owner November 6, 2025 12:25
@ArnyminerZ ArnyminerZ added the bug Something isn't working label Nov 6, 2025
@ArnyminerZ ArnyminerZ linked an issue Nov 6, 2025 that may be closed by this pull request
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #135 (comment)

The "parse and catch exception" approach doesn't seem to be a proper solution, at least not in the handle().

I think we should define a separate parseStartDate() method that

  • has well-defined inputs and outputs,
  • supports the required input formats,
  • is tested separately.

Within that method, we should use other methods to determine the input format. If that's really not possible, we can still use the exception method.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ marked this pull request as draft November 10, 2025 13:51
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 1, 2025 08:09
@ArnyminerZ
Copy link
Member Author

Should be ready. I've added tests for all formats from AOSP Contacts which should cover the original issue (bitfireAT/davx5-ose#1797 - 2013-11-06T00:00:00.000Z)

@ArnyminerZ ArnyminerZ marked this pull request as ready for review December 1, 2025 08:10
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 1, 2025 10:11
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a few small things

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 December 2, 2025 13:59
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Last minor things, then I'd say its done :)

@rfc2822
Copy link
Member

rfc2822 commented Dec 4, 2025

When finished please also request a review from @sunkup

@rfc2822
Copy link
Member

rfc2822 commented Dec 5, 2025

I have updated it & will merge after a copilot review so that I can release 4.5.7-rc.1.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support for parsing date-time values with timezone information (ZonedDateTime/OffsetDateTime) in the EventHandler class, addressing the need to handle timestamps with time components from Android's AOSP Contacts App. Previously, only date-only formats were supported.

  • Refactored date parsing logic to handle both date-time formats (with timezone) and date-only formats
  • Added support for parsing partial dates with time components
  • Implemented comprehensive test coverage for all supported date/time formats

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
lib/src/main/kotlin/at/bitfire/vcard4android/contactrow/EventHandler.kt Refactored parsing logic into separate methods (parseFullDate and parsePartialDate) to handle OffsetDateTime, LocalDate, and PartialDate formats; added support for date-time formats with timezone offsets
lib/src/test/kotlin/at/bitfire/vcard4android/contactrow/EventHandlerTest.kt Added comprehensive unit tests for the new parsing methods covering all supported date/time formats including ISO_DATE_AND_TIME_FORMAT, FULL_DATE_FORMAT, and partial date formats with and without time components

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have adapted the last things a bit. Hope everything still works.

@rfc2822 rfc2822 merged commit 5fb54ec into main Dec 5, 2025
7 checks passed
@rfc2822 rfc2822 deleted the 135-eventhandler-doesnt-handle-start-date-with-time branch December 5, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EventHandler doesn't handle start date with time

2 participants