GNOME Bugzilla – Bug 588720
cursor warping is broken
Last modified: 2010-03-02 05:48:18 UTC
My first attempt at warping the cursor when next/previous diff buttons are pressed was buggy. Here is a patch to hopefully fix that. It also reworks the scrolledwindow warping as that is related to the cursor. Please review carefully, this area of the code was hard to understand and I may have missed something.
Created attachment 138481 [details] [review] fix & rework warping lightly tested on itself, plus 2- and 3-way diffs, seemed OK
I'm still testing the patch, but generally it looks okay to me. The only problematic change I could find is in how equal segments work in three-way diffs. In the old code, if you had two panes with no changes (e.g., meld a a b) then with pane 1 focused, next/prev would still step pane 1 through every change including those between panes 2 and 3. With the new code it does nothing. I think this a problem, since you generally want to stop *anywhere* that a change occurs. As for the rest, I really like the scrolling minimisation change, and those thresholds feel about right. Minor comments follow. + if direction == gdk.SCROLL_DOWN: + # Take the first chunk which is starting after curline + c = chunk + if chunk[1] > curline: break The c = chunk line should appear inside the conditional. Otherwise, when on or past the last chunk, the last chunk will be returned. This can, for example, warp the cursor upwards when scrolling down at the end. + def next_diff(self, direction, jump_to_first=False): + buf = self._get_focused_textview().get_buffer() _get_focused_textview() isn't actually used by anything else. I thought I removed it ages ago. self.textbuffer[pane] should work here, but you need to decide what to do in the unfocused case (i.e., pane == -1). + lo = a.value + a.page_size * 1 / 10 + hi = a.value + a.page_size * 9 / 10 Pure personal preference, but I think that using 0.1 and 0.9 here would be clearer. When I first read this, I wondered if there might be integer division issues; there aren't since page_size is a float, but using float constants here makes it obvious.
> I'm still testing the patch, but generally it looks okay to me. The only > problematic change I could find is in how equal segments work in three-way > diffs. In the old code, if you had two panes with no changes (e.g., > meld a a b) > then with pane 1 focused, next/prev would still step pane 1 through every > change including those between panes 2 and 3. With the new code it does > nothing. I think this a problem, since you generally want to stop > *anywhere* that a change occurs. Ouch, I'll try to look into that > As for the rest, I really like the scrolling minimisation change, and those > thresholds feel about right. Minor comments follow. > > + if direction == gdk.SCROLL_DOWN: > + # Take the first chunk which is starting after curline > + c = chunk > + if chunk[1] > curline: > break > > The c = chunk line should appear inside the conditional. Otherwise, > when on or past the last chunk, the last chunk will be returned. > This can, for example, warp the cursor upwards when scrolling down > at the end. Are you sure, I was thinking the old code was doing exactly the same as my patch. Look how the loop variable is c for DOWN and chunk for UP. This was very confusing, and the cause of the method extraction. I had some versions of the patch having the behaviour you describe but I didn't see it for the present patch do you have a testcase to reproduce ? > + def next_diff(self, direction, jump_to_first=False): > + buf = self._get_focused_textview().get_buffer() > > _get_focused_textview() isn't actually used by anything else. > I thought I removed it ages ago. self.textbuffer[pane] should > work here, but you need to decide what to do in the unfocused > case (i.e., pane == -1). > > + lo = a.value + a.page_size * 1 / 10 > + hi = a.value + a.page_size * 9 / 10 > > Pure personal preference, but I think that using 0.1 and 0.9 here > would be clearer. When I first read this, I wondered if there might > be integer division issues; there aren't since page_size is a float, > but using float constants here makes it obvious. OK I'll respin
I think the behavior on "meld a a b" could be seen as a feature, i.e. it tells you there are no diffs at all in the currently focused pane and its next sibling. At least to me it looks sensible and would be consistent with the UP/DOWN button sensitivity thing I've been working on: the 2 buttons would be grayed if first pane is focused... What would you prefer: - do nothing like with my patch - focus the next change in the next pane, switching focus to that pane - current warp as if you have the middle pane focused, but without switching focus It's not completely obvious to me we unconditionally want the third option. The only real argument I find for this one is: "it's like that today, a change is a regression" And trying the current git code, I'm starting to really dislike it: the cursor warping in and out of sight depending on the size of the hunk you are warping to is not nice. I prefer the warping to go to the first line of the hunk and not the middle of it, because the arrows to merge that hunk are there. And even if only reading, you don't want to jump into the middle of the hunk and then have to scroll manually to the top of it to start understanding what the hunk is about... Any ideas or comments ?
> Are you sure, I was thinking the old code was doing exactly the same > as my patch. Look how the loop variable is c for DOWN and chunk for > UP. This was very confusing, and the cause of the method extraction. You're right, the old code does exactly the same thing, and I agree that it is confusing. However, I'd say that if you're below the last chunk, then surely the expected behaviour is to do nothing... or at least not to scroll up. > I had some versions of the patch having the behaviour you describe > but I didn't see it for the present patch do you have a testcase to > reproduce ? In a current git checkout with the patch applied, run ./meld glade2/dirdiff-ui.xml glade2/vcview-ui.xml place the cursor after the last change (in either pane) and press Ctrl-D. <snip> > It's not completely obvious to me we unconditionally want the > third option. The only real argument I find for this one is: > "it's like that today, a change is a regression" The argument for the way it is currently is that rather than moving through the list of changes in a pane, you're actually moving through the list of changes between all files, and the cursor in the focused pane is just a hint as to where you're up to. However, this is ultimately a decision about what the user's mental model of the application is, and is therefore complicated. Obviously, I think I'm right, but really I have no idea. Having said that, I think the idea of warping the cursor (option 2) to the pane with the change (which is guaranteed to be pane 1?) is a good one. If that happened, should it start at the top? or at the corresponding line? > And trying the current git code, I'm starting to really dislike > it: the cursor warping in and out of sight depending on the size > of the hunk you are warping to is not nice. I prefer the warping > to go to the first line of the hunk and not the middle of it, > because the arrows to merge that hunk are there. And even if > only reading, you don't want to jump into the middle of the > hunk and then have to scroll manually to the top of it to start > understanding what the hunk is about... I absolutely agree. Those changes to the warping behaviour are great, and make it *much* nicer to use.
Here is a revised version with your points handled except for the 3-pane with only 2 different files, which I'll look at next. You were right about the "c = chunk" it was silly to mimic the old code, because it is much better not to warp back to the beginning of last chunk if we scrolled manually after it. - Removed _get_focused_textview() and inlined the code - Used float constants - Only warp cursor if it is not already there
Created attachment 138538 [details] [review] warping patch v2
+ pane = self._get_focused_pane() + if pane not in (0, 1, 2): It would seem simpler to use, if pane != -1: + pane = 0 + buf = self.textview[pane].get_buffer() There's no need for this indirection, buf = self.textbuffer[pane] will work fine. I'm running with this patch now. It's pretty nice, and I haven't noticed any other problems. As for the 3-way comparison, I can't think of a situation in which I only want to stop at changes between two panes. I think I personally always want the diff-jumping to behave as though my cursor is in the middle pane. I am curious as to what other people think though.
Created attachment 138606 [details] [review] warping patch v3 incorporated your latest comments (still not changing 3-way behavior)
Created attachment 138607 [details] [review] extract common method for future use This code will get multiple users in the following patch
Created attachment 138614 [details] [review] Update UP/DOWN sensitivity This is my old patch ported over latest code, it complements nicely the warping patch and I think the behavior is consistent, despite not being what you (Kai) want. I've thought about this and cannot think of a way to do it in a clean and consistent way. Maybe it can be revisited after this previous work has landed, because there's still a bug in current code which needs to be fixed before a release.
I still think that the 3-way behaviour is not what a user probably wants, but your patch is certainly an improvement in every other way. I'd say commit the warping patch, and maybe we'll revisit the issue later. Maybe attach the sensitivity setting patches to bug 533752, so they can be tracked separately.
Now that the base is committed, I'll rebase up/down sensitivity patches and attach them to the other bug report. Thanks for your comments
I'll let the bug report open until the 3 pane behavior is problem is tackled
So I was wrong, and Vincent was right. The current behaviour is correct, and not just because it's easy. The reason is that when you have keyboard actions on diff chunks, navigating to a chunk that you *can't do anything with* is just silly. It kind of makes sense while just browsing, but not at all with editing. Anyway, I think this bug can finally be closed.