370305 - timepicker: double-click on hour should set ":00"

Status: VERIFIED FIXED

(Calendar :: General, enhancement)

Description

[Ray Kiddy]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20061006 Sunbird/0.3

I double-click in a day on the calendar. It opens a sheet for a task. I double-click on the 19 in the hours list and the 19 hours is selected.

BUT I still have to click on ":00". Why?

I am going to go out on a limb and say that a lot of tasks start on the hour.

So, if it could be that a double-click on the hour is all that is needed to start the task on the hour, I would not have to go back to the mouse and navigate to the ":00". I would very much appreciate that.

Comment 1

[gekacheka]

Sounds like a good idea for a nice little convenience in the timepicker popup.  
clarifying summary.  enhancement.
timepicker bugs seem to be in 'General'

Comment 2

[u278231]

Sounds like a good one for a beginner - I'll do it.

Comment 3

[u278231]

Attachment: First try at a patch for this enhancement

So after a little bit of getting-to-know-each-other time with XUL, here's my proposed patch for this enhancement. Essentially all I did was add a method called by double clicking on an hour selector, which selects the minute :00 as well as the hour you clicked on.

Since this is the first time I've submitted a patch, I will admit to being uncertain about the review process. Does all code have to be reviewed? If so, who should be the Requestee? I marked mine for ui-review, with that field blank.

Please let me know if there's anything I need to change, or any errors in my submittal process; I'd be glad to oblige.

Comment 4

[Simon Paquet [:sipaq]]

Comment on attachment 263893 [details] [diff] [review]
First try at a patch for this enhancement

Hi Felix,
please take a look at http://wiki.mozilla.org/Calendar:Review_Process and http://wiki.mozilla.org/Calendar:Module_Ownership

I've added the necessary people as reviewers for your patch. 

To ease the UI review, you should also attach a screenshot showing the revised behaviour of the application.

Comment 5

[u278231]

Hello Simon,

Thanks for the help, I will make sure to explore the wiki further. I will, however, hold off on the screenshot, simply because the behavior added by the patch consists of closing a dialog - which is of course rather hard to capture visually. Cheers!

Comment 6

[Michael Büttner (no reviews TFN)]

Comment on attachment 263893 [details] [diff] [review]
First try at a patch for this enhancement

>+            // select the item
>+            this.selectHourItem( hourItem );
>+
>+            // Change the hour in the selected time. 
>+            this.mSelectedTime.setHours( hourNumber );
>+
>+            this.hasChanged = true;
The handler for double clicks gets called after the click handler has already been called. That's why we already passed the clickHour() method, which makes the above code redundant.

>+            this.selectMinuteItem(document.getAnonymousElementByAttribute(this, "anonid", "time-picker-five-minute-box-0"));
There's no need to select the minute-box, since the popup will be closed right away.

>+            this.hasChanged = true;
This has already been executed once in clickHour().

Basically, you just have too much code in doubleClickHour(). Anyway, thanks for the patch. Finally somebody is tackling this annoying issue :-) I'm eagerly awaiting the next iteration to get this fixed...

Comment 7

[u278231]

Attachment: Seond iteration

Thanks for the feedback! I wasn't aware of the fact that the click event happens even if doubleclick is getting called; that certainly changes things. With that in mind, here's the revised version. All the best,

Felix

Comment 8

[Michael Büttner (no reviews TFN)]

Comment on attachment 264697 [details] [diff] [review]
Seond iteration

>+            // set the minutes to :00
>+	     this.mSelectedTime.setMinutes(0);
Just a style nit, this ^^^ line contains a tab. I'll fix that before checking in.

The patch works as advertised, thanks. r=mickey.

Comment 10

[Christian Jansen]

Comment on attachment 264697 [details] [diff] [review]
Seond iteration

r=christian, patch works as expected.

Comment 11

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 12

[Andreas Treumann]

Verified this issue in lightning (20070773103) and Sunbird (20070731) -> task is fixed.