D4981: patchreview : allow choice of the number of context lines

[René J.V. Bertin]

rjvbb added a comment.


  In https://phabricator.kde.org/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.

INLINE COMMENTS

> kfunk wrote in perforceplugin.h:91
> Ditch the `const`. Not needed for POD types.

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

> kfunk wrote in kdevsvnplugin.cpp:240
> Why does this store `contextLines` into a member? That's totally cumbersome from a API POV.

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?

> kfunk wrote in 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.

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

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

> kfunk wrote in vcsdiffpatchsources.h:45
> Still, odd API. `lines` is ambiguous, too, should be `contextLines` if at all.

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.

REVISION DETAIL
  https://phabricator.kde.org/D4981

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, apol, kdevelop-devel, #kdevelop, Pilzschaf, akshaydeo, surgenight, arrowdodger
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170310/059b4836/attachment.html>