On Sat, Mar 19, 2011 at 10:56 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Looks like we used to only paint HH:MM part but...
> ... we now paint the whole line, which I personally think is a friendly
> move for color challenged people (me included---a larger span of text
> painted in different colors tends to help you still notice it better using
> value/brightness difference, even if your hue perception is weaker than
> other people). But it is a change from the old behaviour and might be
> worth stating in the log message.
For the $feature{'localtime'} disabled case, the coloring is the same as before.
I will paint the whole line in the next spin, and mention it in the
commit message.
>
>> - Â Â return $localtime;
>> + Â Â if ($use_localtime) {
>> + Â Â Â Â Â Â $timestamp .= $alt_time;
>> + Â Â }
>
> You kept -localtime nobody uses?
> While I don't agree with Jakub about the presense / absence of "[]" around
> it (that is not a part of "timestamp", but is how the caller wants to
> prepare the space to plug a timestamp string in), I do agree that the
> second parameter "use-localtime" looks funny, not because it is an unnamed
> parameter (which I personally think is fine) but because it isn't about
> the localness of the displayed time anymore. ÂYour 'localtime' feature
> alone controls in which timezone the timestamp is shown, and this only
> controls the use of 'atnight' highlighting. The parameter needs to be
> renamed, and perhaps you may clarify it further by making it a keyword
> argument as Jakub suggests.
$use_localtime indicates whether or not to add the " (hh:mm -zzzz)" at
the end. This also enables the atnight coloring.
This argument name was suggested in an earlier post and I guess I took
it a little too literally...
Do you think it would be a good idea to take two separate options:
-atnight for the variable coloring, and -alt_time (or some other name)
to show " (hh:mm -zzzz)" after the RFC 2822 string?
Or maybe take one option, named something like "-commitpage", to
indicate that it is a format specific to that view? If it is not
specified, the caller gets back an uncolored RFC 2822 date.
Also, is there a cleaner way of writing this?
sub timestamp_html {
my %date = %{$_[0]};
shift;
my %opts = @_;
Or should I pass in the options as a hash reference, more like $cgi->a():
sub timestamp_html {
my %date = %{$_[0]};
my %opts = %{$_[1]};
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html