Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow :back to work across separate blame views #1127

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

koutcher
Copy link
Collaborator

  • stop clearing the history state when entering the blame view

  • push a blame view history state before entering the diff view

  • stop failing when trying to push an identical history state

Closes #1123

* stop clearing the history state when entering the blame view

* push a blame view history state before entering the diff view

* stop failing when trying to push an identical history state

Closes jonas#1123
@krobelus
Copy link
Contributor

Thanks, this works great! I felt like it would be easy but I didn't understand enough of the view code.

stop failing when trying to push an identical history state

Makes sense, this is required so we don't error on tig blame 529182c -- README.adoc Enter q Enter.

@krobelus
Copy link
Contributor

krobelus commented Jul 21, 2021

I found a similar use case for the diff view.

  • tig show 529182c -- README.adoc
  • :23
    • The cursor should be on the line after "The latest version is", that is, the deleted line containing tig-2.5.3
  • Press b to enter the blame view.
    • What is a bit strange here is that the blame view shows the commit that
      added the line with tig-2.5.4. I expect it to show the commit that
      added the line with tig-2.5.3. This would be consistent with how we
      blame other deletions, so this might be an off-by-one error in tig show
  • Press d to show the diff of commit tig-2.5.4, and :23 to go to the same line (deleted tig-2.5.3).
  • Press b to blame the same line again but this time we actually get to the
    tig-2.5.3 commit.
  • Press d to show the diff of commit tig-2.5.3. Now there is no nice way to get back to the diff of commit tig-2.5.4.
    • If I press q twice, I get back to the diff view which is still on the
      stack, but it shows commit tig-2.5.4. So I think we want to teach the diff
      view how to save/restore state, so :back or q could both go back to the earlier commit.

@koutcher
Copy link
Collaborator Author

There is an issue when the diff view is the initial view. argv_env.commit is not set and the commit selection for blaming is done against HEAD instead of 529182c.

Here is a quick fix but I'm not completely sure it's the right way to do:

--- a/src/diff.c
+++ b/src/diff.c
@@ -34,6 +34,9 @@ diff_open(struct view *view, enum open_flags flags)
 	};
 	enum status_code code;
 
+	if (is_initial_view(view) && opt_rev_args)
+		string_copy_rev(argv_env.commit, opt_rev_args[0]);
+
 	diff_save_line(view, view->private, flags);
 
 	code = begin_update(view, NULL, diff_argv, flags);

krobelus added a commit to krobelus/tig that referenced this pull request Jul 24, 2021
@krobelus
Copy link
Contributor

I've openend #1135 with my attempt at a different fix. It's a bit hacky, I'll recheck it soon.

The other issue with the diff-view history would be solved if :view-close could restore the per-view history, like :back does for the blame view. The diff view doesn't have it's own history yet.
Currently, when in the blame view, a :view-blame will not push a view to the view stack, but only modify the blame view's history.
We could simplify this by always pushing a new blame-view on :view-blame. This would mean that every view on the view stack corresponds to (at most) one view state, which we can restore on :view-close. Of course this might make :view-close worse for the blame view.

Copy link
Owner

@jonas jonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of keeping the history across "blame uses". The initial use case was mostly to be able to deep dive and drill down into "parent blame views". But more horizontal exploration also makes sense.

@@ -393,7 +393,7 @@ push_view_history_state(struct view_history *history, struct position *position,

if (state && data && history->state_alloc &&
!memcmp(state->data, data, history->state_alloc))
return NULL;
return state;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Return the current stack when nothing changed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, otherwise reopening the same blame fails.

@koutcher koutcher merged commit 0d47854 into jonas:master Oct 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow :back to work across separate blame views
3 participants