Review Request 120554: Initial frameworks port of kompare

Thu Oct 16 23:47:35 BST 2014

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120554/#review68582
-----------------------------------------------------------

See my point by point review below. Once all the issues are addressed, we should be all set, but I'll have another look before giving it a "Ship It".

CMakeLists.txt
<https://git.reviewboard.kde.org/r/120554/#comment47766>

    No app icon anymore on the proprietary system(s) using those?

    Looks like the discussion is still ongoing on what to do with that: http://lists.kde.org/?t=140784609900004&r=1&w=2 so I guess we'll have to wait for its outcome. :-( (Thus, no issue raised here.)

komparenavtreepart/komparenavtreepart.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47768>

    Please just delete this obsolete macro entirely.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47769>

    This should pass at least QUrl::RemoveUserInfo to toString(), we don't want to echo passwords in error messages.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47770>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47771>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47772>

    See above. (In this case, you'll also need to add the explicit .toString(QUrl::RemoveUserInfo) call.)

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47773>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47774>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47775>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47776>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47777>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47778>

    See above.

komparepart/kompare_part.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47779>

    See above.

komparepart/kompareprefdlg.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47780>

    If OK is automatically the default button, just delete this line.

    If it is not, then please replace it with something that does the job, e.g.:
    button( QDialogButtonBox::Ok )->setDefault( true );

komparepart/kompareprefdlg.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47781>

    There goes the button separator.

    If button separators are generally unwanted now (as seems the case, judging from the porting script that just deletes the showButtonSeparator calls), we should just remove the line entirely rather than commenting it out. If they're wanted, we should find a way to show them.

    Looking at the dialog in my KDE 4 Kompare, I think the separator is not really needed to begin with, so you can just delete the commented-out line.

kompareurldialog.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47767>

    There goes the button separator.

    If button separators are generally unwanted now (as seems the case, judging from the porting script that just deletes the showButtonSeparator calls), we should just remove the line entirely rather than commenting it out. If they're wanted, we should find a way to show them.

    Looking at the dialog in my KDE 4 Kompare, I think the separator is not really needed to begin with, so you can just delete the commented-out line.

libdialogpages/diffpage.cpp
<https://git.reviewboard.kde.org/r/120554/#comment47783>

    This one ought to be toLocalFile, I think. Or at least pass QUrl::PreferLocalFile to toString(), but I don't think a URL makes sense as a diff program, does it?

- Kevin Kofler

On Okt. 16, 2014, 5:24 vorm., Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120554/
> -----------------------------------------------------------
> 
> (Updated Okt. 16, 2014, 5:24 vorm.)
> 
> 
> Review request for kdelibs and Kevin Kofler.
> 
> 
> Repository: kompare
> 
> 
> Description
> -------
> 
> I spent a bit of time porting kompare to kf5. It builds and runs and compares files and folders but I'm pretty sure I missed something. 
> 
> I'll reread my changes also but wanted to get this out there to be played with also.
> 
> 
> Diffs
> -----
> 
>   libdialogpages/viewpage.cpp e26d72843587cf4ed60f0d7dcde51ec4f19aad29 
>   libdialogpages/viewsettings.h 305e934815175c0fc60c8a045070e48f7b932935 
>   main.cpp ac68e2421c1f02460e732eb4dc7f79036df9db2e 
>   pics/CMakeLists.txt 512f6e8cad202bc592c08531654754bd311fcb5e 
>   pics/hi128-app-kompare.png  
>   pics/hi16-app-kompare.png  
>   pics/hi22-app-kompare.png  
>   pics/hi32-app-kompare.png  
>   pics/hi48-app-kompare.png  
>   pics/hisc-app-kompare.svgz  
>   libdialogpages/pagebase.cpp 4aa33d7d5b8eb6779bb96e5533d0f11235c30aac 
>   libdialogpages/filespage.cpp 417fbd12b0f7622da23d0da0e934476d142df149 
>   libdialogpages/CMakeLists.txt 22906650d1f0f8fb0b5d8d3d272f09d44bf7408c 
>   libdialogpages/diffpage.cpp 94221ca8badbd1773ff48071fd558bd111750e47 
>   komparepart/komparesaveoptionswidget.cpp 06530d85159305fc1330f495a1c52b0155e45e37 
>   komparepart/komparesplitter.h 11a344f29f46d68ca5418c770bd5e502d527e0fe 
>   komparepart/komparesplitter.cpp 2848f881992bae0b0e7141c1f6c47a2239211844 
>   komparepart/kompareview.h 93ea0644a590c56e600e466a69bf227dc93328b1 
>   kompareurldialog.cpp 561dd4518dda0be64198beff56e986da4294fe2b 
>   komparenavtreepart/CMakeLists.txt da52bc7d0d9f032d80f6f2257dbbed1f6fb0e81a 
>   komparenavtreepart/komparenavtreepart.h eb08329be477febe93b4ca7a8c787656abbfc68f 
>   komparenavtreepart/komparenavtreepart.cpp d3bdc93ddaf28e026b7c1847b8d4f6dbc46125ee 
>   komparepart/CMakeLists.txt ee83458a3034c3fb873629d650efe5668955900b 
>   komparepart/kompare_part.h 0c4d3dd40ca32e07b2402280539d03f155555cfc 
>   komparepart/kompare_part.cpp 08df1dc0985391908eb81da9c4cfdd0836cd4b23 
>   komparepart/kompareconnectwidget.h 03eb746c24dc3899b64d3907ae21e0de656e369f 
>   komparepart/kompareconnectwidget.cpp 2a8cb920280f2b42ab09e7962a441529b8cdfc0c 
>   komparepart/komparelistview.cpp b2935c917541984532814d301b6a7f5bdd661c72 
>   komparepart/kompareprefdlg.cpp 0b18696acf270cf5a0351312aa3ffe13eff9b9e6 
>   komparepart/komparesaveoptionswidget.h 9c49815b1b95b9448eb5fccda35e4c7c7fb1e2f1 
>   kompare_shell.h de099ffbcc92a22a4374ad6cfca0bccc6b0e97bc 
>   kompare_shell.cpp 9d22085780fbbffcb9b480cbb16c30e73c0ba71e 
>   CMakeLists.txt 86e4504ad3ae06519cbfaaf35781238f5f234857 
>   doc/CMakeLists.txt 06d898738aabdfc947e89de848e2fbe903d5e6cc 
>   interfaces/CMakeLists.txt 4bb0c6c53e8b995f1c7350cd02268e2e05ddb38a 
>   interfaces/kompareinterface.h a28d209b058fb06cc970e6ba3538ace721319be5 
> 
> Diff: https://git.reviewboard.kde.org/r/120554/diff/
> 
> 
> Testing
> -------
> 
> It builds, runs and seems to wok ok comparing files and folders. The QFileDialog it uses wasn't showing files I expected to see though, may need to play with the filters etc.
> 
> Also ported from KGlobal::ref() and KGlobal::unref() to QEventLoopLocker, though quitting one window closes all windows, not sure if that's expected or not.
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20141016/abb2c4fa/attachment.htm>