Re: [PATCH 4/5] gitweb: Selecting diffs in JavaScript

From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Aug 26, 2007, 16:17 UTC
Message-ID: <20070826161701.GA3300@auto.tuwien.ac.at>
In-Reply-To: <20070826011742.GF1219@pasky.or.cz>

On Sun, Aug 26, 2007 at 03:17:43AM +0200, Petr Baudis wrote:

Show 8 quoted lines

> On Sun, May 20, 2007 at 10:23:31PM CEST, Martin Koegler wrote:
> > The adds support for selecting arbitrary diffs, if the client browser supports
> > JavaScript.
> > 
> > Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
> 
> JFYI, I've just enabled it on repo.or.cz experimentally. Please anyone
> tell me what you think.

I would also be interessed in this.

Show 5 quoted lines

> (Just click at almost any view in gitweb and marvel at the extra base
> and diff links smiling everywhere.)
>
> I'll dump my mind state here, horribly sleepy. Sorry for the
> incomprehensible rant. :-)

I can't see them.

http://repo.or.cz/w/git/repo.git?a=search&h=HEAD&st=grep&s=GitAddLinks reveals, that the function GitAddLinks is never called. You should add a call to GitAddLinks in fixBlameLinks or call both functions in <body onload="...">.

> One thing for certain is that I would get rid of GitAddLinks and instead
> have href() add the extra links there, by default display: none and
> javascript code making it show.

display: none will only work in CSS capable browses, but not text browsers. Also why do we want do increase the size of each generated page?

> Also, there are obvious UI bugs, like
> commit and tree entries for commits having redundant base/diff links;

Is this really a bug? I think it makes the interface more consistent.

Would it be clear for a (new) user, why some tree entries have the base/diff links and some not?

> and Firefox doesn't expand table cells properly for the additional
> links. None of these bugs is easily fixable I think.

In which context?

In eg. http://repo.or.cz/w/git.git?a=log;h=gitgui-0.8.1, there are display errors (even without the base/diff links), if the font size is zommed. In this case, the problem is that there is no real table. The links (div.log_link) are position: relative, so they don't increate the height of the enclosing div. Additionall it has an hardcoded width in pixels.

Show 5 quoted lines

> But maybe it needs more general overhaul and the links added explicitly
> in the views, because it really makes sense to have only a single
> linkpair per entry and it would be good to have this always at the same
> place, and perhaps in a different color. Hmm. And it seems that it's
> getting in the way, overally.

Unless this should be come part of the official gitweb, adding each link explicitly would be a maintaince nightmare.

Changing the color of the base/diff links is no problem. If somebody is interessed in this (and tells me, which colors I should use), I can adapt my patch.

Show 5 quoted lines

> What about having another item in the main action menu, 'diff'?
> Base/diff links are by default display: none but show up when you click
> at 'diff', in green. They will keep showing up until you click 'diff'
> again. When you click 'base', both the selected base link and main
> 'diff' link changes color to red.

We could call GitAddLinks, if the user clicks on the diff link instead of calling GitAddLinks at page load time.

I could rewrite the JavaScript for this, if you think it is so more useful.

mfg Martin Kögler