Open Bug 302253 Opened 19 years ago Updated 7 months ago

Use graphical timezone picker in preferences

Categories

(Calendar :: Preferences, enhancement)

enhancement

Tracking

(Not tracked)

People

(Reporter: jminta, Unassigned, Mentored)

References

Details

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

Attachments

(3 files, 1 obsolete file)

As shaver has said in a few comments on other bugs, a graphical timezone picker
is pretty much a must-have.
Attached patch first draft โ€” โ€” Splinter Review
Here's a first draft of where I'd like to go with this bug.  It's currently
placed in calendar, not base, but that's because the only reasonable place I
could implement it was in Sunbird's preferences.  It's currently a xul window,
but it could be xbl-ified if desired.  However, I don't think that's a good
idea.  It's too big to be put in a popup, so there wouldn't be much use for it.


We also need to come up with a good way to pre-load it, since it takes
~2seconds to load on my computer.
Assignee: shaver → jminta
Status: NEW → ASSIGNED
Attached image world_map.png โ€”
The map I used for this, stolen from GNOME (and not properly enterred in jar.mn
in the above patch :-( sorry!)
IF there are licensing issues with the map, i think both nasa and esa provided a
free satellite photo, which you can use to create a map.
IMHO this timezone picker should involve also geographical information to display LONG/LAT data as found in the vCal format or postal code format UPU S42 and also could be associated to via external location service. see: http://microformats.org/wiki/location-formats#vCard_.26_hCard
This way locations (also restricted to those commonly used by the user) can be immediately associated with timezones and vice versa.

BTW it would help if the map would be available in multiple resolutions.
I'm Joakim Ziegler, the creator of this map image. I'm hereby granting the use of the image under Mozilla's three licenses.
Re-assigning my bugs to nobody@mozilla.org due to recent developments.
Assignee: jminta → nobody
Status: ASSIGNED → NEW
Version: Trunk → unspecified
AFAIK, there is today a graphical timezone picker. So, should this become "need a better graph t pick" or "need a richer t p .." ?

related ? bug 436260
Keywords: helpwanted
We do have a graphical timezone picker, but we don't use it everywhere we could be (i.e the prefs dialog). I thought there was another bug on this, but I guess this will do.
Summary: Need a graphical timezone picker → Use graphical timezone picker in preferences
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=Fallen][lang=js]
Attachment #620829 - Flags: review?(matthew.mecca)
Comment on attachment 620829 [details] [diff] [review]
Patch contains the update made in timezones.js and timezones.xul

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

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 work well.

Please refer to https://wiki.mozilla.org/Calendar:Style_Guide for more info on the following:

::: calendar/base/content/preferences/timezones.js
@@ +47,5 @@
>       * Initialize the timezones pref pane. Sets up dialog controls to match the
>       * values set in prefs.
>       */
>      init: function gTP_init() {
> +		

No need to add this extra line here at the beginning of the function block.

@@ +48,5 @@
>       * values set in prefs.
>       */
>      init: function gTP_init() {
> +		
> +		var tzMenuList = document.getElementById("calendar-timezone-menulist");

It looks like the code in this line didn't actually change, but the spaces were changed to tabs. Please use spaces instead of tabs here and elsewhere, being mindful of the indentation guidelines in the style guide.

@@ +53,4 @@
>          var tzMenuPopup = document.getElementById("calendar-timezone-menupopup");
>  
> +        var tzService = cal.getTimezoneService(); 
> +        var enumerator = tzService.timezoneIds; 

It looks like these lines also did not change, other than an extra space added at the end of the line. Please try to avoid stray whitespace changes - you can tell by looking at your diff file in a text editor, lines that you didn't actually change code on should not show up as changed, if they do there is probably a stray whitespace change.

Optionally you could change the "var" in these lines to "let", but either way please remove the trailing space.

@@ +59,5 @@
>          // don't rely on what order the timezone-service gives you
>          while (enumerator.hasMore()) {
>              var tz = tzService.getTimezone(enumerator.getNext());
>              if (tz && !tz.isFloating && !tz.isUTC) {
> +                var displayName = tz.displayName; 

also here,

@@ +64,2 @@
>                  displayNames.push(displayName);
> +                tzids[displayName] = tz.tzid; 

here,

@@ +67,5 @@
>          }
>          // the display names need to be sorted
>          displayNames.sort(String.localeCompare);
>          for (var i = 0; i < displayNames.length; ++i) {
> +            var displayName = displayNames[i]; 

and here.

@@ +77,5 @@
>              prefValue = calendarDefaultTimezone().tzid;
>          }
>          tzMenuList.value = prefValue;
> +		updateTimezone();
> +

No need to add a blank line at the end of the function block (and again, please use spaces instead of tabs)

@@ +83,5 @@
>  };
> +
> +function updateTimezone() {
> +	var menuList=document.getElementById("calendar-timezone-menulist");
> +	let menuItem=menuList.selectedItem;

Please use let instead of var, here and elsewhere in the patch. Also, please wrap the = operators with spaces.

@@ +90,5 @@
> +		let standardTZOffset = "none";
> +		if (tz.isUTC) {
> +			standardTZOffset = "+0000";
> +		}
> +		else if (!tz.isFloating) {

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

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

@@ +99,5 @@
> +		}
> +
> +		let image = document.getElementById("highlighter");
> +		image.setAttribute("tzid", standardTZOffset);
> +	

No need to add this blank line at the end of the code block.

::: calendar/base/content/preferences/timezones.xul
@@ +37,5 @@
>     - the provisions above, a recipient may use your version of this file under
>     - the terms of any one of the MPL, the GPL or the LGPL.
>     -
>     - ***** END LICENSE BLOCK ***** -->
> +   

The extra line is OK here, but it shouldn't have any spaces or tabs on it.

@@ +52,5 @@
>          <script type="application/javascript"
>                  src="chrome://calendar/content/calendar-ui-utils.js"/>
>          <script type="application/javascript"
>                  src="chrome://calendar/content/calUtils.js"/>
> +		

Same here

@@ +62,5 @@
>  
>          <groupbox>
>              <caption label="&pref.calendar.timezones.list.caption;"/>
>  
> +  

No need for this extra blank line since there's already one above it.

@@ +73,5 @@
> +	  <stack id="timezone-stack">
> +		 <image src="chrome://calendar/skin/timezone_map.png"/>
> +		 <image class="timezone-highlight" tzid="+0000" id="highlighter"/>
> +	  </stack>
> +	</vbox>

tabs and trailing whitespace as mentioned above.
Attachment #620829 - Flags: review?(matthew.mecca) → review-
Assignee: nobody → mib.2ojmp8
Severity: normal → enhancement
Status: NEW → ASSIGNED
Component: Internal Components → Preferences
QA Contact: base → preferences
What does this "graphical timezone picker" mean?

Users could click on the map and the timezone is chosen without going through the drop-down list of cities?
Attached patch Updated Patch โ€” โ€” Splinter Review
Comment on attachment 637186 [details] [diff] [review]
Updated Patch

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

r- to stick this into a binding. I really hope you are still interested, even though I've left this hanging for quite some time.

::: calendar/base/content/preferences/timezones.js
@@ +83,5 @@
> +/**
> + * Handler function to call when the timezone selection has changed. Updates the
> + * timezone-time field and the timezone-stack.
> + */
> +function updateTimezone() {

This function would be better kept in the binding mentioned below, as its duplicate code from the event dialog.

::: calendar/base/content/preferences/timezones.xul
@@ +71,5 @@
>  
> +        <stack id="timezone-stack">
> +            <image src="chrome://calendar/skin/timezone_map.png"/>
> +            <image class="timezone-highlight" tzid="+0000" id="highlighter"/>
> +        </stack>

It would make sense to put this into a binding, the tzid attribute being inherited from the binding. Extra points for renaming it to tzoffset since thats really what it is.

Aside from what I wrote in the email, you should probably extend the binding to also include the menulist. The only thing that wouldn't fit in is the preference attribute on the menulist, so you need to do something like here: https://developer.mozilla.org/en-US/docs/Mozilla/Preferences/Preferences_system/New_attributes#preference-editable
Attachment #637186 - Flags: review?(philipp) → review-
Can you confirm that you're still working on this bug?
Flags: needinfo?(mib.2ojmp8)
As mailed to Mhoye, I am not working on this bug now.
I apologize for not updating it earlier,
Assignee: mib.2ojmp8 → nobody
Flags: needinfo?(mib.2ojmp8)
Status: ASSIGNED → NEW
Attachment #620829 - Attachment is obsolete: true
Mentor: philipp
Whiteboard: [good first bug][mentor=Fallen][lang=js] → [good first bug][lang=js]
I would like to work on this Bug.
Great to hear! Do you have enough information to get started? Please find me on IRC if you have questions, or email me if I am not around.
Assignee: nobody → pathfinder.rover11b
Thank you Fallen, I already started to setup Mozilla build on my MacOS, and I'll contact you through the Chat for further information. Thank you Marlio.
Keywords: good-first-bug
Whiteboard: [good first bug][lang=js] → [lang=js]
Assignee: pathfinder.rover11b → nobody
Keywords: helpwanted

I'd like to take a shot at this, could I be assigned?

I would like to try my luck with this bug if it is still available

Severity: normal → S3
See Also: → 647632
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: