From: Mark Jamsek <mark@jamsek.com>
Subject: Re: tog: extend log view headline highlight
To: Game of Trees <gameoftrees@openbsd.org>
Date: Sun, 11 Sep 2022 14:20:44 +1000

On 22-09-09 04:12PM, Stefan Sperling wrote:
> On Fri, Sep 09, 2022 at 09:45:11PM +1000, Mark Jamsek wrote:
> > On 22-09-09 09:23AM, Stefan Sperling wrote:
> > > On Fri, Sep 09, 2022 at 08:23:48AM +0200, Stefan Sperling wrote:
> > > > What do you think about not highlighting a cursor when its view is not
> > > > in focus? I believe that would give a better visual impression.
> > > 
> > > Something like this (on top of your diff)?
> > > 
> > > It seems the ref and tree views already do this. So my patch only tweaks
> > > the log and blame views. And the diff view doesn't have a selection cursor.
> > 
> > My main concern with this being applied to the log and blame views is
> > that we provide the ability to scroll both these views from the child
> > diff view with the JK|,.|<> key maps. By removing the highlight, we no
> > longer provide a primary visual reference for which entry is selected in
> > the parent log/blame view. I think this might be a greater loss than the
> > benefit gained through minimising confusion as to which view is in
> > focus. Do you think this is a valid concern?
> > 
> > Further, without this visual cue, it looks a bit strange when the entry
> > selected in the log/blame view is the last or first entry at the bottom
> > or top of the view, and the log/blame view starts scrolling as you
> > select the next entry off-screen with JK from the child diff view.
> > 
> > I'm undecided as to whether there's a net gain from this change when
> > these factors are taken into account. Perhaps there's another way we can
> > minimise the confusion and accentuate the focussed view without removing
> > this visual cue in the log and blame views?
> 
> Would making the A_BOLD attribute make sense? Try this:

I think this works ok in terminals with dark schemes. It's arguably not
as good in terminals with light ones, but it does provide some visual
cue. tbh, I prefer the current behaviour but I'm more than happy to
defer to your judgement on this--with the caveat that naddy has no
objections because, IIRC, naddy's not a fan of attributes beyond
A_STANDOUT (though I could be misremembering).

> diff b21ed2d92fa9a52695cee221ef20b75b1a182884 refs/heads/tog-highlight
> commit - b21ed2d92fa9a52695cee221ef20b75b1a182884
> commit + c9f38fbe5854f0986bfe240a76c72ca0e66b058b
> blob - 81a72cd907757863416d7c2e4b5924f8413371a0
> blob + 7e40ce812d03297a7363f682c0a18740fef3d011
> --- tog/tog.c
> +++ tog/tog.c
> @@ -2396,12 +2396,20 @@ draw_commits(struct tog_view *view)
>  	while (entry) {
>  		if (ncommits >= limit - 1)
>  			break;
> -		if (ncommits == s->selected)
> -			wstandout(view->window);
> +		if (ncommits == s->selected) {
> +			if (view->focussed)
> +				wstandout(view->window);
> +			else
> +				wattron(view->window, A_BOLD);
> +		}
>  		err = draw_commit(view, entry->commit, entry->id,
>  		    date_display_cols, author_cols);
> -		if (ncommits == s->selected)
> -			wstandend(view->window);
> +		if (ncommits == s->selected) {
> +			if (view->focussed)
> +				wstandend(view->window);
> +			else
> +				wattroff(view->window, A_BOLD);
> +		}
>  		if (err)
>  			goto done;
>  		ncommits++;
> @@ -5271,14 +5279,18 @@ draw_blame(struct tog_view *view)
>  		wline = NULL;
>  		view->maxx = MAX(view->maxx, width);
>  
> -		if (nprinted == s->selected_line - 1)
> -			wstandout(view->window);
> +		if (nprinted == s->selected_line - 1) {
> +			if (view->focussed)
> +				wstandout(view->window);
> +			else
> +				wattron(view->window, A_BOLD);
> +		}
>  
>  		if (blame->nlines > 0) {
>  			blame_line = &blame->lines[lineno - 1];
>  			if (blame_line->annotated && prev_id &&
>  			    got_object_id_cmp(prev_id, blame_line->id) == 0 &&
> -			    !(nprinted == s->selected_line - 1)) {
> +			    nprinted != s->selected_line - 1) {
>  				waddstr(view->window, "        ");
>  			} else if (blame_line->annotated) {
>  				char *id_str;
> @@ -5307,8 +5319,12 @@ draw_blame(struct tog_view *view)
>  			prev_id = NULL;
>  		}
>  
> -		if (nprinted == s->selected_line - 1)
> -			wstandend(view->window);
> +		if (nprinted == s->selected_line - 1) {
> +			if (view->focussed)
> +				wstandend(view->window);
> +			else
> +				wattroff(view->window, A_BOLD);
> +		}
>  		waddstr(view->window, " ");
>  
>  		if (view->ncols <= 9) {
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68