Scrolling broken with diff-in-log view

[kumanna]

Hi.

If I use the log view, scroll down, and then open a diff by focusing on it and press enter to view it, I expect the arrow keys to move along the log view. The up-arrow does move the parent view, but in a weird way when I want it to go up (i.e. both the highlight line and the view are scrolled up). In other words, when the diff is displayed, an UP arrows causes both the view AND the highlighting line to scroll up simultaneously, while scrolling down keeps the highlight line intact while only scrolling the view.

Please let me know if I need to describe the issue better. I couldn't patch it myself, since much has changed in the way the views are handled since this change was introduced in 5568f99.

[jonas]

I agree that the log view is weird and could use some love. Basically, it acts similar to the main view, and most other views in tig, where the "sub-view" (in this case diff) only gets partial focus when opened in that certain requests (enter, move to next, move to previous) are forwarded to the parent view (in this case the log view). This works well in the main and blame views where 'move to next' translates to move the cursor to the next commit or line.

Fixing this for the log view should be fairly simple. In the meantime, the workaround is to bind up/down to move-up/move-down instead of next/preview (which is the default).

bind diff Up move-up
bind diff Down move-down

[kumanna]

Is this too much of a hack? It modifies the behaviour, but at least makes things work in an expected manner.

Thanks.

diff --git a/tig.c b/tig.c
index 8301295..6bd4cac 100644
--- a/tig.c
+++ b/tig.c
@@ -4341,10 +4341,10 @@ pager_request(struct view *view, enum request request, struct line *line)
                split = 1;
        }

-       /* Always scroll the view even if it was split. That way
-        * you can use Enter to scroll through the log view and
-        * split open each commit diff. */
-       scroll_view(view, REQ_SCROLL_LINE_DOWN);
+       /* Scroll through the view except when the log is
+        * displayed. This is to ensure that we don't "double scroll"
+        * up within the split log view. */
+       if (strcmp(view->name, "log")) scroll_view(view, REQ_SCROLL_LINE_DOWN);

        /* FIXME: A minor workaround. Scrolling the view will call report_clear()
         * but if we are scrolling a non-current view this won't properly

[jonas]

I think it would be cleaner to just add a case REQ_ENTER in log_request() which doesn't do the scroll thing.

[kumanna]

I don't understand. In log_request, currently, when REQ_ENTER is received, pager_request is called, and that causes the extra scrolling. If the behaviour is correct when the down arrow is pressed, we only need to change the behaviour when the up arrow is pressed. How do I detect which key was pressed? Does this need something like REQ_ENTER_DONT_SCROLL and adding a modified pager_request that handles this case?

Sorry for the ramble; I'll patch it if I understand how best to fix this.

[jonas]

I think it was I who was rambling.

The best approach I see is to introduce a new view_flag (for example named VIEW_NO_PARENT_NAV) which will be added to the log view. The view flag will be used in view_driver() to ensure that the diff view receives the move request (from the Up and Down key)

Here's the current code:

    case REQ_NEXT:
    case REQ_PREVIOUS:
            if (view->parent) {
                    int line;

                    view = view->parent;
                    line = view->pos.lineno;
                    move_view(view, request);
                    if (view_is_displayed(view))
                            update_view_title(view);
                    if (line != view->pos.lineno)
                            view_request(view, REQ_ENTER);
            } else {
                    move_view(view, request);
            }
            break;

Which should check the flag before forwarding the move request to the parent view, like this:

            if (view->parent && !view_has_flags(view, VIEW_NO_PARENT_NAV)) {
                    /* ... */
            } else {
                    move_view(view, request);
            }

An extra bonus would be to detach log_request() from pager_request() by having it handle REQ_ENTER itself, since I don't think it makes sense (to me anymore) to scroll down the view when pressing enter on a commit line. I don't use the log view, so I would like to know your opinion.

[kumanna]

Hi Jonas.

Thanks for your hint. Your suggestion was almost right, but it should be view_has_flags(view->parent, VIEW_NO_PARENT_NAV). I have sent a patch to the git list, and CCed you, because I dislike sending a pull request.

As for handling log_request, I frankly don't like scrolling when enter is pressed. For my mutt user sensibility, it is more natural if pressing enter opens up the current commit in focus. I could work on this if you are agreeable to this.

Now, what would be ideal, is if I press enter even on the commit message, it should open up the commit I have focused. The trouble is that we only know the last touched commit. So, if I scroll down from the first commit to the second commit's "commit" line, then immediately scroll back up, then what is displayed on the status bar is the second commit. This would require additional effort, and I might take this up as a secondary task.

What do you think? Thanks!

[jonas]

I agree with you that the log view should not scroll when pressing Enter on a commit.

I think it make sense to make the log view more context sensitive. Example code for how to do this can be found in diff_get_pathname().

    const struct line *header = find_prev_line_by_type(view, line, LINE_DIFF_HEADER);
    if (!header) /* ... */

Basically, find_prev_line_by_type search backwards in the view's array of lines starting from line. So to find the commit associated with a given line inside the log view simply search for LINE_COMMIT.

To handle the back-scroll issue, you could add some logic inside log_request() to pre-process certain move requests based on the view state. For example, if the current line has type LINE_COMMIT, REQ_MOVE_UP/REQ_PREVIOUS should recalculate the 'touched commit', etc. To communicate that the 'touched commit' must be recalculated the log could clear one the view->ref string so that log_select (which needs to be added) knows it must find the commit based on the current line.

Home this makes sense.

[kumanna]

Thanks Jonas. This makes total sense. I'll take a stab at it over the next few days, and seek your comments.

[kumanna]

I've managed to hack together what you've suggested. It works perfectly for me, but I haven't sent in the patches because I wanted your inputs on style. The commits are here:

kumanna@4e699c0
kumanna@aed8f49

Now, in 4e699c0, can I improve this:

string_copy_rev(view->ref, (char *) (context_commit->data + 7));

Or is this an acceptable way of copying the commit hash?

Second, I set view->ref null by doing view->ref[0] = '\0'; and check that it is null by doing view->ref[0] == '\0'. Is this acceptable, or is there a better way of doing it?

If you are happy with the patches, please let me know and I will git send-email them to you.

Thanks.

[kumanna]

Ah, so it also needs work when page up/down is used. Let me handle that as well.

EDIT: Done in 387c693

[kumanna]

Thank you for your quick comments, and for your patience in explaining things. I have refactored them to 29c8404 and 6b143e5. I hope these are more palatable; I can improve it if you have more suggestions.

[kumanna]

Ah, you just pushed to master. I will need to rebase back to master, so once you review the above commits, I shall do so and git send-email them.

Thanks.

[kumanna]

Also, while you're at it, I personally feel that adding the above patch now leads me to question 888611d. This is because the behaviour in the standard tig view is that the arrows move across the commits, while pressing enter scrolls the commit. This is the behaviour in Mutt as well.

I can adjust my usage behaviour either way, so it doesn't matter, but just pointing this out. Thanks.

[kumanna]

I've updated and rebased to master here. This is the same as the patch series sent to the git list. https://github.com/kumanna/tig/tree/log-refactor

[jonas]

Reviewed. I really like these improvements.

[jonas]

I am getting lost in your posts to git@vger and your github repository. Can you please make a new pull request?

[kumanna]

Sorry. I will be consistent and use only Github henceforth.

A pull request will come your way.

Thanks.

[jonas]

No problem. BTW, I've added the method I talked about, maybe it will simplify your patch. It's called string_copy_rev_from_commit_line.

[kumanna]

Ah. Give me a minute, I'll see what you've done.

[jonas]

Yeah, sorry about that, I am a bit disorganized: asking one thing and sending additional info at the same time. ;)

[kumanna]

Okay. I'll resend a pull request.

[jonas]

Forgot to close this from the commit. Anyways, thanks for your work on this @kumanna .