D16830: WIP: Tabbar: Add 'File Properties' menu item and three more (Rename, Delete and Compare)

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> katefileactions.cpp:51
> +    const QString oldFileName = doc->url().fileName();
> +    bool ok;
> +

RAII: here we request a resource without initialization. Correct is:

  bool ok = false;

Even if the bool is set in the next line via out parameter. Over time the code may change and then we have an uninitialized variable. :)

> katefileactions.cpp:93
> +
> +    const auto& url = doc->url();
> +

Here, I would prefer

  const auto && url = ...

Reasoning: && uses a reference if the returned value is a reference, otherwise a copy. In this case, it's more what we want in long term (aka it's always correct, even if the API changes).

> katefileactions.cpp:138
> +
> +    KProcess process;
> +    process << diffExecutable << documentA->url().toLocalFile() << documentB->url().toLocalFile();

Can't you use QProcess? :) Wasn't KProcess close to deprecated? I think KProcess is only required in rare corner cases nowadays.

REPOSITORY
  R40 Kate

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

To: gregormi, #kate
Cc: dhaumann, mmustac, ngraham, kwrite-devel, michaelh, demsking, cullmann, sars
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kwrite-devel/attachments/20181120/73f91926/attachment.html>