[PATCH v2 3/5] ui-shared: introduce line range highlight javascript

[Andy Green]

On 06/23/2018 06:17 PM, John Keeping wrote:
> On Thu, Jun 21, 2018 at 05:34:59PM +0800, Andy Green wrote:
>> This adds a small css class, a clientside js function in
>> cgit.js, and ajs inline script caller in ui-shared
>> functions to interpret the # part of the URL
>> on the client, and apply a highlight to filtered source.
>>
>> Unlike blame highlight boxes which use generated divs, this
>> applied a computed absolute, transparent highlight over the
>> affected line(s) on the client side.
>>
>> The # part of the URL is defined to not be passed to the server,
>> so the highlight can't be rendered on the server side.
>> However this has the advantage that the line range highlight
>> can operate on /blame/ urls trivially, since it doesn't
>> conflict with blame's generated div scheme.
>>
>> pointer-events: none is used on the highlight overlay div to
>> allow the user to cut-and-paste in the highlit region and
>> click on links underneath normally.
>>
>> The JS supports highlighting single lines as before like #n123
>> and also ranges of lines like #n123-135.
>>
>> Because the browser can no longer automatically scroll to the
>> element in the second case, the JS also takes care of extracting
>> the range start element and scrolling to it dynamically.
>>
>> Tested on Linux Firefox 60 + Linux Chrome 67
>>
>> Examples:
>>
>> https://warmcat.com/git/cgit/tree/ui-shared.c#n1146
>> https://warmcat.com/git/cgit/tree/ui-shared.c#n1146-1164
>> https://warmcat.com/git/cgit/blame/ui-shared.c#n1146-1164

They better belong in a series cover letter.

> These examples can move below the "---" since they don't need to be part
> of the permanent Git history.
> 
>> Signed-off-by: Andy Green <andy at warmcat.com>
>> ---
>>   cgit.css    |    8 ++++++++
>>   cgit.js     |   43 +++++++++++++++++++++++++++++++++++++++++++
>>   ui-shared.c |   14 ++++++++++++++
>>   ui-shared.h |    1 +
>>   4 files changed, 66 insertions(+)
>>
>> diff --git a/cgit.css b/cgit.css
>> index 61bbd49..7cb0fd6 100644
>> --- a/cgit.css
>> +++ b/cgit.css
>> @@ -368,6 +368,14 @@ div#cgit table.blame td.lines > div > pre {
>>   	top: 0;
>>   }
>>   
>> +div#cgit div.line_range {
> 
> It looks like the normal convention for CSS classes is kebab-case rather
> than snake_case.
> 
> I also wonder if the name should be "highlighed-lines" or
> "selected-lines" or something like that to indicate that this isn't just
> an arbitrary range of lines but ones that are being pulled out for some
> reason.

OK, "selected-lines" then.

>> diff --git a/ui-shared.c b/ui-shared.c
>> index 082a6f1..7fd6bad 100644
>> --- a/ui-shared.c
>> +++ b/ui-shared.c
>> @@ -787,6 +787,10 @@ void cgit_print_docstart(void)
>>   	html("<link rel='stylesheet' type='text/css' href='");
>>   	html_attr(ctx.cfg.css);
>>   	html("'/>\n");
>> +	html("<script type='text/javascript' src='");
>> +	html_attr(ctx.cfg.js);
>> +	html("'/></script>\n");
> 
> This belongs in the previous patch which adds the .js file support.

OK, they got squashed as requested as well.

>>   	if (ctx.cfg.favicon) {
>>   		html("<link rel='shortcut icon' href='");
>>   		html_attr(ctx.cfg.favicon);
>> @@ -1237,3 +1241,13 @@ void cgit_set_title_from_path(const char *path)
>>   	strcat(new_title, ctx.page.title);
>>   	ctx.page.title = new_title;
>>   }
>> +
>> +void cgit_emit_line_range_highlight_script(int parent_level)
>> +{
>> +	htmlf("<script>\n"
>> +	      "document.addEventListener(\"DOMContentLoaded\", function() {"
>> +	      "	cgit_line_range_highlight(%d);\n"
>> +	      "}, false);\n"
>> +	      "</script>\n", parent_level);
> 
> Can we do this unconditionally in the .js file?

Yes... but it's complicated by having to rediscover if we are in blame 
or tree only by looking at the DOM (since we don't know our server 
virtual path in the client, that's not simple to reliably extract from 
the URL), rather than in the C having complete and reliable knowledge of 
if we are generating blame or tree.  But a simple heuristic works for 
that in the JS by looking at the DOM atm.

> Since it handles the case where the target line element isn't found, I
> don't think it will cause problems to run this on every page.

I changed it to do so.

-Andy