[Differential] [Requested Changes To] D3073: Feature "Diff From Branch" on BranchManager

[kfunk (Kevin Funk)]

kfunk requested changes to this revision.
kfunk added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> branchmanager.cpp:202
> +    {
> +        KMessageBox::messageBox(this, KMessageBox::Sorry, i18n("Already on branch \"%1\"\n", src));
> +    } else {

Coding style: Use early return, e.g.:

  if (!success) {
    // warning
    return;
  }
  
  // normal case

> branchmanager.cpp:222
> +
> +    if (vcsjob->status() == KDevelop::VcsJob::JobSucceeded) {
> +        KDevelop::VcsDiff d = vcsjob->fetchResults().value<KDevelop::VcsDiff>();

Coding style: Use early return, e.g.:

  if (!success) {
    // warning
    return;
  }
  
  // normal case

> branchmanager.cpp:223
> +    if (vcsjob->status() == KDevelop::VcsJob::JobSucceeded) {
> +        KDevelop::VcsDiff d = vcsjob->fetchResults().value<KDevelop::VcsDiff>();
> +        if(d.isEmpty())

Style: No one-char variable names. `d` -> `diff`

> branchmanager.cpp:229
> +            else {
> +                VCSDiffPatchSource* patch=new VCSDiffPatchSource(d);
> +                showVcsDiff(patch);

Style: `a = b`, not `a=b`.

> branchmanager.ui:55
> +       <property name="text">
> +        <string>Diff From</string>
> +       </property>

Hmm, I don't like the action text.

Maybe

- "Diff against branch"
- "Diff between branches"

Opinions?

REPOSITORY
  rKDEVPLATFORM KDevPlatform

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: coliveira, apol, kfunk
Cc: kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20161016/59486762/attachment-0001.html>