Re: [PATCH 00/11] gitweb: Change timezone

From: Kevin Cernekee <cernekee@gmail.com>
Date: Apr 12, 2011, 01:19 UTC
Message-ID: <BANLkTik9D6++P1rg+VbvKj9oTFhRYNmrpQ@mail.gmail.com>
In-Reply-To: <1302389366-21515-1-git-send-email-jnareb@gmail.com>

On Sat, Apr 9, 2011 at 3:49 PM, Jakub Narebski <jnareb@gmail.com> wrote:

> This is split version (with assorted cleanups) of J.H. patch adding
> JavaScript-base ability to change timezone in which dates are
> displayed.

Jakub,

Thanks for the update. This UI does seem to work better than the original "[+]" dropdown interface.

The new code appears to degrade gracefully on IE6 (everything shown in UTC).

Tests on Firefox 3.6.15 looked OK.

Chromium 6.0.472.62 (59676) does not like this operation:

Uncaught Error: NOT_FOUND_ERR: DOM Exception 8
  removeChangeTZForm
  /gitweb-static/gitweb.js:785
onTZFormChange

line 785: var removed = container.removeChild(container.lastChild);

Opera 10.63 resets target.selectedIndex to 1 after calling removeChangeTZForm() from onTZFormChange(). Net effect is that you're always stuck in the "local" zone. Here is a workaround that fixed it for me:

       var target = event.target || event.srcElement;
       var selectedIndex = target.selectedIndex;

       removeChangeTZForm(documentFragment, target, tzClassName);

       var selected = target.options.item(selectedIndex);

Couple other random nitpicks:

> +	// server-side of gitweb produces datetime in UTC,
> +	// so if tz is 'utc' there is no need for changes
> +	var nochange = tz === 'utc';

If I delete my gitweb_tz cookie, then refresh the page:

 - All times show up in my browser's local time (good)

 - Clicking UTC/GMT on the dropdown has no immediate effect; I need to
   refresh the page again to see the times change (bad)

> +	my (undef, undef, $datetime_class) =
> +		gitweb_get_feature('javascript-timezone');
> +	if ($datetime_class) {
> +		$strtime = qq!<span class="datetime">$strtime</span>!;

Should this hard-code class "datetime", or use $datetime_class from the gitweb server configuration?

> +/* JavaScript-base timezone manipulation */

Might want to reword as "JavaScript-based"

> +		// refresh cookie, so it expires 7 days from last use of gitweb
> +		setCookie(tzCookieName, tzCookie, { expires: 7, path: '/' });

Hmm, only 7 days?

> + * and instals appropriate onclick trigger (via event delegation).

"installs"