[TIG][PATCH 0/3] Refactoring of the log view

[TIG][PATCH 0/3] Refactoring of the log view

[Kumar Appaiah]

Hi.

These set of patches refactor the log view to provide a behaviour that
is quite similar to, say, e-mail with Mutt. The key improvements are:

- The current commit is inferred based on the context. For example, if
  you focus on the commit message of a particular commit, the correct
  commit is inferred automagically.

- Scrolling the log view when the diff is open shows the correct
  commit on the screen, rather than have to scroll up and cross the
  commit line to display the screen.

I have decided to revert 888611dd5d407775245d574a3dc5c01b5963a5ba,
since the behaviour with the updated scrolling pattern is much more
consistent.

As always, I will gladly alter the patch based on comments on coding
style and all other aspects.

Thanks!

Kumar

Kumar Appaiah (3):
  Add log_select function to find commit from context in log view
  Display correct diff the context in split log view
  Revert "Scroll diff with arrow keys in log view"

 NEWS  |  1 +
 tig.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 5 deletions(-)

-- 
1.8.3.2

[[TIG][PATCH] 1/3] Add log_select function to find commit from context in log view

[Kumar Appaiah]

This commit introduces and uses the log_select function to find the
correct commit in the unsplit log view. In the log view, if one
scrolls down across a commit line, the current commit (as displayed in
the status bar) gets updated, but not so when scrolling upward across
a commit. The log_select function handles this scenario to to the
``right thing''. In addition, it introduces the log_state structure as
the private entry of the log view to hold a flag that decides whether
to re-evaluate the current commit based on scrolling.

Signed-off-by: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>
---
 tig.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/tig.c b/tig.c
index 72f132a..dd4b0f4 100644
--- a/tig.c
+++ b/tig.c
@@ -4384,6 +4384,33 @@ pager_select(struct view *view, struct line *line)
 	}
 }
 
+struct log_state {
+	bool update_commit_ref;
+};
+
+static void
+log_select(struct view *view, struct line *line)
+{
+	struct log_state *state = (struct log_state *) view->private;
+
+	if (state->update_commit_ref && line->lineno > 1) {
+		/* We need to recalculate the previous commit,
+		   since the user has likely scrolled up. */
+		const struct line *commit_line = find_prev_line_by_type(view, line, LINE_COMMIT);
+
+		if (commit_line)
+			string_copy_rev(view->ref, (char *) (commit_line->data + STRING_SIZE("commit ")));
+	}
+	if (line->type == LINE_COMMIT) {
+		char *text = (char *)line->data + STRING_SIZE("commit ");
+
+		if (!view_has_flags(view, VIEW_NO_REF))
+			string_copy_rev(view->ref, text);
+	}
+	string_copy_rev(ref_commit, view->ref);
+	state->update_commit_ref = FALSE;
+}
+
 static bool
 pager_open(struct view *view, enum open_flags flags)
 {
@@ -4427,11 +4454,30 @@ log_open(struct view *view, enum open_flags flags)
 static enum request
 log_request(struct view *view, enum request request, struct line *line)
 {
+	struct log_state *state = (struct log_state *) view->private;
+
 	switch (request) {
 	case REQ_REFRESH:
 		load_refs();
 		refresh_view(view);
 		return REQ_NONE;
+
+	case REQ_MOVE_UP:
+	case REQ_PREVIOUS:
+		if (line->type == LINE_COMMIT && line->lineno > 1) {
+			/* We are at a commit, and heading upward. We
+			   force log_select to find the previous
+			   commit above, from the context. */
+			state->update_commit_ref = TRUE;
+		}
+		return pager_request(view, request, line);
+
+	case REQ_MOVE_PAGE_UP:
+	case REQ_MOVE_PAGE_DOWN:
+		/* We need to figure out the right commit again. */
+		state->update_commit_ref = TRUE;
+		return pager_request(view, request, line);
+
 	default:
 		return pager_request(view, request, line);
 	}
@@ -4441,13 +4487,13 @@ static struct view_ops log_ops = {
 	"line",
 	{ "log" },
 	VIEW_ADD_PAGER_REFS | VIEW_OPEN_DIFF | VIEW_SEND_CHILD_ENTER | VIEW_NO_PARENT_NAV,
-	0,
+	sizeof(struct log_state),
 	log_open,
 	pager_read,
 	pager_draw,
 	log_request,
 	pager_grep,
-	pager_select,
+	log_select,
 };
 
 struct diff_state {
-- 
1.8.3.2

Re: [[TIG][PATCH] 1/3] Add log_select function to find commit from context in log view

[Jonas Fonseca]

On Fri, Aug 2, 2013 at 8:23 PM, Kumar Appaiah <a.kumar@alumni.iitm.ac.in> wrote:
> This commit introduces and uses the log_select function to find the
> correct commit in the unsplit log view. In the log view, if one
> scrolls down across a commit line, the current commit (as displayed in
> the status bar) gets updated, but not so when scrolling upward across
> a commit. The log_select function handles this scenario to to the

s/to to/to do/

> ``right thing''. In addition, it introduces the log_state structure as
> the private entry of the log view to hold a flag that decides whether
> to re-evaluate the current commit based on scrolling.
>
> Signed-off-by: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>
> ---
>  tig.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/tig.c b/tig.c
> index 72f132a..dd4b0f4 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -4384,6 +4384,33 @@ pager_select(struct view *view, struct line *line)
>         }
>  }
>
> +struct log_state {
> +       bool update_commit_ref;
> +};
> +
> +static void
> +log_select(struct view *view, struct line *line)
> +{
> +       struct log_state *state = (struct log_state *) view->private;
> +
> +       if (state->update_commit_ref && line->lineno > 1) {
> +               /* We need to recalculate the previous commit,
> +                  since the user has likely scrolled up. */

I'd prefer that state->update_commit_ref is given another name so it
won't be necessary to have these comments everywhere the field is
used, for example recalculate_commit_context. The comment could be
moved to the declaration in struct log_state to explain its use.

Multi-line comments should start with '*' for each additonal line, e.g.

  /* bla bla
   * bla bla */

> +               const struct line *commit_line = find_prev_line_by_type(view, line, LINE_COMMIT);
> +
> +               if (commit_line)
> +                       string_copy_rev(view->ref, (char *) (commit_line->data + STRING_SIZE("commit ")));

You mentioned elsewhere that this looked funny, and I guess you are
right. I will extract this into a utility method so you can simply
call: string_copy_rev_from_line(view->ref, commit_line); ...

> +       }
> +       if (line->type == LINE_COMMIT) {
> +               char *text = (char *)line->data + STRING_SIZE("commit ");
> +
> +               if (!view_has_flags(view, VIEW_NO_REF))
> +                       string_copy_rev(view->ref, text);

... and: string_copy_rev_from_line(view->ref, line);

> +       }
> +       string_copy_rev(ref_commit, view->ref);
> +       state->update_commit_ref = FALSE;
> +}
> +
>  static bool
>  pager_open(struct view *view, enum open_flags flags)
>  {
> @@ -4427,11 +4454,30 @@ log_open(struct view *view, enum open_flags flags)
>  static enum request
>  log_request(struct view *view, enum request request, struct line *line)
>  {
> +       struct log_state *state = (struct log_state *) view->private;

There's no need to cast view->private here.

> +
>         switch (request) {
>         case REQ_REFRESH:
>                 load_refs();
>                 refresh_view(view);
>                 return REQ_NONE;
> +
> +       case REQ_MOVE_UP:
> +       case REQ_PREVIOUS:
> +               if (line->type == LINE_COMMIT && line->lineno > 1) {
> +                       /* We are at a commit, and heading upward. We
> +                          force log_select to find the previous
> +                          commit above, from the context. */

Please delete this comment.

> +                       state->update_commit_ref = TRUE;
> +               }
> +               return pager_request(view, request, line);

There's not really any reason to call pager_request here, since it
only handles REQ_ENTER.

> +
> +       case REQ_MOVE_PAGE_UP:
> +       case REQ_MOVE_PAGE_DOWN:
> +               /* We need to figure out the right commit again. */

Please delete this this comment.

> +               state->update_commit_ref = TRUE;
> +               return pager_request(view, request, line);

Calling pager_request again.

> +
>         default:
>                 return pager_request(view, request, line);
>         }
> @@ -4441,13 +4487,13 @@ static struct view_ops log_ops = {
>         "line",
>         { "log" },
>         VIEW_ADD_PAGER_REFS | VIEW_OPEN_DIFF | VIEW_SEND_CHILD_ENTER | VIEW_NO_PARENT_NAV,
> -       0,
> +       sizeof(struct log_state),
>         log_open,
>         pager_read,
>         pager_draw,
>         log_request,
>         pager_grep,
> -       pager_select,
> +       log_select,
>  };
>
>  struct diff_state {
> --
> 1.8.3.2
>

-- 
Jonas Fonseca

Re: [[TIG][PATCH] 1/3] Add log_select function to find commit from context in log view

[Kumar Appaiah]

Dear Jonas,

Thanks for the patient review.
On Mon, Aug 05, 2013 at 11:27:44PM -0400, Jonas Fonseca wrote:
> On Fri, Aug 2, 2013 at 8:23 PM, Kumar Appaiah <a.kumar@alumni.iitm.ac.in> wrote:
> > This commit introduces and uses the log_select function to find the
> > correct commit in the unsplit log view. In the log view, if one
> > scrolls down across a commit line, the current commit (as displayed in
> > the status bar) gets updated, but not so when scrolling upward across
> > a commit. The log_select function handles this scenario to to the
> 
> s/to to/to do/

Done.

> > ``right thing''. In addition, it introduces the log_state structure as
> > the private entry of the log view to hold a flag that decides whether
> > to re-evaluate the current commit based on scrolling.
> >
> > Signed-off-by: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>
> > ---
> >  tig.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/tig.c b/tig.c
> > index 72f132a..dd4b0f4 100644
> > --- a/tig.c
> > +++ b/tig.c
> > @@ -4384,6 +4384,33 @@ pager_select(struct view *view, struct line *line)
> >         }
> >  }
> >
> > +struct log_state {
> > +       bool update_commit_ref;
> > +};
> > +
> > +static void
> > +log_select(struct view *view, struct line *line)
> > +{
> > +       struct log_state *state = (struct log_state *) view->private;
> > +
> > +       if (state->update_commit_ref && line->lineno > 1) {
> > +               /* We need to recalculate the previous commit,
> > +                  since the user has likely scrolled up. */
> 
> I'd prefer that state->update_commit_ref is given another name so it
> won't be necessary to have these comments everywhere the field is
> used, for example recalculate_commit_context. The comment could be
> moved to the declaration in struct log_state to explain its use.

Done.

> Multi-line comments should start with '*' for each additonal line, e.g.
> 
>   /* bla bla
>    * bla bla */

Done.

> > +               const struct line *commit_line = find_prev_line_by_type(view, line, LINE_COMMIT);
> > +
> > +               if (commit_line)
> > +                       string_copy_rev(view->ref, (char *) (commit_line->data + STRING_SIZE("commit ")));
> 
> You mentioned elsewhere that this looked funny, and I guess you are
> right. I will extract this into a utility method so you can simply
> call: string_copy_rev_from_line(view->ref, commit_line); ...

I will wait on this. The next iteration of the patch will still have
this problem.

> > +       }
> > +       if (line->type == LINE_COMMIT) {
> > +               char *text = (char *)line->data + STRING_SIZE("commit ");
> > +
> > +               if (!view_has_flags(view, VIEW_NO_REF))
> > +                       string_copy_rev(view->ref, text);
> 
> ... and: string_copy_rev_from_line(view->ref, line);

I understand.

> > +       }
> > +       string_copy_rev(ref_commit, view->ref);
> > +       state->update_commit_ref = FALSE;
> > +}
> > +
> >  static bool
> >  pager_open(struct view *view, enum open_flags flags)
> >  {
> > @@ -4427,11 +4454,30 @@ log_open(struct view *view, enum open_flags flags)
> >  static enum request
> >  log_request(struct view *view, enum request request, struct line *line)
> >  {
> > +       struct log_state *state = (struct log_state *) view->private;
> 
> There's no need to cast view->private here.

Done.

> > +
> >         switch (request) {
> >         case REQ_REFRESH:
> >                 load_refs();
> >                 refresh_view(view);
> >                 return REQ_NONE;
> > +
> > +       case REQ_MOVE_UP:
> > +       case REQ_PREVIOUS:
> > +               if (line->type == LINE_COMMIT && line->lineno > 1) {
> > +                       /* We are at a commit, and heading upward. We
> > +                          force log_select to find the previous
> > +                          commit above, from the context. */
> 
> Please delete this comment.

Done.

> > +                       state->update_commit_ref = TRUE;
> > +               }
> > +               return pager_request(view, request, line);
> 
> There's not really any reason to call pager_request here, since it
> only handles REQ_ENTER.

Done.

> > +
> > +       case REQ_MOVE_PAGE_UP:
> > +       case REQ_MOVE_PAGE_DOWN:
> > +               /* We need to figure out the right commit again. */
> 
> Please delete this this comment.

Done.

> > +               state->update_commit_ref = TRUE;
> > +               return pager_request(view, request, line);
> 
> Calling pager_request again.

Done.

I will send in another patch to review shortly.

Thanks!

Kumar
-- 
Kumar Appaiah

Re: [[TIG][PATCH] 1/3] Add log_select function to find commit from context in log view

[Jonas Fonseca]

On Fri, Aug 2, 2013 at 8:23 PM, Kumar Appaiah <a.kumar@alumni.iitm.ac.in> wrote:
> diff --git a/tig.c b/tig.c
> index 72f132a..dd4b0f4 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -4427,11 +4454,30 @@ log_open(struct view *view, enum open_flags flags)
>  static enum request
>  log_request(struct view *view, enum request request, struct line *line)
>  {
> +       struct log_state *state = (struct log_state *) view->private;
> +
>         switch (request) {
>         case REQ_REFRESH:
>                 load_refs();
>                 refresh_view(view);
>                 return REQ_NONE;
> +
> +       case REQ_MOVE_UP:
> +       case REQ_PREVIOUS:
> +               if (line->type == LINE_COMMIT && line->lineno > 1) {
> +                       /* We are at a commit, and heading upward. We
> +                          force log_select to find the previous
> +                          commit above, from the context. */
> +                       state->update_commit_ref = TRUE;
> +               }
> +               return pager_request(view, request, line);
> +
> +       case REQ_MOVE_PAGE_UP:
> +       case REQ_MOVE_PAGE_DOWN:
> +               /* We need to figure out the right commit again. */
> +               state->update_commit_ref = TRUE;
> +               return pager_request(view, request, line);
> +
>         default:
>                 return pager_request(view, request, line);
>         }

I forgot to mention there is one use case this doesn't currently
handle, namely jumping to a specific line using ':<number>'. Other
than detecting this by tracking the current line number in log_state I
haven't come up with a good way to detect that a recalculation is
required.

[[TIG][PATCH] 2/3] Display correct diff the context in split log view

[Kumar Appaiah]

In the log view, when scrolling across a commit, the diff view should
automatically switch to the commit whose context the cursor is on in
the log view. This commit changes things to catch the REQ_ENTER in the
log view and handle recalculation of the commit and diff display from
log_request, rather than delegating it to pager_request. In addition,
it also gets rid of unexpected upward scrolling of the log view.

Fixes GH #155

Signed-Off-By: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>
---
 NEWS  |  1 +
 tig.c | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/NEWS b/NEWS
index 0394407..f59e517 100644
--- a/NEWS
+++ b/NEWS
@@ -46,6 +46,7 @@ Bug fixes:
  - Fix rendering glitch for branch names.
  - Do not apply diff styling to untracked files in the stage view. (GH #153)
  - Fix tree indentation for entries containing combining characters. (GH #170)
+ - Introduce a more natural context-sensitive log display. (GH #155)
 
 tig-1.1
 -------
diff --git a/tig.c b/tig.c
index dd4b0f4..53947b7 100644
--- a/tig.c
+++ b/tig.c
@@ -4478,6 +4478,18 @@ log_request(struct view *view, enum request request, struct line *line)
 		state->update_commit_ref = TRUE;
 		return pager_request(view, request, line);
 
+	case REQ_ENTER:
+		/* Recalculate the correct commit for the context. */
+		state->update_commit_ref = TRUE;
+
+		open_view(view, REQ_VIEW_DIFF, OPEN_SPLIT);
+		update_view_title(view);
+
+		/* We don't want to delegate this to pager_request,
+		   since we don't want the extra scrolling of the log
+		   view. */
+		return REQ_NONE;
+
 	default:
 		return pager_request(view, request, line);
 	}
-- 
1.8.3.2

Re: [[TIG][PATCH] 2/3] Display correct diff the context in split log view

[Jonas Fonseca]

On Fri, Aug 2, 2013 at 8:23 PM, Kumar Appaiah <a.kumar@alumni.iitm.ac.in> wrote:
> In the log view, when scrolling across a commit, the diff view should
> automatically switch to the commit whose context the cursor is on in
> the log view. This commit changes things to catch the REQ_ENTER in the
> log view and handle recalculation of the commit and diff display from
> log_request, rather than delegating it to pager_request. In addition,
> it also gets rid of unexpected upward scrolling of the log view.
>
> Fixes GH #155
>
> Signed-Off-By: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>
> ---
>  NEWS  |  1 +
>  tig.c | 12 ++++++++++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 0394407..f59e517 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -46,6 +46,7 @@ Bug fixes:
>   - Fix rendering glitch for branch names.
>   - Do not apply diff styling to untracked files in the stage view. (GH #153)
>   - Fix tree indentation for entries containing combining characters. (GH #170)
> + - Introduce a more natural context-sensitive log display. (GH #155)
>
>  tig-1.1
>  -------
> diff --git a/tig.c b/tig.c
> index dd4b0f4..53947b7 100644
> --- a/tig.c
> +++ b/tig.c
> @@ -4478,6 +4478,18 @@ log_request(struct view *view, enum request request, struct line *line)
>                 state->update_commit_ref = TRUE;
>                 return pager_request(view, request, line);
>
> +       case REQ_ENTER:
> +               /* Recalculate the correct commit for the context. */

See my dislike for this type of comments. ;)

> +               state->update_commit_ref = TRUE;
> +
> +               open_view(view, REQ_VIEW_DIFF, OPEN_SPLIT);

This is called every time the user presses up/down. There should be a
check that compares the VIEW(REQ_VIEW_DIFF)->ref to ref_commit.

> +               update_view_title(view);

This can be deleted. pager_request require this hack due to the
automatic scrolling (if I recall correctly).

> +
> +               /* We don't want to delegate this to pager_request,
> +                  since we don't want the extra scrolling of the log
> +                  view. */

This explanation should IMO go into the commit message and not a
comment since it is somewhat confusing unless you are familiar with
the previous behaviour.

> +               return REQ_NONE;
> +
>         default:
>                 return pager_request(view, request, line);

This line can be changed to `return request;`

>         }
> --
> 1.8.3.2
>

-- 
Jonas Fonseca

[[TIG][PATCH] 3/3] Revert "Scroll diff with arrow keys in log view"

[Kumar Appaiah]

This reverts commit 888611dd5d407775245d574a3dc5c01b5963a5ba. This is
because, in the re-engineered log view, scrolling the log with the
arrows now updates the diff in the diff view when the screen is
split. This resembles the earlier behaviour, and is also what users of
software like Mutt (which uses the pager view concept) would expect.

Signed-Off-By: Kumar Appaiah <a.kumar@alumni.iitm.ac.in>

Conflicts:
	tig.c
---
 tig.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tig.c b/tig.c
index 53947b7..65c91a0 100644
--- a/tig.c
+++ b/tig.c
@@ -1901,7 +1901,6 @@ enum view_flag {
 	VIEW_STDIN		= 1 << 8,
 	VIEW_SEND_CHILD_ENTER	= 1 << 9,
 	VIEW_FILE_FILTER	= 1 << 10,
-	VIEW_NO_PARENT_NAV	= 1 << 11,
 };
 
 #define view_has_flags(view, flag)	((view)->ops->flags & (flag))
@@ -3774,7 +3773,7 @@ view_driver(struct view *view, enum request request)
 
 	case REQ_NEXT:
 	case REQ_PREVIOUS:
-		if (view->parent && !view_has_flags(view->parent, VIEW_NO_PARENT_NAV)) {
+		if (view->parent) {
 			int line;
 
 			view = view->parent;
@@ -4498,7 +4497,7 @@ log_request(struct view *view, enum request request, struct line *line)
 static struct view_ops log_ops = {
 	"line",
 	{ "log" },
-	VIEW_ADD_PAGER_REFS | VIEW_OPEN_DIFF | VIEW_SEND_CHILD_ENTER | VIEW_NO_PARENT_NAV,
+	VIEW_ADD_PAGER_REFS | VIEW_OPEN_DIFF | VIEW_SEND_CHILD_ENTER,
 	sizeof(struct log_state),
 	log_open,
 	pager_read,
-- 
1.8.3.2

Re: [TIG][PATCH 0/3] Refactoring of the log view

[Jonas Fonseca]

On Fri, Aug 2, 2013 at 8:23 PM, Kumar Appaiah <a.kumar@alumni.iitm.ac.in> wrote:
> These set of patches refactor the log view to provide a behaviour that
> is quite similar to, say, e-mail with Mutt. The key improvements are:
>
> - The current commit is inferred based on the context. For example, if
>   you focus on the commit message of a particular commit, the correct
>   commit is inferred automagically.
>
> - Scrolling the log view when the diff is open shows the correct
>   commit on the screen, rather than have to scroll up and cross the
>   commit line to display the screen.

Thanks, great improvements. I am still considering whether to queue
them until after the next release or include them.

> I have decided to revert 888611dd5d407775245d574a3dc5c01b5963a5ba,
> since the behaviour with the updated scrolling pattern is much more
> consistent.

OK, makes sense.

The next step will be to find out how to highlight the diff stat in
the log view. :-D