Review Request 117511: Add class for finding the kde4 config and apps home dirs.

[David Faure]

> On April 22, 2014, 9:50 a.m., Kevin Krammer wrote:
> > src/lib/util/kdelibs4migration.cpp, line 81
> > <https://git.reviewboard.kde.org/r/117511/diff/2/?file=267469#file267469line81>
> >
> >     would QStringLiteral work here?

Given the multiple implementations of QStringLiteral depending on compiler and C++11 support, I'd rather not risk it.
IIRC it uses a lambda on msvc..... sounds tricky.


> On April 22, 2014, 9:50 a.m., Kevin Krammer wrote:
> > src/lib/util/kdelibs4migration.cpp, line 82
> > <https://git.reviewboard.kde.org/r/117511/diff/2/?file=267469#file267469line82>
> >
> >     Hmm. I think that looks weird.
> >     Can this be split in the type definition (struct something) and the constant defintion?

I don't get that. If I make a list of type names, what do I do with it?
All I need is a type->subdir lookup table
(just like kstandarddirs has/had, btw)


> On April 22, 2014, 9:50 a.m., Kevin Krammer wrote:
> > src/lib/util/kdelibs4migration.cpp, line 97
> > <https://git.reviewboard.kde.org/r/117511/diff/2/?file=267469#file267469line97>
> >
> >     Also maybe just a personal taste, but I find it better to explicitly use parentheses when mixing boolean and arithmetic operators, i.e. make it explicit which operator has precendence. In this case putting parentheses around the size calculation.
> >     Or even calculating the size as a const int before the loop (can it be done as a const_expr outside the function?).
> >
> 
> Kevin Krammer wrote:
>     Or as a std:find_if()?
>     Sorry, have just watched a Going Native talk about "no raw loops" :)

std::find_if requires iterators, which I don't really have here. Plus it's not usual in KDE code (so it reduces maintainability/readability).
I moved the size as a const int before the loop (which actually improves readability).


> On April 22, 2014, 9:50 a.m., Kevin Krammer wrote:
> > src/lib/util/kdelibs4migration.cpp, line 106
> > <https://git.reviewboard.kde.org/r/117511/diff/2/?file=267469#file267469line106>
> >
> >     Do we have some logging categories for kdecoreaddons?

No. Porting to categorized logging has to be done everywhere in frameworks....


- David


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


On April 22, 2014, 9:32 a.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117511/
> -----------------------------------------------------------
> 
> (Updated April 22, 2014, 9:32 a.m.)
> 
> 
> Review request for KDE Frameworks, Ivan Čukić and Kevin Krammer.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> Add class for finding the kde4 config and apps home dirs.
> 
> To help applications migrating to the kf5/qt5 directories.
> 
> 
> Diffs
> -----
> 
>   autotests/CMakeLists.txt 2f14b3a229b07071ed6e8b0772e03ee798db6c03 
>   autotests/kdelibs4migrationtest.cpp PRE-CREATION 
>   src/lib/CMakeLists.txt 39ca3b8e9d5a4f8ffa06ca2ccf017b02ac245fd7 
>   src/lib/util/kdelibs4migration.h PRE-CREATION 
>   src/lib/util/kdelibs4migration.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/117511/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140423/105c96f9/attachment.html>