On 23-01-20 09:25PM, Mikhail wrote:
> On Fri, Jan 20, 2023 at 09:09:07PM +0300, Mikhail wrote:
> > Inlined patch adds short timed prompts to status bar about things like
> > switching between diff algorithms, showing white spaces in patches,
> > toggling bin/ascii views for blobs, changing committer and author
> > representations, and switch between date/name sorting for ref view.
> > 
> > Originally I wanted it because I needed to see current diff algorithm,
> > but decided to expand the patch to show all togglable events, except for
> > ref view, where, for example, showing IDs speaks for itself by changing
> > the look of the table and doesn't require a prompt.

I like the idea of this; however, where the information is currently
echoed feels a little incongruous. And, perhaps more importantly,
depending on the user's terminal width and current view, the information
may not even be visible.

I'm also not sure about displaying some of these actions. You're right
about the display of reference IDs being obvious. Likewise for toggling
the reference's last modified date, and showing tree entry IDs too. The
algorithm definitely makes sense as it's not easily, if at all,
discernable.  Likewise for ignoring whitespace. However, toggling
between author and committer can be surmised already; granted, you need
to enter the diff view to make this distinction. Similarly, forcing
ASCII text is also apparent from the diff; in a similar vein, the code
doesn't report adjusting context lines as the diff speaks for itself.
Lastly, the reference sort order is already evident, especially if the
'm' keymap is toggled but even if not, lexicographic order is obvious in
most cases so it's simple to tell when sorted by name or by date.

That said, I really like the general idea, but it might be nice to see
such action updates echoed to the bottom left of screen where we display
prefixed count mod key input.  This way, it will always be visible
irrespective of terminal width and view, which is important. It is also
more consistent behaviour as this is where we already echo responses to
user input, and, imo, won't feel out of place like it does appended to
the end of the headline.

The below diff demonstrates this change in placement. The view struct
char pointer 'prompt' is also renamed to 'action' as this seems more
appropriate; prompt implies we're requesting input from the user, but
the report describes action taken in response to user input. And
the wording of some action reports has been changed to be more
descriptive too.

There's also a related fix that should probably go in regardless of
whether this action report lands: in view_loop() we initialise at
a refresh rate of one tenth per second before reducing this to one
second updates. However, the reduction was never hit due to a typo in
the conditional. The following hunk fixes this:

@@ -1739,7 +1778,7 @@ view_loop(struct tog_view *view)
 	while (!TAILQ_EMPTY(&views) && !done && !tog_thread_error &&
 	    !tog_fatal_signal_received()) {
 		/* Refresh fast during initialization, then become slower. */
-		if (fast_refresh && fast_refresh-- == 0)
+		if (fast_refresh && --fast_refresh == 0)
 			halfdelay(10); /* switch to once per second */
 
 		err = view_input(&new_view, &done, view, &views);

Complete diff:

diffstat /home/mark/src/got
 M  tog/tog.c  |  55+  5-

1 file changed, 55 insertions(+), 5 deletions(-)

diff /home/mark/src/got
commit - 7713cc5e4f5544e81909670d592e89526ed86c9b
path + /home/mark/src/got
blob - 35282ce50aa4dec0745aa6402cd0b50d87b7416b
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -690,6 +690,7 @@ struct tog_view {
 #define TOG_SEARCH_HAVE_NONE	3
 	regex_t regex;
 	regmatch_t regmatch;
+	const char *action;
 };
 
 static const struct got_error *open_diff_view(struct tog_view *,
@@ -1460,6 +1461,35 @@ view_input(struct tog_view **new, int *done, struct to
 }
 
 static const struct got_error *
+action_report(struct tog_view *view)
+{
+	struct tog_view *v = view;
+
+	if (view_is_hsplit_top(view))
+		v = view->child;
+	else if (view->mode == TOG_VIEW_SPLIT_VERT && view->parent)
+		v = view->parent;
+
+	wmove(v->window, v->nlines - 1, 0);
+	wclrtoeol(v->window);
+	if (mvwprintw(v->window, v->nlines - 1, 0, ":%s", view->action) == ERR)
+		return got_error_msg(GOT_ERR_RANGE, "mvwprintw");
+	wrefresh(v->window);
+
+	/*
+	 * Clear action status report. Only clear in blame view
+	 * once annotating is complete, otherwise it's too fast.
+	 */
+	if (view->type == TOG_VIEW_BLAME) {
+		if (view->state.blame.blame_complete)
+			view->action = NULL;
+	} else
+		view->action = NULL;
+
+	return NULL;
+}
+
+static const struct got_error *
 view_input(struct tog_view **new, int *done, struct tog_view *view,
     struct tog_view_list_head *views)
 {
@@ -1469,6 +1499,12 @@ view_input(struct tog_view **new, int *done, struct to
 
 	*new = NULL;
 
+	if (view->action) {
+		err = action_report(view);
+		if (err)
+			return err;
+	}
+
 	/* Clear "no matches" indicator. */
 	if (view->search_next_done == TOG_SEARCH_NO_MORE ||
 	    view->search_next_done == TOG_SEARCH_HAVE_NONE) {
@@ -1668,10 +1704,13 @@ view_input(struct tog_view **new, int *done, struct to
 			err = view->input(new, view, ch);
 		break;
 	case 'A':
-		if (tog_diff_algo == GOT_DIFF_ALGORITHM_MYERS)
+		if (tog_diff_algo == GOT_DIFF_ALGORITHM_MYERS) {
 			tog_diff_algo = GOT_DIFF_ALGORITHM_PATIENCE;
-		else
+			view->action = "Patience algorithm enabled";
+		} else {
 			tog_diff_algo = GOT_DIFF_ALGORITHM_MYERS;
+			view->action = "Myers algorithm enabled";
+		}
 		TAILQ_FOREACH(v, views, entry) {
 			if (v->reset) {
 				err = v->reset(v);
@@ -1739,7 +1778,7 @@ view_loop(struct tog_view *view)
 	while (!TAILQ_EMPTY(&views) && !done && !tog_thread_error &&
 	    !tog_fatal_signal_received()) {
 		/* Refresh fast during initialization, then become slower. */
-		if (fast_refresh && fast_refresh-- == 0)
+		if (fast_refresh && --fast_refresh == 0)
 			halfdelay(10); /* switch to once per second */
 
 		err = view_input(&new_view, &done, view, &views);
@@ -3718,6 +3757,8 @@ input_log_view(struct tog_view **new_view, struct tog_
 		break;
 	case '@':
 		s->use_committer = !s->use_committer;
+		view->action = s->use_committer ?
+		    "show committer enabled" : "show author enabled";
 		break;
 	case 'G':
 	case '*':
@@ -5261,10 +5302,18 @@ input_diff_view(struct tog_view **new_view, struct tog
 		break;
 	case 'a':
 	case 'w':
-		if (ch == 'a')
+		if (ch == 'a') {
 			s->force_text_diff = !s->force_text_diff;
-		else if (ch == 'w')
+			view->action = s->force_text_diff ?
+			    "force ASCII text enabled" :
+			    "force ASCII text disabled";
+		}
+		else if (ch == 'w') {
 			s->ignore_whitespace = !s->ignore_whitespace;
+			view->action = s->ignore_whitespace ?
+			    "ignore whitespace enabled" :
+			    "ignore whitespace disabled";
+		}
 		err = reset_diff_view(view);
 		break;
 	case 'g':
@@ -8233,6 +8282,7 @@ input_ref_view(struct tog_view **new_view, struct tog_
 		break;
 	case 'o':
 		s->sort_by_date = !s->sort_by_date;
+		view->action = s->sort_by_date ? "sort by date" : "sort by name";
 		view->count = 0;
 		err = got_reflist_sort(&tog_refs, s->sort_by_date ?
 		    got_ref_cmp_by_commit_timestamp_descending :

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