Review Request 116056: Port to Qt5 and Kf5 of dnssd ioslave and kded module

[Alex Merry]

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


Argh, sorry, I completely missed this review request, and just went ahead and created a ported frameworks branch.

I'll give you some feedback anyway, for future reference...


CMakeLists.txt
<https://git.reviewboard.kde.org/r/116056/#comment35733>

    Not needed, since this module only installs runtime code, and no headers



ioslave/CMakeLists.txt
<https://git.reviewboard.kde.org/r/116056/#comment35738>

    I found I needed to link against KF5::I18n; I wonder why you didn't?



ioslave/CMakeLists.txt
<https://git.reviewboard.kde.org/r/116056/#comment35734>

    Not needed, the KDECMakeSettings module already does this for MODULE library targets.



ioslave/dnssd.h
<https://git.reviewboard.kde.org/r/116056/#comment35735>

    The comment should have gone as well.



ioslave/dnssd.cpp
<https://git.reviewboard.kde.org/r/116056/#comment35736>

    qplatformdefs.h would add everything that was needed, and be cross-platform.



ioslave/dnssd.cpp
<https://git.reviewboard.kde.org/r/116056/#comment35737>

    QStringLiteral is better than QLatin1String for arguments that will just be cast to QString like this.  QLatin1String is more for things like equality tests with QStrings.



kdedmodule/watcher.h
<https://git.reviewboard.kde.org/r/116056/#comment35739>

    With Qt 5, we can use Q_DECL_OVERRIDE in subclasses, like
    QUrl constructUrl() Q_DECL_OVERRIDE;


Thanks for doing this work, and sorry for not using it!

In terms of what to do instead, I suggest one of:
- look into porting things in kde-runtime (see http://community.kde.org/Frameworks/Epics/New_Runtime_Organization)
- the "reduce mentions of kde 4 in source code" task from http://community.kde.org/Frameworks/Epics/KF5.0_Release_Preparation#Tasks_for_Final_Release
- look for todos and warnings in the frameworks to resolve

If you want more pointers, I and other frameworks folks are usually hanging around on #kde-devel on irc, and there's the kde-frameworks-devel email list.

- Alex Merry


On Feb. 25, 2014, 8:02 p.m., Matthieu Gallien wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/116056/
> -----------------------------------------------------------
> 
> (Updated Feb. 25, 2014, 8:02 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kdnssd
> 
> 
> Description
> -------
> 
> Basic port to Qt5 and Kf5 in order to help the merge of the two kdnssd repositories.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt df842d4 
>   ioslave/CMakeLists.txt 40c2d67 
>   ioslave/dnssd.h 89afd8d 
>   ioslave/dnssd.cpp c0c8ada 
>   ioslave/zeroconfurl.h f4f06de 
>   kdedmodule/CMakeLists.txt 6232940 
>   kdedmodule/dnssdwatcher.h a2062fc 
>   kdedmodule/dnssdwatcher.cpp 2e4dc25 
>   kdedmodule/watcher.h 5d5470b 
>   kdedmodule/watcher.cpp 21018b9 
> 
> Diff: https://git.reviewboard.kde.org/r/116056/diff/
> 
> 
> Testing
> -------
> 
> Not much. I do not know how to test the ioslave without something like dolphin.
> 
> 
> Thanks,
> 
> Matthieu Gallien
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140226/a28fbfd4/attachment-0001.html>