Re: [PATCHv5 06/12] gitweb: allow extra text after action in page header
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Sep 27, 2010, 06:56 UTC
- Message-ID
- <AANLkTinpZ5gusCG4SoKbi54URyn7T4THNp2NeVM6uFL2@mail.gmail.com>
- In-Reply-To
- <201009262011.28211.jnareb@gmail.com>
2010/9/26 Jakub Narebski <jnareb@gmail.com>:
Show 12 quoted lines
> On Fri, 24 Sep 2010, Giuseppe Bilotta wrote:
>
>> This extra text is intended to 'specify' the action. Therefore, if it's
>> present, the action name in the header will be turned into a link to
>> the action itself but without specifying any parameter.
>
> What this feature is intended *for*? I guess it is meant to be used
> in the case where there is additional parameter that specifies action,
> like e.g. a single remote view, where $action is 'remotes', but there
> is extra parameter ('hash_base' is (ab)used for this) that specifies
> remote. Then we want to make 'remotes' in "breadcrumbs" navigation at
> top of page to link to generic 'remotes' view. Isn't it?Yes, this is exactly the intended purpose of this feature. It is of course possible to extend other views to have a similar behavior (I'm thinking in particular about log views here).
> But the above is just my guess, covering only case where there is both > $action defined, and 'header_extra' option set. > > You need to explain this in the commit message.
I will clarify this in the next rehash of the patch.
Show 34 quoted lines
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>> gitweb/gitweb.perl | 10 +++++++++-
>> 1 files changed, 9 insertions(+), 1 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index e70897e..76cf806 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -3514,7 +3514,15 @@ EOF
>> if (defined $project) {
>> print $cgi->a({-href => href(action=>"summary")}, esc_html($project));
>> if (defined $action) {
>> - print " / $action";
>> + my $action_print = $action ;
>> + if (defined $opts{'header_extra'}) {
>
> We spell optional named parameter with '-' as prefix, for example
> $opts{'-remove_signoff'} in git_print_log(), to be able to use key
> without quoting (bareword-like, autoquoting), like $opts{-nohtml}
> or $opts{-pad_before}, or $opts{-size}, or $opts{-tag}... though
> gitweb is not very consisnte here.
>
> So it should probably be
>
> + if (defined $opts{'-header_extra'}) {
>
> or even
>
> + if (defined $opts{-header_extra}) {
>
>
> I also think that we can think of better name for this option than
> 'header_extra', although what this name could be eludes me.I will add the dash to the option. Naming it header_extra keeps the meaning of this extra text generic, but considering that the intended use is mostly for the single-remote view (or similar, if/when they are added) we could call it something related (I can only think of 'main_argument' right now but I think this would suck more than header_extra).
Show 7 quoted lines
>> + $action_print = $cgi->a({-href => href(action=>$action)},
>> + esc_html($action));
>
> I don't think we need to run esc_html on action: it is checked that it
> is one of specified set of possible values, and it can be ensured that
> it neither contains anything than printable characters, not any HTML
> special characters.Ok, I will remove it.
Show 7 quoted lines
>> + }
>> + print " / $action_print";
>> + }
>> + if (defined $opts{'header_extra'}) {
>> + print " / $opts{'header_extra'}";
>
> Hmmm...You don't sound very convinced. I had some doubts myself about whether the slash should be inserted autmatically or whether it should be up to the caller to include it in header_extra, but I'm not sure this is what you are perplexed about.
-- Giuseppe "Oblomov" Bilotta