Timezone selector should automatically pre-selected default timezone

ASSIGNED
Assigned to

Status

Calendar
Dialogs
--
minor
ASSIGNED
7 years ago
2 years ago

People

(Reporter: juergen.edner, Assigned: Merouane Atig, Mentored)

Tracking

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0b8) Gecko/20100101 Firefox/4.0b8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.2.13) Gecko/20101207 Lightning/1.0b3pre Thunderbird/3.1.7

If you're creating a new event and want to change the default timezone, e.g. 'Europe/Berlin' to 'Europe/Istanbul' the list of available timezones always shows all zones in alphabetical order starting with character 'A' instead of preselecting 'Europe/Berlin'. You always have to scroll through the whole list to select an other european timezone.

Reproducible: Always

Steps to Reproduce:
1. create a new event and select the displayed timezone link.
2. the timezone window opens showing the active timezone.
3. open the pull-down list to change the timezone.

Actual Results:  
The list of available timezones shows all timezones in alphabetical order starting with character 'A' although the default timezone is e.g. 'Europe/Berlin'. You always have to scroll through the whole list to select an other european timezone.

Expected Results:  
The list of available timezones should be shown and the default timezone, e.g.  'Europe/Berlin' should be preselected.
Shouldn't be too hard to do, but not a priority for the release. Anyone wanting to get into calendar code, this is a good candidate.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]

Comment 2

7 years ago
The source for this seems to be ./chrome/src/content/calendar/calendar-event-dialog-timezone.xul and the js file.

But the selectedIndex attribute is allready set correctly. If you just hit the scrollbar and move it, you see it is even selected.

So we would have to scroll to the selected position in the menulist, but this should be done by the toolkit, shouldn't it?

Looking at https://developer.mozilla.org/en/XUL/menulist there does not seem to be any scrolling solution.

Btw. this problems applies to way more fields. For example the category dropdown is affected too.
Possibly the value is set to early, try calling that function on a timeout (with setTimeout(func, 0);
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]

Comment 4

6 years ago
Hello,

I would like to work on this bug, can you please assign it to me?
Assignee: nobody → aaron_pua
Status: NEW → ASSIGNED

Comment 5

6 years ago
Aww I had emailed the mentor of this project earlier asking to work on this bug. Oh well, I guess I asked the wrong person.
Mentor was correct! I'll email you in the evening or tomorrow, this bug is definitely a good candidate to get started.
Heh, I had another OReilly webinar ICS entry situation that pushed me to file a bug about this timezone selection dialog. Good to see someone beat me to it.

I think the only decent solution with a list that long would be a text-entry + dropdown combobox. With obvious the local timezone pre-selected.

I'm attaching the ICS that's causing me annoyances:

Error: Unknown timezone "US-Pacific" in "Webcast: Preparing your website for conversion optimization to increase revenue and enhance visitor experience".  Treated as 'floating' local timezone instead: 17.04.2012 10:00
Created attachment 613345 [details]
Forces you to choose a local timezone

Comment 9

5 years ago
So I think I have found the bug and I have made some changes to the code, but I am having trouble trying to test my changes due to the fact that I do not know how to rebuild the project with my changes. Can someone give me a step-by-step process of how I would go about doing this? Sorry, I am a current student and this is my first time working on this sort of project.
(In reply to Aaron Pua from comment #9)
> So I think I have found the bug and I have made some changes to the code,
> but I am having trouble trying to test my changes due to the fact that I do
> not know how to rebuild the project with my changes. Can someone give me a
> step-by-step process of how I would go about doing this? Sorry, I am a
> current student and this is my first time working on this sort of project.

There's some documentation here: https://wiki.mozilla.org/Calendar:Build

If you have any questions, feel free to email me.

Comment 11

5 years ago
Created attachment 622303 [details] [diff] [review]
This patch edits the calendar-event-dialog-timezone.js.

Rather than scrolling through timezones alphabetically, it will detect the default timezone of the country and show those first, then display the rest alphabetically after it. Once it hits the end of the list, it will wraparound to the timezones before the country of the default timezone. 

I am also unsure if I assigned the review to the right person. I see that Phillip is away till May 28 and I need someone to review it before then as this is for a college project due before that time.)
Attachment #622303 - Flags: review?(matthew.mecca)
Comment on attachment 622303 [details] [diff] [review]
This patch edits the calendar-event-dialog-timezone.js.

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

First off, thanks for the patch! I'll be giving r- for this version of the patch, feel free to upload a new version with the following review comments considered.

Functionally, this seems to do what is says, I'll pass ui-review to Richard. One issue I noticed is that this doesn't really solve the problem for regions with many timezones since it splits the list by the first part of the timezone display only - for example, in my case America/New York is the default timezone, but it's still not visible in the list when opened because there are so many timezones starting with America/. I would say that if we're going to split the list rather than display alphabetically, we should split at the default timezone, but I'll leave that to Richard.

Moving on to code review:

- Please use spaces instead of tabs. You can refer to https://wiki.mozilla.org/Calendar:Style_Guide for more details.

::: calendar/base/content/dialogs/calendar-event-dialog-timezone.js
@@ +73,5 @@
>          }
>      }
> +	
> +	//sorting the timezones in alphabetical order
> +	displayNames.sort(String.localeCompare);	

Watch out for stray whitespace changes and trailing whitespace, such as the tab introduced at the end of this line. I suggest using a text editor that offers an option to view whitespace characters to make this easier.

@@ +76,5 @@
> +	//sorting the timezones in alphabetical order
> +	displayNames.sort(String.localeCompare);	
> +	
> +	//finding the default timezone index
> +	var defaultTimezone = calendarDefaultTimezone();

Please use "let" instead of "var", here and elsewhere in the patch.

@@ +78,5 @@
> +	
> +	//finding the default timezone index
> +	var defaultTimezone = calendarDefaultTimezone();
> +	var temp1 = new Array();
> +	var temp2 = new Array();	

Using a more descriptive variable name will make make the code easier to understand by others working on it later. Also no need to initialize these since they will always be directly assigned later before they are referenced - something like:

let defaultTzParts;

@@ +84,5 @@
> +	for (var i = 0; i < displayNames.length; ++i) {
> +		var displayName = displayNames[i];
> +		temp1 = defaultTimezone['tzid'].split("/");
> +		temp2 = displayName.split("/");
> +		if(temp1[0] == temp2[0]){

Please use a single space after "if" and between ")" and "{".
If Richard agrees with my idea on splitting at the default timezone instead, you could probably get rid of the arrays and just use

if (defaultTimezone.displayName == displayName) {

@@ +90,5 @@
> +			break;
> +		}
> +		else {
> +			defaultTimezonePosition = 0;
> +		}

Please place the else clause on the same line as the closing brace, as in

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

In this case I believe you could just initialize defaultTimezonePosition to 0 when it is defined rather than use the else clause here.

@@ +94,5 @@
> +		}
> +	}
> +	
> +
> +	//display timezones starting with the country of the default timezone index to ending index of displayNames array (alphabetical order) 

Please wrap long comments over multiple lines as in

// Long comment line ...
// continued ...
[code]

@@ +103,5 @@
> +	
> +	//display timezones continuing after the end of the displayNames array (wraparound) from beginning index to the country of the default timezone index (alphabetical order) 
> +	for (var i = 0; i < defaultTimezonePosition; ++i) {               
> +		var displayName = displayNames[i];
> +		addMenuItem(tzMenuPopup, displayName, tzids[displayName])

Please include the trailing ';' here

@@ +129,5 @@
>   * @param timezone      The calITimezone to look for.
>   * @return              The index of the childnode below "timezone-menulist"
>   */
>  function findTimezone(timezone) {
> +    var tzid = timezone.tzid; //#################################

Please remove the stray debugging comments here

::: calendar/base/content/preferences/timezones.js
@@ -69,5 @@
>          for (var i = 0; i < displayNames.length; ++i) {
>              var displayName = displayNames[i];
>              addMenuItem(tzMenuPopup, displayName, tzids[displayName]);
>          }
> -

Stray whitespace change here, no actual code changes need to touch this file.

::: calendar/timezones/update.js
@@ +121,5 @@
>      ret.sort();
>      ret += tz.latitude;
>      ret += tz.longitude;
>      return ret;
> +}///////////////////////////////////////////////////////////

Please remove the stray debugging comments here

@@ +246,5 @@
>              oldSet[row.tzid] = oldSet[row.alias];
>          }
>      } finally {
>          statement.reset();
> +    } //////////////////////////////////////////////////////////////////

and here.
Attachment #622303 - Flags: review?(matthew.mecca) → review-
Comment on attachment 622303 [details] [diff] [review]
This patch edits the calendar-event-dialog-timezone.js.

Richard, would you mind taking over ui-review for this?
Attachment #622303 - Flags: ui-review?(richard.marti)

Comment 14

5 years ago
Created attachment 622630 [details] [diff] [review]
Revised Patch

This is a revised patch from the review changes that Matthew Mecca recommended w/o splitting the list starting with the default timezone
Attachment #622630 - Flags: ui-review?(richard.marti)
Attachment #622630 - Flags: review?(matthew.mecca)
Attachment #622303 - Flags: ui-review?(richard.marti)
Comment on attachment 622630 [details] [diff] [review]
Revised Patch

I agree with Mathew the default timezone should be visible and if possible in the upper third.

ui-r- because this patch doesn't do this.

PS: this patch is using tabs instead of spaces.
Attachment #622630 - Flags: ui-review?(richard.marti) → ui-review-

Comment 16

5 years ago
Created attachment 622904 [details] [diff] [review]
UI Revised Patch w/o Tabs

I have made the changes that Richard and Matthew have recommended. This patch now alters the list to start with the default timezone to the end of the list alphabetically, then wraps around starting with the first timezone in the list to the index of the default timezone. Also, I have changed the tabs to spaces.
Attachment #622303 - Attachment is obsolete: true
Attachment #622630 - Attachment is obsolete: true
Attachment #622904 - Flags: ui-review?(richard.marti)
Attachment #622904 - Flags: review?(matthew.mecca)
Attachment #622630 - Flags: review?(matthew.mecca)
Comment on attachment 622904 [details] [diff] [review]
UI Revised Patch w/o Tabs

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

Looks good, there are still a few stray whitespace changes, r=mmecca with those fixed.

Please upload a final patch for check-in with the following fixes and any other changes Richard may specify.

::: calendar/base/content/dialogs/calendar-event-dialog-timezone.js
@@ +98,5 @@
> +    //display timezones continuing after the 
> +    //end of the displayNames array (wraparound) 
> +    //starting from the beginning of the timezone list  
> +    //to the default timezone index (alphabetical order) 
> +    for (let i = 0; i < defaultTimezonePosition; ++i) {               

Please remove the trailing spaces from this line.

::: calendar/base/content/preferences/timezones.js
@@ -69,5 @@
>          for (var i = 0; i < displayNames.length; ++i) {
>              var displayName = displayNames[i];
>              addMenuItem(tzMenuPopup, displayName, tzids[displayName]);
>          }
> -

It looks like a blank line was inadvertently removed here.
Attachment #622904 - Flags: review?(matthew.mecca) → review+

Comment 18

5 years ago
Created attachment 623023 [details] [diff] [review]
Final Revised Patch

Hopefully the final patch. I have followed all the guidelines that Matthew Mecca Recommended.
Attachment #622904 - Attachment is obsolete: true
Attachment #623023 - Flags: ui-review?(richard.marti)
Attachment #623023 - Flags: review?(matthew.mecca)
Attachment #622904 - Flags: ui-review?(richard.marti)
Comment on attachment 623023 [details] [diff] [review]
Final Revised Patch

This is looking good. The only I'm seeing is when the time zone is like Europe/Paris this is on top but when I want to set Europe/Amsterdam then I have to scroll to almost the bottom because of the wraparound. If the scroll position could be steered then the actual time zone could be scrolled into the view. But then the 'Local Time' and UTC/GMT aren't visible.

This patch's behavior is better than the actual, so ui-r=me
Attachment #623023 - Flags: ui-review?(richard.marti) → ui-review+
Attachment #623023 - Flags: review?(matthew.mecca) → review+

Comment 20

4 years ago
Can you confirm that you're still working on this bug?
Flags: needinfo?(aaron_pua)

Comment 21

4 years ago
I am not.
Assignee: aaron_pua → nobody
Flags: needinfo?(aaron_pua)
Status: ASSIGNED → NEW
Mentor: philipp@bugzilla.kewis.ch
Whiteboard: [good first bug][mentor=Fallen][lang=js] → [good first bug][lang=js]
(Assignee)

Comment 22

3 years ago
I would like to take over this issue. It seems to me that Attachment #623023 [details] [diff] has already been positively reviewed. Do I only need to update the patch so that it can be applied?
I'm glad to hear you'd like to take over this issue. The only thing that needs to be taken care of is comment 19. Let me know if you need anything.
Assignee: nobody → yahoocbien-dev
Status: NEW → ASSIGNED
You need to log in before you can comment on or make changes to this bug.