Opened 2 years ago

Closed 18 months ago

#13606 closed bug (fixed)

Implement functions to get localized long/short dayofweek name and short month name in BDateFormat.

Reported by: akshay Owned by: pulkomandy
Priority: normal Milestone: Unscheduled
Component: Kits/Locale Kit Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

  1. Implement BDateFormat::GetShortMonthName(): To get short month name.

(e.g Jan, Feb etc.)

  1. Implement BDateFormat::GetDayName(): To get long day name (e.g Monday, Tuesday etc.)
  2. Implement BDateFormat::GetShortDayName(): To get short day name (e.g Mon, Tue etc.)

Change History (18)

comment:1 by akshay, 2 years ago

Has a Patch: set

comment:2 by pulkomandy, 2 years ago

I would rather extend GetMonthName with an extra parameter (a BDateFormatStyle), and implement a single GetDayName with a similar parameter.

comment:3 by akshay, 2 years ago

Has a Patch: unset

comment:4 by akshay, 2 years ago

Has a Patch: set

comment:5 by akshay, 2 years ago

I wrote to the icu-support mailing list, they told the value of count is 8 for weekdays because the first element is not used and they are using an offset of 1. It's been like that since the beginning of ICU to make it possible to use the weekday constants directly as offsets.

initializer code (there is an offset of 1)

http://bugs.icu-project.org/trac/browser/trunk/icu4c/source/i18n/dtfmtsym.cpp#L2377 http://bugs.icu-project.org/trac/browser/trunk/icu4c/source/i18n/unicode/calendar.h#L255

comment:6 by akshay, 2 years ago

Also now I am using icu::DateFormatSymbols::getWeekdays and getMonths which takes context and width as an argument. In our API I am only allowing specifying of the width and keeping the context as standalone for both the month and weekday name because I guess we simply need the month name here irrespective of the date pattern.

http://www.icu-project.org/apiref/icu4c/classicu_1_1DateFormatSymbols.html#a961239103aa7be4c22384c265333c19f http://www.icu-project.org/apiref/icu4c/classicu_1_1DateFormatSymbols.html#a4dd415f62c609b04c096976f6205dcf4

comment:7 by pulkomandy, 2 years ago

You could reuse BDateFormatStyle from http://cgit.haiku-os.org/haiku/tree/headers/os/locale/FormattingConventions.h instead of adding BDateFormatWidth? They may not exactly match, but having two enums looks like the user of the API could mistake one for the other.

I also think that the icu DateFormat created by _CreateDateFormat could be stored as a field of the BDateFormat class. This would avoid re-creating it everytime we call one of these functions?

comment:8 by akshay, 2 years ago

How do I do it then if I use BDateFormatStyle..Use full or long for wide, short for short, medium for abbreviated? and for narrow? Wouldn't that make it confusing between date pattern and symbol width? The icu docs are really confusing in this regard..(lot of confusion over abbreviated and short)

ya we can create the icu DateFormat during initialization and apply the style within the overloaded Format() , GetMonthName() and GetDayName() ?

But the way the style is applied currently is a bit difficult to understand. Why are we not passing EStyle directly (long/full/short) while calling createDateInstance here.. https://github.com/haiku/haiku/blob/master/src/kits/locale/DateFormat.cpp#L361

comment:9 by pulkomandy, 2 years ago

For the enums, I don't mind renaming the values so they make sense in both contexts.

For the date formatter, it's been a while since I wrote this code, but I'm starting to remember some things about it. The class is designed so methods can be called from multiple threads without creating interference. This is why each call will create a local date formatter using _CreateDateFormatter, avoiding conflicts with other threads using the same object (since eventually they won't share any ICU context).

However, creating the formatter on ICU side is a costly operation. So maybe creating it once and using a lock to protect it would be better.

One would need to check which guarantees ICU makes about thread safety. For example, if it is possible to call "query" (read-only) functions from multiple threads without locking, then we could create the date format once, and then use it from multiple methods, but only as long as they don't need to change the internal object state.

Your help on this is very welcome, however it may be going outside the original scope of your GSoC project. That's fine by me, I appreciate that you are digging in Haiku sourcecode and fixing the bugs there. But it's your decision wether you want to invest more time on this or prefer to get the calendar app going first.

comment:10 by axeld, 2 years ago

It would also be possible to document that a BDateFormatter must not be used from more than one thread, and don't use locking there. For time critical things this might actually be the best option.

A possible extension would then be a BThreadSafeDateFormatter that actually does the locking, if convenience is more important than performance :-)

Last edited 2 years ago by axeld (previous) (diff)

comment:11 by pulkomandy, 2 years ago

Ok, let's declare BDateFormatter is not handling thread safety issues by itself, and should be used from only one thread, or with external locking. This will allow to create the formatter(s) at class creation, and then use it/them for all formatting needs.

comment:12 by akshay, 2 years ago

Just saw your last comment..

I'll work on this today and revert back as I face any issues...

comment:13 by akshay, 2 years ago

I have updated the original patch to use BDateFormatStyle. I am using it as it is without renaming the values. Will make another patch for the optimization part shortly.

comment:14 by pulkomandy, 2 years ago

Hi,

The two remaining patches don't apply anymore (I guess they conflict with other changes done). Could you please rebase them on the current sources?

And if you want to try it, you can now submit patches using Gerrit at https://review.haiku-os.org .

comment:15 by pulkomandy, 18 months ago

Resolution: fixed
Status: newclosed

Patch was already applied as bb3ad41ada3ad8.

Note: See TracTickets for help on using tickets.