Opened 2 years ago

Closed 2 years ago

#13508 closed bug (fixed)

[Patch] Resolve Style Formatting Issue in BTimeUnitFormat/BDurationFormat

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

Issue:

BTimeUnitFormat doesn't incorporate style formatting while formatting a time unit. Format() does take style as an argument but the style is not used anywhere. So currently the abbreviated style(B_TIME_UNIT_ABBREVIATED) doesn't work and by default the time unit is formatted to the full style.

Fix:

  1. Move the style flag from BTimeUnitFormat::Format() to the BTimeUnitFormat constructors and call the relevant icu::TimeUnitFormat constructor.
  2. Map the Haiku defined style unit to the corresponding ICU unit.
  3. Move the style flag from BDurationFormat::Format() to the BDurationFormat constructors to map the changes in BTimeUnitFormat.

Attachments (2)

Change History (12)

comment:1 Changed 2 years ago by akshay

Has a Patch: set

comment:2 Changed 2 years ago by pulkomandy

Commit message:

  • Please break lines to a reasonable width (usually 80 columns)

TimeUnitFormat.h:

  • Why the change in enum order?

DurationFormat.cpp

  • Line 38: indentation problem? Should be indented just 1 tab (4 columns)

TimeUnitFormat.cpp

  • If the style is invalid, fFormatter is never set. It should be set to NULL.
  • I think you do not need the "s" prefix in skStyleMap. kStyleMap should be enough. Also it could have a more descritive name, kTimeUnitStyleToICU maybe?

comment:3 Changed 2 years ago by akshay

  1. I'll fix the commit message.
  1. Just to keep the order same: http://www.icu-project.org/apiref/icu4c/tmutfmt_8h_source.html
  1. Will fix the indentation issue.
  1. 'sk' was for static constant? kTimeUnitStyleToICU is fine to me.

comment:4 Changed 2 years ago by pulkomandy

I understand the logic for "sk", but I don't think we usually combine the letters. I could be wrong :)

The order for the constants does not need to be the same as there is a map to convert them. I think the important thing would be that all our constants are in the same order, and IIRC we went with "shortest to longest" for other formatting constants (for example for date formats), so it would be nice to have them all in the same order in the public APIs.

comment:5 Changed 2 years ago by akshay

Don't know about that. Don't see any mention to static constant in coding guidelines. Also the patch submission guidelines says this: "When patching existing files that do not follow our Coding Guidelines, it is preferable to apply the stylization to the entire file. If that is not possible, then conform to the pervading style being used." So do I change the same for skUnitMap?

Okay I'll change it to the original order.

comment:6 Changed 2 years ago by akshay

Has a Patch: unset

comment:7 Changed 2 years ago by akshay

Has a Patch: set

comment:8 Changed 2 years ago by akshay

pulkomandy: Do I need to make any other changes to the patch?

comment:9 Changed 2 years ago by pulkomandy

Applied in hrev51173. Thanks!

comment:10 Changed 2 years ago by korli

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.