Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#12101 - Option for date format

Attached to Project: Rockbox
Opened by Matthieu Pupat (tieum) - Saturday, 07 May 2011, 02:52 GMT+2
Last edited by sideral (sideral) - Tuesday, 10 May 2011, 09:19 GMT+2
Task Type Patches
Category User Interface
Status Unconfirmed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Private No

Details

Attached is a patch that:

* adds an option to chose the date format in the system menu (choice between YMD, DMY and MDY)
* adds a %cF theme tag so that theme can know in which order to display the date

There are a few new strings that I only added in English.

This is my very first rockbox patch so please let me know if anything should be improved.

I did the patch against svn 29827 and tested it in a simulated Fuze v2 as well as a real one.
   time_format.diff (7.1 KiB) 
 apps/lang/english.lang             |   68 +++++++++++++++++++++++++++++++++++++
 apps/gui/skin_engine/skin_tokens.c |    6 +++
 apps/settings.h                    |    1 
 apps/menus/time_menu.c             |   24 +++++++++++--
 apps/talk.c                        |   21 +++++++++--
 apps/settings_list.c               |    3 +
 lib/skin_parser/tag_table.c        |    1 
 lib/skin_parser/tag_table.h        |    1 
 8 files changed, 119 insertions(+), 6 deletions(-)

This task depends upon

Comment by Alexander Levin (fml2) - Saturday, 07 May 2011, 21:11 GMT+2
I have several thoughts on this.

1. I'm not sure we need such configurability in the menu. The date is set once and is then just there. Where we do need configurability is in the WPS or in the status bar, but then we already have tags for specific parts of the date, so the theme authors can display it how they want. The same applies to the time format, so I'd exclude that option too.

If we'd still include this feature, I have some (pure subjective) comments on the code style.

2. I'd introduce constants (or enums) for the date formats, e.g. DATEFORMAT_DMY, ..._MDY, ..._YMD

3. I would rename the lang symbols from LANG_DATE_YMD to LANG_DATEFORMAT_YMD etc.

4. I would rename the wps token to SKIN_TOKEN_RTC_DATEFORMAT

5. I would place the menu option for the date format *before* the option for the time format

6. In the switch (global_settings.dateformat) I'd probably introduce local variable for day, month, and year, and then use them in sprintf -- to avoid code duplication. Same for the 'talk_date'.

Otherwise the code looks OK to me (I have not tried it though).

But: see 1 above.
Comment by Matthieu Pupat (tieum) - Saturday, 07 May 2011, 21:20 GMT+2
For time there is %cf that allows themes to know if they should display the date in 12 or 24 format (although all themes does not take advantage of this). The thinking being this option is to make exactly the same available for the date. Now the date display in themes is fixed depending on what the theme authors choose. For sure I can hack in my preferred theme and change the order but it would be nice if I can use any theme and choose to display the time and date the way I am used to it and not the way the theme authors choose too.
Comment by Thomas Martitz (kugel.) - Saturday, 07 May 2011, 21:29 GMT+2
You can read all settings from the WPS via %St, no need for a new tag.
Comment by Matthieu Pupat (tieum) - Sunday, 08 May 2011, 20:20 GMT+2
I did nit know this. Thanks. But you still need to have the option available for the user to choose which this patch does then it is easier to have a dedicated option.
Comment by Marek Salaba (salaba) - Monday, 09 May 2011, 10:41 GMT+2
Set the date format is very great idea for non-english speaking countries / users.

Loading...