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

[Kevin Funk]

kfunk requested changes to this revision.
kfunk added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> perforceplugin.h:91
>                             const KDevelop::VcsRevision& dstRevision,
> +                           const int contextLines=-1,
>                             KDevelop::VcsDiff::Type = KDevelop::VcsDiff::DiffUnified,

Ditch the `const`. Not needed for POD types.

> kdevsvnplugin.cpp:240
> +    if (contextLines > 0) {
> +        m_contextLines = contextLines;
> +    }

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

> kdevsvnplugin.h:158
>      ThreadWeaver::Queue* m_jobQueue;
> +    int m_contextLines=3;
>  };

Makes no sense to have this as member

> ibasicversioncontrol.h:197
>                            const VcsRevision& dstRevision,
> +                          const int contextLines=-1,
>                            VcsDiff::Type = VcsDiff::DiffUnified,

Dito, ditch `const`

> ipatchsource.h:53
> +        ///The default implementation ignores the argument and calls update()
> +        virtual void update(int) {
> +            update();

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.

> vcsdiffpatchsources.h:45
>      virtual ~VCSDiffUpdater();
> -    virtual KDevelop::VcsDiff update() const = 0;
> +    virtual KDevelop::VcsDiff update(int lines=-1) const = 0;
>      virtual KDevelop::IBasicVersionControl* vcs() const = 0;

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

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/d85464e0/attachment-0001.html>