Add Kompare as compare application with KIO support
ClosedPublic

Authored by abika on Oct 27 2019, 5:24 PM.

Details

Summary

It can use the original KIO URLs. No need for using temporary local files.
And remove the obsolete space character check for file paths for KDiff3.

CCBUG: 401064

Test Plan
  • Tested opening two smb:// remote files in Kompare with "Compare by content" feature
  • Tested opeing two local files with spaces in filename and folder path in KDiff3
  • Tested opeing two smb:// remote files with spaces in filename and folder path in KDiff3

Diff Detail

Repository
R167 Krusader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
abika requested review of this revision.Oct 27 2019, 5:24 PM
abika created this revision.
abika added a subscriber: Krusader.
yurchor added a subscriber: yurchor.Nov 2 2019, 7:02 PM

Tested to work as expected. Thanks for your work.

+1

pino requested changes to this revision.Nov 2 2019, 10:48 PM
pino added a subscriber: pino.
pino added inline comments.
krusader/krslots.cpp
98

static please

218–219

why not QList::contains()?

also, the checks for spaces in url1 and url2 are removed, which seems another unrelated change

228–231

please avoid unrelated changes

240–243

ditto

This revision now requires changes to proceed.Nov 2 2019, 10:48 PM
abika updated this revision to Diff 69535.Nov 10 2019, 4:26 PM
  • fixup! Add Kompare as compare application with KIO support
abika marked 2 inline comments as done.Nov 10 2019, 4:31 PM
abika added inline comments.
krusader/krslots.cpp
218–219

why not QList::contains()?

eh, of course.

also, the checks for spaces in url1 and url2 are removed, which seems another unrelated change

See the Git comment/Differential summary. It is not necessary anymore.

228–231

Although this is unrelated, these are safe changes nearby which make the code a bit cleaner. -> "Boy Scout rule"

pino requested changes to this revision.Nov 10 2019, 5:26 PM

Mostly OK from my POV, just please remove the unrelated changes to the patch (i.e. the brackets additions in two places).
I have no idea what "Boy Scout rule" is supposed to mean, however adding unrelated changes makes history reading harder, especially when wanting to check why certain changes were done. Again, material for a different patch than this.

This revision now requires changes to proceed.Nov 10 2019, 5:26 PM
In D24987#560752, @pino wrote:

I have no idea what "Boy Scout rule" is supposed to mean

The Boy Scout Rule can be summarized as: Leave your code better than you found it.

asensi added a subscriber: asensi.Jan 16 2020, 11:20 PM

Meanwhile the other issues are talked about, I performed some tests under Kubuntu 18.04 LTS and Kubuntu 19.10 and the resulting code worked. Thanks, Alex, Yuri and Pino!

abika updated this revision to Diff 74427.Jan 27 2020, 2:18 PM
abika marked an inline comment as done.
  • fixup! Add Kompare as compare application with KIO support
abika added a comment.Jan 27 2020, 2:25 PM

I removed the two added brackets again.

I will separate those code cleanup changes into different commits in the future.
But having different code reviews for total minor changes like this is unpractical. I have time for Krusader only every few weeks and this is too much overhead. The better choice then is to not do them anymore at all.

asensi accepted this revision as: asensi.Jan 27 2020, 11:22 PM

The new version of the proposal keeps being acceptable, of course :-)

In the meantime, the not directly related changes were applied in https://phabricator.kde.org/D27609.

Dear developers,

As code reviews in Phabricator are not going to be migrated to Gitlab (https://marc.info/?l=kde-core-devel&m=158909698416266&w=2), the commented issues were solved and this review is accepted since a long time ago: I'm going to do the git push if there is no objection. Thanks for all the developing and testing!

gengisdave accepted this revision.Fri, May 15, 7:38 AM
gengisdave added a subscriber: gengisdave.

+1

This revision was not accepted when it landed; it landed in state Needs Review.Fri, May 15, 11:51 PM
Closed by commit R167:d8a5556e65ab: Add Kompare as compare application with KIO support (authored by Alexander Bikadorov <alex.bikadorov@kdemail.net>, committed by asensi). · Explain Why
This revision was automatically updated to reflect the committed changes.

Taking into account the latest messages here, and after several months of using this helpful commit, I've published the commit before the Phabricator to Gitlab migration started. Please, feel free to suggest or improve whatever you consider.