patchreview : allow choice of the number of context lines
Needs ReviewPublic

Authored by rjvbb on Mar 8 2017, 10:30 PM.

Details

Reviewers
kfunk
mwolff
kossebau
Group Reviewers
KDevelop
Summary

There are times where it can be useful or even necessary to use a larger number of lines of context than the usual standard 3 lines in unified diffs.

One example is the kind of patch that can be applied multiple times, repeating the programmed difference each time. That actually happens quite often to me.

Another example is uploading a so-called "raw diff" (changes not committed locally) to Phabricator, using the Purpose/Phabricator plugin or otherwise. Such patches would normally lack context beyond what is contained in the patch itself. Increasing the number of lines makes the review process easier.

This patch introduces a subtle extension to the patchreview UI that allows to select a custom number of context lines: it adds a context menu to the Update button that offers a selection of context lines plus the special "whole file" value to include the full file as context. Once a number has been selected it will be be used each time the Update button is clicked or the diff is updated for other reasons, for the lifetime of the current patch review process.

Uploading raw diffs generated with the "whole file" option to Phabricator provides reviewers with the same reviewing convenience as uploading committed differences.

Test Plan

Works as intended as far as the custom context feature has been implemented in the various VCS plugins.

It would be nice if the patchfile window reloaded automatically whenever the file changes (on all platforms; works on Linux, not on Mac).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rjvbb created this revision.Mar 8 2017, 10:30 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 8 2017, 10:30 PM
apol added a subscriber: apol.Mar 9 2017, 9:34 AM

This is not integrating it in the UI, is it?

rjvbb added a comment.EditedMar 9 2017, 9:42 AM
In D4981#93816, @apol wrote:

This is not integrating it in the UI, is it?

It is: see the change to patchreview.ui and patchreviewtoolview.cpp . Easy to miss I guess with all the required other changes (and that's without an actual implementation for at least bazaar and svn) :)

I just notice that I forgot to add a few changes to vcsdiffpatchsources.cpp, dang.

rjvbb updated this revision to Diff 12325.Mar 9 2017, 12:10 PM

this adds the missing hunk for vcsdiffpathsources.cpp

rjvbb added a comment.Mar 9 2017, 12:13 PM
This comment was removed by rjvbb.
rjvbb added a comment.Mar 9 2017, 12:15 PM

Screenshot of the UI

apol added a comment.Mar 9 2017, 7:10 PM

I'm sorry, but I don't see when this is useful.

rjvbb added a comment.Mar 9 2017, 7:34 PM
In D4981#93946, @apol wrote:

I'm sorry, but I don't see when this is useful.

That's described in the summary. You may not believe those arguments but they're not invented. Also see the May 12th 2014 comment on https://secure.phabricator.com/T5029 ; a large enough number of context lines is the "official" way to upload useful raw patches to Phab .

The patch has many small changes but I don't think that any of them is complicated and hard to maintain, should be apply and mostly forget.

apol added a comment.Mar 9 2017, 11:10 PM
In D4981#93948, @rjvbb wrote:
In D4981#93946, @apol wrote:

I'm sorry, but I don't see when this is useful.

That's described in the summary. You may not believe those arguments but they're not invented. Also see the May 12th 2014 comment on https://secure.phabricator.com/T5029 ; a large enough number of context lines is the "official" way to upload useful raw patches to Phab .

The patch has many small changes but I don't think that any of them is complicated and hard to maintain, should be apply and mostly forget.

No, but it all adds complexity.

If we need more context on the git patches, I'd add it right away on git plugin. No need to abstract it on the whole architecture.

rjvbb added a comment.Mar 10 2017, 9:02 AM

It adds a bit of complexity, but it looks worse than it is if you ask me. It's in fact not much more than a single additional argument; the complexity stems more from the number of places where that argument has to be added than from the changes themselves.

In turn that stems from the complexity of the whole underlying design. It's not the first time I set out to make what I thought would be a simple change was annoyed to find I needed to make way more changes in way more hard to find places than you'd expect. It was indeed my plan to test the whole idea only with git, but I couldn't see a way to put the spinbox where it is now, make it trigger a patch review update and access its information only in the git plugin. Besides, the feature should probably be implemented for the other VCS flavours supported by Phab, at least svn.

Maybe we could use the occasion to change the API to pass the arguments that have default values via a pointer to a class or struct which can hopefully be defined completely in the base class?

Or maybe this new parameter can be implemented as a member variable in IBasicVersionControl rather than as a function argument? The class already has a setter for another property so it isn't purely abstract, and this would mean a lot less passing around of an additional variable. I'll have a look at that approach, see if it makes the patch less complex.

kfunk requested changes to this revision.Mar 10 2017, 9:15 AM
kfunk added a subscriber: kfunk.

-1 as it is. The changes to IBasicVersionControl are fine IMO, but the ones to the other interfaces need careful reconsideration.

plugins/perforce/perforceplugin.h
91 ↗(On Diff #12325)

Ditch the const. Not needed for POD types.

plugins/subversion/kdevsvnplugin.cpp
239

Why does this store contextLines into a member? That's totally cumbersome from a API POV.

plugins/subversion/kdevsvnplugin.h
158 ↗(On Diff #12325)

Makes no sense to have this as member

vcs/interfaces/ibasicversioncontrol.h
208

Dito, ditch const

vcs/interfaces/ipatchsource.h
53

I really don't like this API, but can't think of anything better right now.

If at all, this needs better API documentation, with named arguments in the signature + @param in doxygen, etc.

vcs/widgets/vcsdiffpatchsources.h
45

Still, odd API. lines is ambiguous, too, should be contextLines if at all.

This revision now requires changes to proceed.Mar 10 2017, 9:15 AM
kfunk added a comment.EditedMar 10 2017, 9:17 AM
In D4981#93852, @rjvbb wrote:

Screenshot of the UI

I also don't like that additional combo box there. We already have way too many buttons in that bar. This feature is useful for around 0.01% of our users...

In D4981#94034, @kfunk wrote:

I also don't like that additional combo box there. We already have way too many buttons in that bar. This feature is useful for around 0.01% of our users...

That's a bit of a bold statement, remember that the main reason for implementing the feature now is uploading useful patches to Phabricator. Even if we assume that part of the current users of the ReviewBoard plugin will adapt to Phab's different approach and upload locally committed patches (once that' possible from within KDevelop) another part of that population will continue to work with diffs made from uncommitted local changes. That much is more or less clear from upstream exchanges on the topic (I myself will probably use both modes).

Either way, do you have a better location where the control can still be accessed and it's clear the thing even exists? If there are too many buttons one should maybe reconsider the 4 left-most buttons. I've never once even had the idea of using those, and I use the patchreview toolview a lot.

plugins/perforce/perforceplugin.h
91 ↗(On Diff #12325)

I see why it's not needed (doh), but what does POD stand for?

plugins/subversion/kdevsvnplugin.cpp
239

I could indeed just as well have passed on the parameter to diff2() as an additional argument.

I take it then that you wouldn't be in favour of making the contextLines parameter a member variable of the base class, with a setter function so that it doesn't have to be passed around regardless of whether an implementation actually support it?

vcs/interfaces/ipatchsource.h
53

How about the alternative virtual void update(int contextLines=-1) = 0;?

Or else, make contextLines a member variable with a setter.

vcs/widgets/vcsdiffpatchsources.h
45

Here too one could make the parameter a member variable with setter method instead of an argument. It probably makes even more sense here than elsewhere.

rjvbb updated this revision to Diff 12373.Mar 10 2017, 7:14 PM
rjvbb edited edge metadata.
rjvbb marked 10 inline comments as done.

new revision incorporating most suggested changes.

I've implemented my own suggestion to add a contextLines variable with setter (and getter, to be complete) for VCSDiffUpdater. I think the same approach for IBasicVersionControl would reduce the complexity or at least the amount of changes. A priori the patchreview toolview could set the desired number of contextlines, and that parameter would then no longer need to be passed down through possibly multiple function calls; it'd be available in those backends that decide to (can) support the feature.

rjvbb updated this revision to Diff 12374.Mar 10 2017, 8:25 PM

I've had some time on my hand, so here's a version of the patch that implements the idea setters in IBasicVersionControl and iPatchSource instead of handing down the contextLines argument via function arguments.

This makes the patch about 100 lines shorter, possibly more if I drop the changes to some of the VCS plugins.

rjvbb updated this revision to Diff 12375.Mar 10 2017, 8:26 PM

(previous upload was due to operator error)

apol added a comment.Mar 10 2017, 8:33 PM

Guys, why? Why do we need this in the UI? Doesn't the user have enough things to worry about already?

In D4981#94255, @apol wrote:

Guys, why? Why do we need this in the UI? Doesn't the user have enough things to worry about already?

In the patchreview toolview? Let me see

  • there are 2 buttons (prev/next difference) that don't do anything, is that my build that's broken or should I report this?
  • above all don't touch the "Run Tests" button

I really don't see any other place to put the control, and omitting a control makes the feature useless.

I'd be happy to hide the spinbox by default and unhide it only when an option is selected in the application settings, but there's no logical place to put that option currently.

apol added a comment.Mar 11 2017, 12:43 AM
In D4981#94268, @rjvbb wrote:

I'd be happy to hide the spinbox by default and unhide it only when an option is selected in the application settings, but there's no logical place to put that option currently.

Just make git patches generate more context, which is what you're trying to fix.

rjvbb added a comment.EditedMar 11 2017, 9:07 AM

Just make git patches generate more context, which is what you're trying to fix.

How would you yourself take it if those patches all of a sudden included the whole file even for a typo correction? If I found that an acceptable approach I would have proposed it myself.

The only alternative I see ATM is to replace the spinbox with some kind drop-down menu that contains a smallish selection of appropriate context size (1-10, All?). Could be hidden under the update button but I have no idea though how you'd implement that. Could also be put in the context menu of the patchfile document, but that and any other approach will probably introduce a lot of unnecessary complexity. An item in the toplevel menu could also do the trick AFAIAC, the View menu maybe?
Frankly, do you all use such narrow windows that the additional spinbox (or a small button-with-dropdown) is something that really changes anything for you? How often do you even pay attention to the Update button and anything to the left of it?

I notice just now that the patchreview toolview also has a "Cancel Review" button when you open it via the "Patch Review" toolbar button. That one disappears as soon as you open the toolview via "<VCS>/show differences" and never comes back. Between that and the "Run Tests" button which IMHO has nothing to do with patch reviewing (and thus should be moved elsewhere if it's to be useful from within a patch review) the PR GUI could indeed do with some cleanup.

Let's see what other reactions I get on the forum poll, beyond the 4 early votes it got that support my approach perfectly.

rjvbb updated this revision to Diff 12382.Mar 11 2017, 1:56 PM

This revision removes the spinbox from the UI and replaces it with a limited-choice contextmenu hidden under the Update button itself. A tooltip informs the user of the feature's existence.

I haven't yet removed the spinbox hooks, and I'll look into triggering the contextmenu via a tap-and-hold too (just for giggles but could be nice for us Mac users who generally don't have a physical right mouse button).

rjvbb updated this revision to Diff 12383.Mar 11 2017, 2:12 PM

don't leak QActions and add a title to the diff context contextmenu

rjvbb updated this revision to Diff 12389.Mar 11 2017, 4:20 PM

this adds a proof-of-concept implementation how to open a button's contextmenu in reaction to a click-and-hold "gesture".

Nothing I'm married to, it was fun figuring out how to do this kind of thing.

rjvbb updated this revision to Diff 12703.Mar 22 2017, 9:23 PM

patch rebased on the 5.1 branch

rjvbb updated this revision to Diff 18427.Aug 20 2017, 8:51 AM

patch updated for the 5.1 branch and now also supporting branch diffs.

rjvbb edited the summary of this revision. (Show Details)Aug 20 2017, 9:02 AM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R33 KDevPlatform.
mwolff resigned from this revision.Nov 19 2017, 9:53 PM
kossebau resigned from this revision.Nov 24 2017, 11:37 PM
rjvbb updated this revision to Diff 68452.Oct 21 2019, 4:04 PM

Rebased for the 5.4 branch.