302253 - Use graphical timezone picker in preferences

Status: NEW

(Calendar :: Preferences, enhancement)

Description

[Joey Minta]

As shaver has said in a few comments on other bugs, a graphical timezone picker
is pretty much a must-have.

Comment 1

[Joey Minta]

Attachment: first draft

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.

Comment 2

[Joey Minta]

Attachment: world_map.png

The map I used for this, stolen from GNOME (and not properly enterred in jar.mn
in the above patch :-( sorry!)

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

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.

Comment 4

[boris]

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.

Comment 5

[Joakim Ziegler]

I'm Joakim Ziegler, the creator of this map image. I'm hereby granting the use of the image under Mozilla's three licenses.

Comment 6

[Joey Minta]

Re-assigning my bugs to [e-mail] due to recent developments.

Comment 7

[ovidiu]

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

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 9

[mib_2ojmp8]

Attachment: Patch contains the update made in timezones.js and timezones.xul

Comment 10

[Matthew Mecca [:mmecca]]

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.

Comment 12

[石庭豐 (Seak, Teng-Fong)]

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?

Comment 13

[mib_2ojmp8]

Attachment: Updated Patch

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

Comment 15

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 16

[mib_2ojmp8]

As mailed to Mhoye, I am not working on this bug now.
I apologize for not updating it earlier,

Comment 17

[Rakshitha Muranga Rodrigo]

I would like to work on this Bug.

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 19

[Rakshitha Muranga Rodrigo]

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.

Comment 20

[athosar1452]

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

Comment 21

[Manuel Carretero]

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