296007 - sorted timezonepicker

Status: NEW

(Calendar :: General, enhancement)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: 

Event dialog will need a timezone picker for choosing timezones for dates.

Bug 224905 develops one approach.

This bug is for exploring an approach more like a menulist.

Eventually would like to be able to use autocomplete, so user can type some
letters or an offset and it will show only the timezones that contain the
substring.  So the descriptions on each line will contain not only the city, but
also the country, the offset from UTC, and the common timezone abbreviations for
it.  

The descriptions would be sorted by offset, then abbreviation, then location.
* Timezones at the same offset would be sorted together.  Timezones with similar
names or abbreviations but different offsets would be separated. (For example,
CST is China Stadard Time as well as Central Standard Time, AST is Atlantic
Standard Time and Arab Standard Time). 
* Timezone offset could be computed dyamically, so an PDT/PST timezone would be
sorted as -07:00 for summer dates and -08:00 for winter dates.
* Within timezones at the same offset, timezones with the same abbreviation
would be sorted together.  (For example, -04:00 AST Atlantic Standard Time
timezones would be grouped together, and -04:00 EDT Eastern Daylight Time
timezones would be grouped together during the EDT summer.)
* Within timezones with the same abbreviation, timezones would be sorted by
country as well as city.  (For example, this would group MST timezones by
country: Canada, Mexico, USA.)


Reproducible: Always

Steps to Reproduce:

Comment 1

[gekacheka]

Attachment: initial timezonepicker, with en-US timezone-names.properties

(patch -l -p 2 -i file.patch)

Prototype implementation provides offset, abbreviation, and country data, and
sorts them to populate a menulist.

Timezone data includes the RFC2445 "floating" timezone.

Patch creates 3 new files, does not change any old files.

Future work:
* initial value should be selected in popup
* on change handlers
* autocomplete

Comment 2

[gekacheka]

Attachment: try timezone picker in event dialog (not connected to events)

(patch -l -p 2 -i file.patch)

This patch simply adds one timezone picker after the start date in the event
dialog for trying it out.  It does not connect the picker to the event, so it
does not initialize the timezone from the event and it does not save the
timezone with the event.

One impression: The sorting works nicely, but 
* it needs to select the initial value so nearby timezones will be easy to
locate.
* it needs something like autocomplete to help filter choices

Q. Which autocomplete would be appropriate?  There seem to be more than one.
   xulplanet lists both mozilla autocomplete and firefox autocomplete,
   though its doc may be out of date, as it says on the firefox 
   nsIAutoCompleteSearch is removed without saying what replaces it.
Q. Is it possible to write a autocompleter in javascript?

Feel free to develop or fork this if you can figure out answers or
alternatives.

Comment 3

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

I'm a bit reluctant on having a duplicate of the data that is already in the
timezone database from libical, but I also don't know a good way of reading that
data.

For the ui: a map of the world where you can click on your city might be nice.

Comment 4

[Mike Shaver (:shaver emeritus)]

For events, I'd like to see us have a short list of the user's
previously-selected timezones, and an "Other..." which pops up a fuller TZ
picker.  We'd also use that larger TZ picker in prefs, if the user pressed the
"Time zone..." button or whatever.

I don't much mind duplicating the libical data in our build, especially if we
can auto-generate it somehow.  Doing something more perfect -- and locale-happy
-- will eat more time than it's worth.

I think the TZpicker should live in base/content, though!

Comment 5

[gekacheka]

Attachment: snapshot v2 of timezonepicker: menulist workarounds, attribute setting handler for pref dialog

(patch -l -p 2 -i file.patch)

[preserving snapshot, not for review]

The preference dialog uses widgetStateManager, which requires that the state be accessible in attributes.  In particular, it restores state using setAttribute.  This timezonepicker uses an internal menulist, so this version adds event listeners so when a value is picked, the timezone picker attribute is set (and available for saving preferences), and when the timezone picker value attribute is set (from restoring preferences), the menulist value is updated.

The values did not propagate straightforwardly; some properties whose implementation gets/sets an attribute did not seem to be effective.  Examples:
when an item was picked, menulist.value was not the value of the selected item, and had to use menulist.selectedItem.getAttribute('value') [see onPick method].
When a value is set via setAttribute, the event seems to hold the label of the item instead of the value of the item [see attribute modified handler].

Feature: this version works in a preference dialog (patch coming).

Annoyance: with this version is that when the timezone menulist pops up, the current timezone is not selected, nor is the list scrolled to show it.  Highlighting the current value doesn't seem to be possible using a menulist of menuitems, since menuitems highlight as soon as you hover the mouse, so a modified approach will be needed.

Comment 6

[gekacheka]

Attachment: try timezone picker in preference dialog (saves & restores, nonstandard element)

(patch -l -p 2 -i file.patch)

Includes wsm_attributes so widget state manager will save its value attribute (not property) even though <timezonepicker> is not a standard element.

(Note that the preferences dialog uses the toolkit widgetStateManager, and does  not work with the old copy of the widgetStateManager in calendar/resources/content/pref/wsm.js.  That file could probably be deleted before it wastes anyone else's time.)

Comment 7

[gekacheka]

Attachment: try timezone picker in preference dialog, v2 (saves & restores, nonstandard element; add ids)

(patch -l -p 2 -i file.patch)

Fix indentation, add ids, add flex.

Comment 8

[gekacheka]

Attachment: v3 timezonepicker: use listbox, highlight current, columns

(patch -l -p 2 -i file.patch)

Changes in v3: 
o <timezonepicker> popup now has a <timezonelist>.
  * <timezonelist> is implemented with a <listbox> of <listitems>, so it
    - scrolls to current item
    - highlights current item
    - uses columns
    - uses a small number of visible rows (unlike v2)
  * <timezonelist> can be used independently for a picker without popup.
  * both <timezonepicker> and <timezonelist> inherit flex.
o The timezone list order is now in east to west order
  (in v2, was ordered by negative to positive utc offsets, which is
  west to east).
o Some timezones that were historically different but have now merged
  are commented out in the data file so they no longer appear in the
  timezone list.  For example, for the en-US data I commented out many
  Atlantic Standard Time island timezones leaving the largest,
  AST:America/Puerto Rico (I hope islanders aren't too offended).

Operation comments
o This timezonepicker seems good enough for occasional use, such as in the
  preference dialog.  Showing the timezone abbreviations helps.
o For frequent dialogs like the event dialog,
  * I agree with shaver that it would be better to have a shorter
    autocomplete-like popup with frequently/recently chosen timezones, 
    and leave the full list for an 'other...' or 'more...' button.  
  * It might be useful for localizers to initialize the
    recent/frequent timezone history depending on the locale,
    such as with US timezones for the en-US locale.
  * Have not yet implemented typeahead filtering/autocomplete
    (would like typing PST to show just the timezones containing PST,
    or at least scroll to the first one.)
o Speed
  * It is a little slow to load.
  * It is slower to popup than version 2, maybe partly due to more boxes
    to create and layout for the columns.
  * There's a tradeoff between preloading the data so it pops up
    quickly, or not-preloading the data so the page loads quickly.
    These tradeoffs could be explored further.
  * It would be nice if there was/is a way to share the preloaded
    data across pages so it only has to be loaded once rather
    than reloaded for each page.  Maybe it could be done with
    an application-wide shared XUL popup, though I haven't tried
    integrating that with an XBL component.

Code issues:
o Like in v2, requires some workarounds to get/set attributes.
  * For example, had to add 'selected' attribute in popupshown
    handler to highlight current item even though listbox already
    thinks item is selected.
  * Bug: the first selected item stays highlighted.
  * In fact, listitem.selected property implementation gets and 
    sets the value of the 'selected' attribute.  
http://lxr.mozilla.org/mozilla/source/toolkit/content/widgets/listbox.xml#715
    So it looks like it should be impossible for listitem.selected and
    listitem.getAttribute('selected') to be different, but in practice
    they can be, so setting an attribute directly rather than through
    a property can sometimes help get things working, as above.
  * (Seems like: Maybe underneath the DOM interfaces there are two
    representations of nodes and attributes, one before popup is 
    shown and one afterward, and not all 'user' modifications are
    transferred from one to the other before/after a popup.)

Bugs: 
o bug: first selected item stays highlighted even when another item
  is selected (as discussed above).
o bug: clicking on arrows on ends of scrollbar shouldn't clear the
  menulist input field.

Comment 9

[gekacheka]

*** Bug 318219 has been marked as a duplicate of this bug. ***

Comment 10

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

I took a very quick look at the patch, and noticed some things:
- You are reading the .properties file directly. Is there any reason not to use the string bundle service?
- You are talking to the console service. Why not Components.utils.reportError?
- You do a lot of calculations to get the timezone difference. Can we just hardcode those values, or do a parse once, startup? (maybe somewhere in the backend)

Comment 11

[gekacheka]

(In reply to comment #10)
> I took a very quick look at the patch, and noticed some things:
> - You are reading the .properties file directly. Is there any reason not to use
> the string bundle service?

Unlike normal users of properties files, this code needs to read all the values, and it needs both the 'keys' (timezone ids) and the 'values' (localized abbreviations and place names).  It doesn't need random access.  For this use pattern, it's not clear there is an advantage to using string bundles, but please point out advantages if you see some.

> - You are talking to the console service. Why not Components.utils.reportError?

reportError puts up a bug red stop sign.  This is just warning that some (probably obscure) timezone id wasn't recognized (maybe is no longer supported or changed spelling in olsen timezone database) and the locale needs updating.  It continues on with other timezones.  I should add a note to the message explaining that the timezone is skipped.

> - You do a lot of calculations to get the timezone difference. Can we just
> hardcode those values, or do a parse once, startup? (maybe somewhere in the
> backend)

The calculations display the offset in each timezone at a particular dateTime.  The offset will be different for dates of events at different times of year.

Thanks.

Comment 12

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

(In reply to comment #11)
> Unlike normal users of properties files, this code needs to read all the
> values, and it needs both the 'keys' (timezone ids) and the 'values' (localized
> abbreviations and place names). 

You can use nsIStringBundle.getSimpleEnumeration() to get all the properties. (although it's not clear from the idl what it actaully returns in the enumerator. You'll have to read the source for that.)

> For this use
> pattern, it's not clear there is an advantage to using string bundles, but
> please point out advantages if you see some.
>
The main advantage that I see is that you are not trying to write another parser for the file format. There is a well tested parser, so i would just use that. Less code duplication etc.
 
> The calculations display the offset in each timezone at a particular dateTime. 
> The offset will be different for dates of events at different times of year.

That still leaves to option to calculate it just once, in a service. But do we really need to show the offset? As a user, i don't really care about it. In fact, I don't know the offset that i'm in. I just know the city name. I dion't even know the name, so maybe we should hide that too. (But I live in  a country with just one timezone. Maybe if you live in a country with multiple timezone, you need to deal with them more often, and you do care about it the names)

Comment 13

[gekacheka]

Attachment: v4 timezonepicker: use tree (faster but blocked by bug 319062)

(patch -l -p 2 -i file.patch)

Changes:
o Implement timezonelist using xul tree rather than listbox to avoid 
  creating and laying out cell objects.
  * added an index from tz id to row to implement row selection
    when value is set (listbox tracks values and does this itself, 
    tree does not).
  * Had to add css files for column widths, tree without cell DOM 
    does not size columns to cell contents like the listbox did.
  * difficulty: tree selection is controlled by tree.view.selection, 
    but tree.view == null before tree is shown (in popup), so cannot
    preselect value in tree.  Instead, must reselect value when
    popup is shown, being careful not to trigger hidepopup immediately.
o Moved some string initializations from update back into the constructor.
o Took utcDateTime.jsDate.time() out of update loop since it doesn't change.
o Used <stringbundle> to load localized timezone place and abbreviations.
  (enables localizers to use localization tools, even if they save in \uNNNN
   encoding.)
o This version use Components.utils.reportError to warn about unrecognized
  timezones.  
  * Is there a corresponding way to create a warning rather than an error?
  * console.logStringMessage preserved newlines.  reportError turns
    newlines into spaces, so line breaks are lost.
o Other cleanup.


Profiling results:

  Using listbox, creating listrows of listcells:
    1.2 sec: page load takes about a second (below adds to ~1200ms)

      parse:  220 (in constructor)
      update: 942 (in update method)
	calc:   251 (recalculate offsets)
	sort:   120
	cell:   571 (make listcells)

    2.5 sec: popping up the listbox the first time takes about 2.5 seconds
      (2500ms ~ 10x time calculation time).

    Offset calculation looks insignificant.  Need to get rid of cells!
    Strategy: use a tree, so don't need to create/layout individual cells.

  Using tree backed by data model (nsITreeView):
  (and moved some string manipulation to parse, and improved the calc loop)

    <1 sec: page load is less than a second (below adds to ~600ms)

      parse:  270
      update: 320
	calc:   150 (330 1st time)
	sort:   160
	index:   10

    ~0.25 sec: popping up the tree the first time takes about 1/4 second.

  Tree (without cell DOM) is certainly much faster.

  Tree has a major bug when used in popups, however:
    clicking selects the wrong row (bug 319062).
    (workaround: click ~5 rows below what you want.)

If desired, maybe use v3 until bug 319062 is fixed.  
No typeahead/autocomplete yet.

Comment 14

[Majken Connor [:Kensie]]

For the less technically inclined, I don't suppose you could include screenshots of how the patches look if they look different? Not sure if the differences you're saying are only differences in code, or if they mean differences in the UI as well.

Comment 15

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

A much more straightforward way to make this faster would be by reducing the number of timezones. I mean, does europe really need 30 zones? a max of 5 would do just as well.

Comment 16

[Joey Minta]

What advantages does this provide over a graphical picker? (bug 302253) To me, a graphical picker seems like a good way to reduce the difficulty for the user to handle this many zones.  Or is this intended to be used in conjunction with that?

Comment 17

[Majken Connor [:Kensie]]

Well I suppose it would depend on the situation and the user, but I could see quite a few instances where a graphical picker would be impractical - either it'd be difficult to select the right region, or it'd need to be too large.  Then there are cases where people know what GMT offset they want, but aren't necessarily familiar with a map.

In any case, I personally think it's a good idea to get the current way of doing things working as well as possible until such a time that it may or may not be replaced.

I'd really need to see how exactly you'd implement all the time zones on the map to comment further, but I can definitely imagine cases where I'd rather just work with a dropdown, especially if I'm only adjusting by an hour or so.

Comment 18

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove