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

Use fbclient-interfaces to decode time-fields with time zone. #362

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

andy-123
Copy link
Contributor

@andy-123 andy-123 commented Dec 29, 2023

This PR ueses fbclient lib to decode ISC_TIME_TZ / ISC_TIME_TIMESTAMP-structs.

AFAICS isql does it the same way.

However, the following commands produces different result (flamerobin <-> isql).

set time zone 'America/Sao_Paulo';
select localtime, current_time from rdb$database;

        LOCALTIME                                   CURRENT_TIME 
        ============= ============================================== 
FR:     07:24:35.0000 08:24:35.0000 America/Sao_Paulo       
ISQL:   07:24:35.0000 07:24:35.0000 America/Sao_Paulo    

TODO

  • (1) use time-zone names from fbclient-functions to detect GMT-fallback (= failed to load ICU)
  • (2) load fb_get_master_interface dynamic to work with older fbclient-libs (< 3.0)
  • [ ] (3) analyse "select cast('2018-01-01 16:01:19 America/Sao_Paulo' as timestamp with time zone)
    from rdb$database;"
    It returns ... 15:01 ... (fbclient-lib bug?)
    (my icu was outdated; 2019)
  • [ ] (4) select current_time(stamp) returns the wrong time too. I've modified fbclient-lib to get the expected value. I'm unsure if it is a fbclient-lib bug? (common/TimeZoneUtil.cpp:TimeZoneUtil::decodeTimeStamp - line ~ 770 - i've commented "+ icuLib.ucalGet(icuCalendar, UCAL_DST_OFFSET, &icuErrorCode)" out.
  • [ ] (4a) regardless bug or not - a fix for the the "may-be-buggy"-fbclient-lib is needed.
  • Test: Windows - flamerobin + older fbclient.dll (3.0/2.5) (runs?) - yes :)
  • Test: Linux - flamerobin + older fbclient.dll (3.0/2.5) (runs?) - yes :)
  • Test: Windows - is time-zone + time correct ? (does this PR really fix the bug?)
    Yes: Short story - tzdata directory was missing. Should be on the same directory as fbclient.dll.
  • Test: Linux - is time-zone + time correct ? (does this PR really fix the bug?)
    Yes: Short story - self-build or old libfbclient with outdated time-zone database.

To point (4):
* isql calls setlocale(LC_CTYPE, ""); (~ line 771) .... so it's startet with no locale (GMT-TimeZone). "+ icuLib.ucalGet(icuCalendar, UCAL_DST_OFFSET, &icuErrorCode)" is 0 and has no affect.

 * Remove some IBPP_LATE_BIND directives (not needed anymore)
 * define m_get_master_interface-prototype
… to load ICU (GMT-fallback).

* In this case, fbclient (IUtil.decodeTime(stamp)Tz) returns "GMT*" as the time zone name.
* "GMT*" is now also displayed by flamerobin in this case.
@arvanus
Copy link
Collaborator

arvanus commented Feb 1, 2024

Hi, is there some status about the 1hour+ divergence?
Thanks!

@andy-123
Copy link
Contributor Author

andy-123 commented Feb 6, 2024

No, unfortunately there was no helpful answer to either your or my support request.
My two options that I still see are:
Either to find a open-source-tool that uses the Firebird API “correctly”, look into code of this tool, and then to be able to apply this knowledge to Flamerobin.
The other option would be to use icu directly.
I still have a little time this week, but I'll start pursuing these two options.

@andy-123 andy-123 marked this pull request as ready for review February 14, 2024 20:36
@andy-123
Copy link
Contributor Author

My implementation was already okay. The problems were caused by an incomplete / unclean fbclient installation.
Long story: Under Linux the tzdata directory was (probably) not found. I was able to solve the problem by setting the environment variable LD_LIBRARY_PATH.
Under Windows it was enough to copy the tzdata directory into the Flamerobin root directory. I had already copied the fbclient.dll there.
@arvanus Maybe a similar problem existed in your test.

@arvanus
Copy link
Collaborator

arvanus commented Feb 14, 2024

I'll take a look in the next days, out of time this days
Thanks for your great work!

@andy-123
Copy link
Contributor Author

andy-123 commented Apr 5, 2024

Hi, any news about reviewing this PR?

@arvanus arvanus merged commit 7379b82 into mariuz:master Apr 7, 2024
4 checks passed
@arvanus
Copy link
Collaborator

arvanus commented Oct 14, 2024

Hello @andy-123
Can you take a look, because now the user can't change time fields when you don't have timezone in the field

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.

2 participants