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

Some minor updates for scrolling and for a double-click in diff #1039

Closed
wants to merge 1 commit into from
Closed

Some minor updates for scrolling and for a double-click in diff #1039

wants to merge 1 commit into from

Conversation

mckellyln
Copy link
Contributor

@jonas Thank you for awesome tig. I use it every day.
Here are a few small updates that I think improve scrolling, and to not Enter when double-clicking on a diff line.
What do you think ?
thx,
-m

@mckellyln
Copy link
Contributor Author

Perhaps the 3 test diffs are due to the small changes in scrolling ?

@krobelus
Copy link
Contributor

I wanted to try this because I'm not sure what exactly changed.
Does it need mouse support? I tried set mouse = true, using reasonable ncurses options, but the mouse behavior doesn't change at all, clicking just selects text, just like before I enabled mouse.

@mckellyln
Copy link
Contributor Author

The changes are small but I found quite useful. The changes are for both mouse or key scrolling.
1). double-click mouse in diff window no longer causes cursor to move down one line.
2). scrolling with mouse does not allow for scrolling past the last line at the bottom.
3). scrolling now moves the cursor same number of lines as I think is typical with other editors/viewers.
And I use these settings in my .tigrc for Shift-Down/Up for key-based scrolling:

bind generic   <ScrollFwd>  scroll-line-down
bind generic   <ScrollBack> scroll-line-up
# and for mouse -
set mouse              = yes
set mouse-scroll       = 10
set mouse-wheel-cursor = no

@mckellyln
Copy link
Contributor Author

mckellyln commented Sep 25, 2020

I configured --with-ncursesw and have ncurses 6.2 installed. But I don't think you need the most recent ncurses for any of this.

Copy link
Contributor

@krobelus krobelus left a comment

Choose a reason for hiding this comment

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

Thanks, it works with this config! 1. and 2. are obvious improvements!
I somehow can't figure how to reproduce the difference from 3. 🤔

src/view.c Outdated
@@ -164,7 +172,24 @@ scroll_view(struct view *view, enum request request)
die("request %d not handled in switch", request);
}

unsigned long orig_offset = view->pos.offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the style convention here is to always declare variables at the beginning of a block (pre-C99).


int i;
if (move_lines < 0) {
for(i=0; i<-move_lines; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

at first I thought this was a reverse dereference operator 😆

@mckellyln
Copy link
Contributor Author

mckellyln commented Sep 25, 2020

ok, I will move the declaration up, outside the block in a amended commit in a few.
For item 3 move down a few lines so you are not at the top and then scroll a few lines. Without my change the curosr moved with the scroll. I wanted the cursor to remain at the same screen position.

@mckellyln
Copy link
Contributor Author

Amended commit.

@krobelus
Copy link
Contributor

For item 3 move down a few lines so you are not at the top and then scroll a few lines

Right, I see it now, it's a really nice improvement as well. I had already noticed the difference before, but then re-enabled mouse-wheel-cursor which always moves the cursor, so it's not visible with that.

@mckellyln
Copy link
Contributor Author

Closing for now, may resubmit PR soon.

@mckellyln mckellyln closed this Dec 29, 2020
@krobelus
Copy link
Contributor

This still applies cleanly and works well, I don't think this needs more work (besides maybe small style changes).

@mckellyln
Copy link
Contributor Author

Resubmitted from another repo at: #1060

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.

2 participants