Skip to content
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

Fix #1460 - conversion of win32 variant dates within DST offset windows #1461

Closed
wants to merge 1 commit into from

Conversation

eranl
Copy link

@eranl eranl commented Sep 1, 2022

The main goal here is to fix #1460, which I found can be done in a straightforward way by using:

  • java.util.Calendar for converting between UTC and local time, and
  • win32 APIs for converting between local time and variant date.

While testing the fix, I realized that dates with non-zero millisecond values were not converted correctly by the win32 APIs, so I added some correction logic for preserving millisecond values. BTW, I found that the existing implementation didn't handle some millisecond values correctly either.

Overall, this is relatively simple logic, avoiding the reconstruction of complex calculations provided by the above APIs.

Added OleAuto.VariantTimeToSystemTime and a test for it.
Added tests for DST offset window that fails with existing logic, and tests for millisecond dates, some of which fail with existing logic.

Note: OaIdl.DATE_OFFSET is not used anymore, but I left it in since it's declared public.

@eranl eranl changed the title Fix #1460 - conversion of win32 variant dates within DST overlap windows Fix #1460 - conversion of win32 variant dates within DST date-shift windows Sep 1, 2022
@eranl eranl changed the title Fix #1460 - conversion of win32 variant dates within DST date-shift windows Fix #1460 - conversion of win32 variant dates within DST offset windows Sep 1, 2022
@eranl eranl marked this pull request as ready for review September 1, 2022 22:21
@matthiasblaesing
Copy link
Member

Change looks good to me. I would have preferred a pure-java way, but using the native function looks reasonable. Appveyor is also happy. Please:

  • squash the two commits into one
  • make sure valid author information is present

For the latter you can check the info using the call git log -n 1 or any git client, that is not github, which tries to be to clever. The author information should hold your real name and a valid e-mail.

https://www.git-tower.com/learn/git/faq/change-author-name-email
https://stackoverflow.com/questions/16217801/git-rebase-change-author
https://stackoverflow.com/questions/750172/how-do-i-change-the-author-and-committer-name-email-for-multiple-commits

- java.util.Calendar for converting between UTC and local time, and
- win32 APIs for converting between local time and variant date.
- some correction logic around the latter for maintaining millisecond values

Benefits:
1. fixes conversion of dates within DST offset windows
2. fixes conversion of dates with millisecond values
3. relatively simple logic, avoiding the reconstruction of complex calculations implemented in the above APIs
@eranl
Copy link
Author

eranl commented Sep 6, 2022

Thanks for your feedback.

* squash the two commits into one

Done

* make sure valid author information is present

For the latter you can check the info using the call git log -n 1 or any git client, that is not github, which tries to be to clever. The author information should hold your real name and a valid e-mail.

https://www.git-tower.com/learn/git/faq/change-author-name-email
https://stackoverflow.com/questions/16217801/git-rebase-change-author
https://stackoverflow.com/questions/750172/how-do-i-change-the-author-and-committer-name-email-for-multiple-commits

I updated my real name. For email, I've followed these guidelines for private addresses.

@matthiasblaesing
Copy link
Member

I updated my real name. For email, I've followed these guidelines for private addresses.

Please use a real e-mail adress for author information.

@matthiasblaesing
Copy link
Member

@eranl any update on this?

@eranl
Copy link
Author

eranl commented Sep 24, 2022

I would like to avoid exposing my email address publicly. GitHub supports a way to identify authors and communicate with them while keeping their email addresses private, so I'd like to continue using that.

@matthiasblaesing
Copy link
Member

Regrettably then someone else needs to redo the work. I will not merge without full author information.

@eranl
Copy link
Author

eranl commented Sep 24, 2022

Ok, I give up. Just updated my email address.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OaIdl.DATE(Date javaDate) sets wrong date in some cases
2 participants