Implement a Relative DateTime Formatter

[akshay]

Currently, there is a BDurationFormat which takes in a time interval and gives a formatted string such as "1 hour:2 minutes:3 seconds". A RelativeDateTime formatter is required which formats relative dates.

I have done some initial work on the same taking a lot of help from existing classes in locale kit(BDurationFormat/BTimeUnitFormat). Currently it's a basic implementation and I would like to have views before working further on this.

It's a simple cut-off logic based upon icu::RelativeDateTimeFormatter using the numeric-only style.

Input: Takes in a string buffer to fill with the formatted string, and the time (no. of seconds since epoch) to format, relative to the current time.

Returns: status code.

Currently it formats the relative date as follows:

1. time same as current calendar time: " in 0 seconds"
2. within 1 minute(before or after): '20 seconds ago', 'in 20 seconds'.
3. within 1 hour: '20 minutes ago', 'in 20 minutes'.
4. within 1 day: '5 hours ago', 'in 1 hour'.
5. within 1 week: '4 days ago', 'in 5 days'.
6. within 1 month: '2 weeks ago', 'in 3 weeks'.
7. within 1 year: '1 month ago', 'in 5 months'.
8. anything after that: '1 year ago', 'in 5 years'.

This result is with an offset of 1. For an offset of 2 we can have it as within 2 days: '36 hours ago', 'in 14 hours'. Also for time same as current time, a text term such as 'now' can be shown.

Instead of whole numbers values for years, months etc, icu::RelativeDateTimeFormatter also supports fractional values (for e.g 3.2 years, 2.5 months). Numeric style (for e.g: in 1 day, 1 week ago), or text style (for e.g: tomorrow, next week, last year) or a combination of both can be used. Day of week can also be incorporated in formatting (for e.g next Tuesday, in 4 Tuesdays).

Various cut-off logic can be implemented based upon icu::RelativeDateTimeFormatter.

Also for formatting I'm currently using icu::RelativeDateTimeFormatter::formatNumeric(), which I am not very sure about using, because it is a part of ICU Draft API and may be changed in the future versions(introduced in ICU 57). A combination of UDateDirection and UDateRelativeUnit can be used in calls to icu::RelativeDateTimeFormatter::format() which is a part of stable API, to achieve the same. So maybe I'll base this upon it.

[pulkomandy]

Using "experimental" APIs from ICU is fine. There may be some changes in later versions but usually we can adjust our code when we update ICU (which we don't do that often).

It would be nice to adjust existing code to benefit from this. WebPositive download window and Tracker copy dialog both come to mind as cases where we need such a thing.

Code review:

• line 100: strange indenting style, I don't think the : should be alone on its line
• you should use new<std::nothrow> instead of new if you want to check allocation errors by getting NULL pointers. Otherwise, new will throw an std::bad_alloc exception when it runs out of memory, and NULL checks are useless (but the exception should be caught). Since catching the exception from the initialization list of a constructor is not easy, std::nothrow is probably the best solution here. (if you copied that pattern from another class, it should be fixed there as well).
• I find it unhandy that this class works with a time_t. Is it not possible to use a BDateTime instead?
• Why does it need to know about the timezone? If it's working with time_t dates, these should always be in UTC. If it's working with BDateTime, each BDateTime object should know about its own timezone (and DST setting if applicable), so that it is possible to compute a time delta between dates from different timezones.

[akshay]

• After this gets committed I'll look at existing code requiring this and incorporate it there.

• ​https://github.com/haiku/haiku/blob/master/src/kits/locale/DurationFormat.cpp#L56 ​https://github.com/haiku/haiku/blob/master/src/kits/locale/TimeUnitFormat.cpp#L70-L94

coding guidelines says - "The ':' always comes on its own line". Conflicting indendation style all over causes confusion everytime.

• such NULL checks without using std::nothrow is there at quite a few places in locale kit. I'll fix them as well.

• Ya, in case of time_t, setting of timezone is not required. Not sure why BDurationFormat requires it.

I have a doubt regarding using BDateTime. It's not timezone aware so the timezone(associated with a particular BDateTime) has to be separately stored every time we create a BDateTime. Should we make the BDateTime timezone aware by itself?

In case a BDateTime is used here(as an input to RelativeDateTimeFormat::Format()), timezone associated with it would be required to set the calendar to that, or we calculate the time_t using that info, and use that with calendar set to default timezone(like it's there currently?

[pulkomandy]

The ideal solution would be that BDateTime gets operators (+, -, etc) implemented, allowing us to perform date math easily on it (substracting a date from another returns a duration, etc). For these operators to make sense, the dates need to be aware of the timezone and DST setting (so you can substract two dates which are in different timezones or before/after DST and still get meaningful results). This means a rather large rework of BDateTime, however.

On 32-bit Haiku, time_t stops working in 2038 so I try to avoid it as much as possible. 2038 is not this far away anymore. But given that BDurationFormat is already done this way, I guess we can let that issue on the side for now and merge the patch anyway.

[akshay]

Using std::nothrow gives error as the UMemory::operator new() does not take nothrow as an argument. Internally it calls uprv_malloc() which normally returns nullptr if it's out of memory. So a NULL check would work fine here?

[akshay]

Changes:

RelativeDateFormat.cpp

• Removed #include <new>.
• line 41: changed constant name from skunitmap to kTimeUnitToICUDateField.
• line 100: ':' is not on its own line.
• line 115: Added line 'delete fCalendar'.
• line 165: Indentation changes.

RelativeDateFormat.h

• line 18: Reduced blank lines from 3 to 1

[pulkomandy]

Ah yes, I missed the fact that ICU is handling the "new" in their own way already.

So, do we keep the API for setting a timezone for now? I thought it had no effect and could be removed? (even if we don't introduce time zone support in BDateFormat yet)

Also, I think one may want to use this for events in the future as well as in the past? Did you test its behavior with negative time_t? Does it give "2 hours ago" and the like as expected?

[akshay]

• Ya, it works well for past events.... '2 hours ago' if English is selected and 'il y a 2 heures' if Francais is selected as language.

• We remove it. It's of no use now. I was trying it out with DateTime so forgot to remove it. I am making another patch with the SetTimezone function removed.

I have another query:

1. How do I build webpositive and how much time does it take, or right way to test changes if I make any here ​https://github.com/haiku/haiku/blob/master/src/apps/webpositive/DownloadProgressView.cpp#L791-L846

I am running a gcc2 hybrid. I opened terminal, moved to the root of my haiku clone. Did a setarch x86. Then ./configure. then moved to src/apps. and then jam. It's taking a lot of time to build.

[pulkomandy]

You need to tell jam what you want to build. For x86_gcc2: jam -q "<x86>WebPositive". For other setups: jam -q WebPositive. There is no need to go inside the src/apps directory to run Jam.

[akshay]

Thanks.

Added a patch with SetTimeZone() removed.

[akshay]

Added #include <stdlib.h> required for abs.

[pulkomandy]

Applied in hrev51374.