809018 - DateTimePicker does not have 12 hour mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Offshoot from Bug 808405. DateTimePicker should have a 12 hour mode according to current locale.

Comment 1

[cheta.gup]

I am completely new to bugzilla , moreover i don't find the interface of bugzilla user friendly , if someone can please tell me the procedure of getting a mentor to help me fixing this bug

Comment 2

[Wesley Johnston (:wesj)]

Jim can help you by putting some comments in here, or you can come try to find someone to help in #mobile in irc. First step to working on this will be to build Fennec on your machine though. The steps to doing that are listed in gory super detail here:

https://wiki.mozilla.org/Mobile/Fennec/Android

But basically you need to be able to build firefox:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
and then also download the Android SDK and NDK.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to cheta.gup from comment #1)
> I am completely new to bugzilla , moreover i don't find the interface of
> bugzilla user friendly , if someone can please tell me the procedure of
> getting a mentor to help me fixing this bug

Hi Cheta, thank you for your interest in the bug! Since I filed this bug, I can be your mentor. You can reach me through IRC: I use the nick 'jchen' in #mobile channel on irc.mozilla.org. Feel free to talk to me anytime. You can also reach me through the email address when you click on my name here in Bugzilla. I will send you an email.

Do you have a device to test this bug with? The DateTimePicker feature in this bug is supported on Android >= 3.0. Like Wes said, your first step is to make a build environment. You would need Linux, OS X, or a VM for developing Fennec. The guide at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_Fennec is a good start.

Once you have the source code and know how to build Fennec, you can start looking through some of the Fennec code. This bug is in the Java portion of the Fennec code, which is under the directory mobile/android. More specifically,  DateTimePicker is in the file mobile/android/base/widget/DateTimePicker.java

The DateTimePicker is the box shown when you use an input field with type="datetime" or "datetime-local". Inside Fennec, if you go to this address, http://miketaylr.com/pres/html5/forms2.html, and click on the box next to "Datetime", you will be able to see the DateTimePicker.

The bug in this case is that the DateTimePicker does not have a mode to set time in 12 hour format (for example, it shows "13:34" instead of "1:34 PM"), even if the system is set to display time in 12 hour format. Usually if you set the system language to English (U.S.), the display time should change to 12 hour format. Therefore, the gist of this bug is to add an optional box for selecting AM/PM, and add logic to limit the hour to 1-12 in 12 hour mode. The code also has to convert the time from 12 hour mode to 24 hour mode when it returns.

I know this is a lot of information, so please let me know if you have any questions. I can guide you through each of these steps above.

Comment 4

[Agam]

Jim: logic looks simple.but how to go around the class DateTimePicker?.
i am also new here

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Any progress, Agam? Let me know if you have questions.

Comment 6

[Agam]

*been travelling*
yeah..modified once.have to try.
simple doubt,we build again everytime?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Agam from comment #6)
> *been travelling*
> yeah..modified once.have to try.
> simple doubt,we build again everytime?

Yes you have to build again and install the apk in order to have the latest version on your emulator/device. After you build one time, future builds should be a lot faster.

Comment 8

[Agam]

Attachment: 1st patch.works.not tested much

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Agam from comment #8)
> Created attachment 700878 [details] [diff] [review]
> 1st patch.works.not tested much

Great work Agam! I just have a few comments,

1) Some countries use 12-hour and other countries use 24-hour, so we should have both 24-hour and 12-hour modes. For example, you can add a boolean field, mIs12HourMode. Then, we should set the mode based on the user setting. See http://developer.android.com/reference/android/text/format/DateFormat.html#is24HourFormat%28android.content.Context%29

2) I think you should change 'Meridian' to 'AmPm'. 'Meridian' is not a commonly-used word to mean morning/afternoon, at least in the U.S.

3) In 12-hour mode, the hours go 12, 1, 2, ..., 10, 11, 12, ... There is no 0 hour in 12-hour mode.

4) Also, instead of having "AM" and "PM" in the code, you should use strings in the system language. See http://developer.android.com/reference/android/text/format/DateUtils.html#getAMPMString%28int%29

Keep up the good work Agam!

Comment 10

[Agam]

Attachment: suggestion 1 and 3 done.2 should be done at last.problem in sugg 4

1.flip mis12HourMode to see change for both cases(comment line in setCurrentLocale.
2.will meridian to AMPM everywhere once rest issues are resolved
3.done
4.couldnt see/test it on the emulator.tried both android:visibility="invisible" and setVisibility().any suggestions ?

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 701753 [details] [diff] [review]
suggestion 1 and 3 done.2 should be done at last.problem in sugg 4

(In reply to Agam from comment #10)
> Created attachment 701753 [details] [diff] [review]
> suggestion 1 and 3 done.2 should be done at last.problem in sugg 4

Looks great!

> 1.flip mis12HourMode to see change for both cases(comment line in
> setCurrentLocale.
> 2.will meridian to AMPM everywhere once rest issues are resolved
> 3.done
> 4.couldnt see/test it on the emulator.tried both
> android:visibility="invisible" and setVisibility().any suggestions ?

Is you look in the displayPickers() method, you can see that setHourShown() is not called for (mState == pickersState.DATETIME), which is our case here. So your code in setHourShown() is not run at all. I suggest you add a set12HourShown() method and call it from displayPickers().

Also you should normally use "gone" for android:visibility or setVisibility, not "invisible"

Great progress! Keep it up! :)

Just a quick tip, when your patch is still a "work in progress", it is better to use the "feedback" option rather than the "review" option. And when your patch is completely finished, you can then use "review".

Comment 12

[Agam]

Attachment: all 4 issues fixed.only need to remove the comments then identation and test

sry for taking too long.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 702665 [details] [diff] [review]
all 4 issues fixed.only need to remove the comments then identation and test

(In reply to Agam from comment #12)
> Created attachment 702665 [details] [diff] [review]
> all 4 issues fixed.only need to remove the comments then identation and test
> 
> sry for taking too long.

You're doing great! just a couple more issues that I found,

1) If you switch between AM/PM, you can see the date changes as well. This is because you're calling "setTempDate(Calendar.AM_PM, ...". The setTempDate method adjusts the date too. The better way is to just call
> mTempDate.set(Calendar.AM_PM, newVal);//

2) If you set the time to 12:00AM, and click "Set", the result will show "24:00" in the input field, but you want it to show "00:00" in the input field.

3) Now in 24-hour mode, the hours still go from 01 to 12, it should go from 00 to 23.

Also, when you have a new patch, you can mark your old patch as obsolete under the "Obsoletes" section in the new attachment page.

You're almost done! :)

Comment 14

[Agam]

Attachment: issues fixed excepted the formatting

any leads about fixing the format issue ?

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 703978 [details] [diff] [review]
issues fixed excepted the formatting

Review of attachment 703978 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Agam from comment #14)
> Created attachment 703978 [details] [diff] [review]
> issues fixed excepted the formatting
> 
> any leads about fixing the format issue ?

I think that's a separate bug, so you don't have to do anything about it now :)


Your patch looks great! Just some small style changes below and you are very close to finishing!

(I changed review to feedback for now. Please ask for review again for your next patch. Thanks!)

::: mobile/android/base/resources/layout/datetime_picker.xml
@@ +111,5 @@
>                  android:layout_marginRight="1dip"
>                  android:focusable="true"
>                  android:focusableInTouchMode="true"
>                  />
> +	

Please remove extra spaces here. Empty lines should not have extra spaces.

@@ +129,5 @@
>                  android:layout_marginRight="1dip"
>                  android:focusable="true"
>                  android:focusableInTouchMode="true"
>                  />
> +	

Extra spaces.

::: mobile/android/base/widget/DateTimePicker.java
@@ +106,5 @@
>      public static enum pickersState { DATE, MONTH, WEEK, TIME, DATETIME };
>  
>      public class OnValueChangeListener implements NumberPicker.OnValueChangeListener {
>          public void onValueChange(NumberPicker picker, int oldVal, int newVal) {
> +	    updateInputState();

You are using tabs in your patch, and we want to only use spaces. So you should replace tabs with spaces. Your text editor may have an option to change tabs to spaces.

@@ +125,5 @@
>                      int old = mTempDate.get(Calendar.WEEK_OF_YEAR);
>                      int maxWeekOfYear = mTempDate.getActualMaximum(Calendar.WEEK_OF_YEAR);
>                      setTempDate(Calendar.WEEK_OF_YEAR, old, newVal, 0, maxWeekOfYear);
>                  } else if (picker == mYearSpinner && mYearEnabled) {
> +		    int month=mTempDate.get(Calendar.MONTH);

It's good style to put spaces around
operators like =, i.e. "month = mTempDate"

@@ +138,5 @@
>                  } else if (picker == mHourSpinner && mHourEnabled) {
> +		      if(mIs12HourMode){
> +		      setTempDate(Calendar.HOUR, oldVal, newVal, 1, 12);
> +		      }
> +		   else	{

Put 'else {' in the same line as '}', i.e. 

if (...) {
  ...
} else {
  ...
}

@@ +145,4 @@
>                  } else if (picker == mMinuteSpinner && mMinuteEnabled) {
> +		      setTempDate(Calendar.MINUTE, oldVal, newVal, 0, 59);
> +                } else if (picker == mAMPMSpinner && mHourEnabled) {
> +		      mTempDate.set(Calendar.AM_PM,newVal);	

There is an extra tab at the end

@@ +145,5 @@
>                  } else if (picker == mMinuteSpinner && mMinuteEnabled) {
> +		      setTempDate(Calendar.MINUTE, oldVal, newVal, 0, 59);
> +                } else if (picker == mAMPMSpinner && mHourEnabled) {
> +		      mTempDate.set(Calendar.AM_PM,newVal);	
> +                }else {

Space before 'else', i.e. '} else {'

@@ +173,5 @@
>                  } else if (picker == mHourSpinner && mHourEnabled){
> +			  if(mIs12HourMode){
> +			      mTempDate.set(Calendar.HOUR, newVal);
> +			  }
> +			  else	{

} else {

@@ +180,4 @@
>                  } else if (picker == mMinuteSpinner && mMinuteEnabled){
>                      mTempDate.set(Calendar.MINUTE, newVal);
> +                } else if (picker == mAMPMSpinner && mHourEnabled){
> +                    mTempDate.set(Calendar.AM_PM, newVal);//13th

Remove comment at the end.

@@ +338,5 @@
>  
>          mYearSpinner = setupSpinner(R.id.year, DEFAULT_START_YEAR,
>                                      DEFAULT_END_YEAR);
>          mYearSpinnerInput = (EditText) mYearSpinner.getChildAt(1);
> +//my 0th edit..11

Remove comment

@@ +343,4 @@
>  
> +	mAMPMSpinner = setupSpinner(R.id.ampm, 0, 1);
> +        mAMPMSpinner.setFormatter(TWO_DIGIT_FORMATTER);
> +	

Extra tab

@@ +346,5 @@
> +	
> +	if(mIs12HourMode)	{
> +	    mHourSpinner = setupSpinner(R.id.hour, 1, 12);//b3
> +	    mAMPMSpinnerInput = (EditText) mAMPMSpinner.getChildAt(1);
> +	    mAMPMSpinner.setDisplayedValues(mShortAMPMs); //9

Remove end comments

@@ +348,5 @@
> +	    mHourSpinner = setupSpinner(R.id.hour, 1, 12);//b3
> +	    mAMPMSpinnerInput = (EditText) mAMPMSpinner.getChildAt(1);
> +	    mAMPMSpinner.setDisplayedValues(mShortAMPMs); //9
> +	}
> +	 else	{

} else {

@@ +350,5 @@
> +	    mAMPMSpinner.setDisplayedValues(mShortAMPMs); //9
> +	}
> +	 else	{
> +	    mHourSpinner = setupSpinner(R.id.hour, 0, 23);
> +	    mAMPMSpinnerInput=null;

Space; 'mAMPMSpinnerInput = null'

@@ +352,5 @@
> +	 else	{
> +	    mHourSpinner = setupSpinner(R.id.hour, 0, 23);
> +	    mAMPMSpinnerInput=null;
> +	 }
> +	

Extra tab

@@ +358,2 @@
>          mHourSpinnerInput = (EditText) mHourSpinner.getChildAt(1);
> +	

Extra tab

@@ +488,5 @@
> +	    if(mIs12HourMode){
> +            mHourSpinner.setValue(mCurrentDate.get(Calendar.HOUR));
> +		mAMPMSpinner.setValue(mCurrentDate.get(Calendar.AM_PM));
> +		mAMPMSpinner.setDisplayedValues(mShortAMPMs);}
> +	    else{mHourSpinner.setValue(mCurrentDate.get(Calendar.HOUR_OF_DAY));} //bugg

Remove end comment

@@ +578,5 @@
> +	if(shown){
> +	    mAMPMSpinner.setVisibility(VISIBLE);
> +	}
> +	  else{
> +	      mAMPMSpinner.setVisibility(GONE);}

Move '}' to new line

@@ +579,5 @@
> +	    mAMPMSpinner.setVisibility(VISIBLE);
> +	}
> +	  else{
> +	      mAMPMSpinner.setVisibility(GONE);}
> +	}

Please put an empty line here, before 'private void setHourShown(boolean shown) {'

@@ +586,4 @@
>              mHourSpinner.setVisibility(VISIBLE);
>              mHourEnabled= true;
> +	 } 
> +	else {

} else {

@@ +611,5 @@
>              return;
>          }
>  
>          mCurrentLocale = locale;
> +	mIs12HourMode=!DateFormat.is24HourFormat(getContext());//b1

Add space, 'mIs12HourMode = !DateFormat', and remove end comment

@@ +620,5 @@
>          mMaxDate = getCalendarForLocale(mMaxDate, locale);
>          mCurrentDate = getCalendarForLocale(mCurrentDate, locale);
>  
>          mNumberOfMonths = mTempDate.getActualMaximum(Calendar.MONTH) + 1;
> +	

Extra tab

@@ +625,5 @@
> +	mShortAMPMs=new String[2];
> +	mShortAMPMs[0]=DateUtils.getAMPMString(Calendar.AM);
> +	mShortAMPMs[1]=DateUtils.getAMPMString(Calendar.PM);
> +	mShortMonths = new String[mNumberOfMonths];
> +        

Extra tab

@@ +636,5 @@
>      private Calendar getCalendarForLocale(Calendar oldCalendar, Locale locale) {
>          if (oldCalendar == null) {
> +	    return Calendar.getInstance(locale);
> +        } 
> +	else {

} else {

Comment 16

[Agam]

Attachment: first patch :)

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 704585 [details] [diff] [review]
first patch :)

Review of attachment 704585 [details] [diff] [review]:
-----------------------------------------------------------------

Nice job Agam! Congrats on your first bug! :)

Usually at this point you can ask somebody to check-in the patch for you by changing the "checkin" field of the patch to "?", but I can do it for you this time.

Now that you've experienced the process, feel free to look for other bugs that you might be interested in (you can go back to the list at http://www.joshmatthews.net/bugsahoy/?mobile=1 if you want). Let me know if you have more questions. Thanks!

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ccdc82053d

Comment 19

[Daniel Holbert [:dholbert]]

Looks like this landed with the wrong committer info -- the patch contains:
> # User Agam Jain <[e-mail]>
but the cset from comment 18 has jchen as the committer (and reviewer :))

jchen, mind backing out & re-landing w/ that fixed? (you can do that as 2 csets in a single push, so you'll only get 1 bonus TBPL cycle)

Comment 20

[Ryan VanderMeulen [:RyanVM]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> jchen, mind backing out & re-landing w/ that fixed? (you can do that as 2
> csets in a single push, so you'll only get 1 bonus TBPL cycle)

Bonus points for putting DONTBUILD in the top cset so that no bonus tbpl cycles are created.

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e24d955295b

Re-push: https://hg.mozilla.org/integration/mozilla-inbound/rev/00f8e147686a

Thanks guys!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00f8e147686a