On 22-09-15 03:35PM, Stefan Sperling wrote:
> On Thu, Sep 15, 2022 at 11:03:48PM +1000, Mark Jamsek wrote:
> > I tried a few different things, most notably scroll bars and stsp's
> > suggestion. The scroll bars were interesting but ultimately look out of
> > place in a TUI app, imo. So I settled on a highlighted comment and
> > percentage indicator: "scroll down for more...[mn%]"
> > 
> > I think this both provides a good cue to the user and fits the general
> > tog aesthetic. If you'd like to see the scroll bar version, let me know
> > and I'll shoot through the diff.
> > 
> > As previously mentioned, I'd like to add backwards search via the '?'
> > key map to tog, so I've only documented H and F1 but runtime help is
> > still accessible via ? for now.
> 
> ok stsp@
> 
> This is a great start. I have some comments below, which can be dealt
> with later if/when you feel like it.

Thanks, committed! And replies inline.

> >  static const struct got_error *
> > -search_next_diff_view(struct tog_view *view)
> > +search_set_view(struct tog_view *view, FILE **f, off_t **line_offsets,
> > +    size_t *nlines, int **first, int **last, int **match, int **selected)
> >  {
> > -	struct tog_diff_view_state *s = &view->state.diff;
> > +	*f = NULL;
> > +	*first = *last = *match = *selected = NULL;
> > +	*line_offsets = NULL;
> > +
> > +	switch (view->type) {
> > +	case (TOG_VIEW_DIFF): {
> > +		struct tog_diff_view_state *s = &view->state.diff;
> > +
> > +		*f = s->f;
> > +		*nlines = s->nlines;
> > +		*match = &s->matched_line;
> > +		*first = &s->first_displayed_line;
> > +		*last = &s->last_displayed_line;
> > +		*selected = &s->selected_line;
> > +		break;
> > +	}
> > +	case (TOG_VIEW_BLAME): {
> > +		struct tog_blame_view_state *s = &view->state.blame;
> > +
> > +		*f = s->blame.f;
> > +		*nlines = s->blame.nlines;
> > +		*line_offsets = s->blame.line_offsets;
> > +		*match = &s->matched_line;
> > +		*first = &s->first_displayed_line;
> > +		*last = &s->last_displayed_line;
> > +		*selected = &s->selected_line;
> > +		break;
> > +	}
> > +	case (TOG_VIEW_HELP): {
> > +		struct tog_help_view_state *s = &view->state.help;
> > +
> > +		*f = s->f;
> > +		*nlines = s->nlines;
> > +		*line_offsets = s->line_offsets;
> > +		*match = &s->matched_line;
> > +		*first = &s->first_displayed_line;
> > +		*last = &s->last_displayed_line;
> > +		*selected = &s->selected_line;
> > +		break;
> > +	}
> > +	default:
> > +		return got_error_msg(GOT_ERR_NOT_IMPL,
> > +		    "view search not supported");
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> The above function could also be modelled as a new function pointer in
> struct tog_view, so the different cases above could be called individually.
> This is matter of taste, though, and just cosmetic.

Good idea. This is a better solution I think as it's more consistent
with the current code. I'll do this; thanks!

> > +/*
> > + * Write keymap section headers, keys, and key info in km to f.
> > + * Save line offset to *off. If terminal has UTF8 encoding enabled,
> > + * wrap control and symbolic keys in guillemets, else use <>.
> > + * For example (top=UTF8, bottom=ASCII):
> > + * Global
> > + *   k ❬C-p❭ ❬Up❭               Move cursor or page up one line
> > + * Global
> > + *   k <C-p> <Up>               Move cursor or page up one line
> > + */
> > +static const struct got_error *
> > +format_help_line(off_t *off, FILE *f, const struct tog_key_map *km, int width)
> > +{
> > +	int n, len = width;
> > +
> > +	if (km->keys) {
> > +		char	*t0, *t, *k;
> > +		int	 cs, s, first = 1;
> > +
> > +		cs = got_locale_is_utf8();
> > +
> > +		t = t0 = strdup(km->keys);
> > +		if (t0 == NULL)
> > +			return got_error_from_errno("strdup");
> > +
> > +		len = strlen(km->keys);
> > +		while ((k = strsep(&t, " ")) != NULL) {
> > +			s = strlen(k) > 1;  /* control or symbolic key */
> > +			n = fprintf(f, "%s%s%s%s%s", first ? "  " : "",
> > +			    cs && s ? "❬" : s ? "<" : "", k,
> > +			    cs && s ? "❭" : s ? ">" : "", t ? " " : "");
> 
> The above embeds UTF-8 characters into the source code. This means tog.c
> can only be viewed and edited with an editor that is aware of UTF-8, and
> there are people among OpenBSD developers who run in the C locale.
> So making UTF-8 a requirement is not ideal.
> 
> For example, tmux deals with this by using lookup-tables such as the
> one below:
> 
> (from usr.bin/tmux/tty-acs.c)
> /* UTF-8 heavy borders. */
> static const struct utf8_data tty_acs_heavy_borders_list[] = {
> 	{ "", 0, 0, 0 },
> 	{ "\342\224\203", 0, 3, 1 }, /* U+2503 */
> 	{ "\342\224\201", 0, 3, 1 }, /* U+2501 */
> 	{ "\342\224\217", 0, 3, 1 }, /* U+250F */
> 	{ "\342\224\223", 0, 3, 1 }, /* U+2513 */
> 	{ "\342\224\227", 0, 3, 1 }, /* U+2517 */
> 	{ "\342\224\233", 0, 3, 1 }, /* U+251B */
> 	{ "\342\224\263", 0, 3, 1 }, /* U+2533 */
> 	{ "\342\224\273", 0, 3, 1 }, /* U+253B */
> 	{ "\342\224\243", 0, 3, 1 }, /* U+2523 */
> 	{ "\342\224\253", 0, 3, 1 }, /* U+252B */
> 	{ "\342\225\213", 0, 3, 1 }, /* U+254B */
> 	{ "\302\267",	  0, 2, 1 }  /* U+00B7 */
> };
> 
> We should probably be using a similar approach to render "❬" and "❭".

I thought of this problem when I first started this the other weekend
and meant to loopback to a solution so thanks for providing this hint.
I'll add a fix for this too.

> > @@ -8376,6 +8985,14 @@ view_dispatch_request(struct tog_view **new_view, stru
> >  		if (err)
> >  			view_close(*new_view);
> >  		break;
> > +	case TOG_VIEW_HELP:
> > +		*new_view = view_open(0, 0, y, x, TOG_VIEW_HELP);
> > +		if (*new_view == NULL)
> > +			return got_error_from_errno("view_open");
> > +		err = open_help_view(*new_view, view);
> > +		if (err)
> > +			view_close(*new_view);
> > +		break;
> 
> Because the help view is implemented as a normal view, tog's parent/child
> view behaviour determines where a new help view appears. For example,
> when in horizontal split mode, the help view appears either full-screen
> or in a split-screen, depending on which view has focus when the help view
> is opened. I would expect help text to always appear in the same frame as
> the view which is being documented. This would likely require some further
> refactoring of the way new views are opened?

Yes, we'll need to tweak the new view code a bit, but I don't think this
will be difficult.

That said, this is the only thing I'm a little unsure about tbh. The
original implementation did something like this: a pad popped up,
partially obscuring the current view, but no other views could be
accessed until the help pad was closed. One of the secondary reasons
I dropped this approach was because I thought it would be useful to have
the help reference visible while browsing the primary view. But the
current implementation doesn't fully cater to this either because, as
you allude to, it adheres to the current view rules such that the help
view only opens as a child view if opened from a parent, but when opened
from a child, it becomes a parent and blocks all previous views.

I'll cook a diff implementing your suggested change and shoot it through
so we can get a feel for what we prefer. As I typed this and thought
about it more, I think this makes the most sense anyway. Thanks, Stefan!

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