Review Request 112527: Clean up KEmoticons framework (prior to splitting)

[David Faure]

> On Sept. 8, 2013, 8:45 a.m., David Faure wrote:
> > staging/kemoticons/src/core/kemoticonsprovider.cpp, line 146
> > <http://git.reviewboard.kde.org/r/112527/diff/4/?file=188119#file188119line146>
> >
> >     file.fileName() is the same as emo. Yes, the name of the methods in QFile are a bit confusing.
> >     
> >     If this code was meant to extract the actual filename only (/tmp/foo.png -> foo.png) then you have to use QFileInfo::fileName().
> >     
> >     Hence my questions about absolute or relative paths, I don't actually know what "emo" contains in this method.
> 
> David Gil Oliva wrote:
>     As I said before, emo is an absolute path.

OK then this line of code is broken :)

file.fileName() is the same as emo, i.e. an absolute path.
Concatenating that to themePath + '/' makes no sense, you'd end up with /usr/path/to/theme/usr/full/path/to/emoticon.


> On Sept. 8, 2013, 8:45 a.m., David Faure wrote:
> > staging/kemoticons/src/core/kemoticonsprovider.h, line 85
> > <http://git.reviewboard.kde.org/r/112527/diff/4/?file=188118#file188118line85>
> >
> >     absolute path, or relative path?
> 
> David Gil Oliva wrote:
>     All of them are absolute. Should I specify it in the docs? Or should I modify the code so that it also accepts relative paths and converts them in absolute?

I guess I got confused by the broken line of code below. Path in KDE usually means absolute, yes.

Add "full" before "path", or leave it as is.

Definitely don't add support for relative paths if this wasn't there before, it means the base dir is unclear :)


- David


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/112527/#review39561
-----------------------------------------------------------


On Sept. 8, 2013, 12:58 a.m., David Gil Oliva wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112527/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2013, 12:58 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Description
> -------
> 
> Clean up KEmoticons framework (prior to splitting)
> 
> --Clean up includes
> --Turn KEmoticonsProvider abstract and reorganize some code
> --Revise documentation
> --Use QScopedPointer where useful
> --Port away from KIO in KEmoticonsProvider
> --Put const where fit
> 
> TODO:
> --Port away from KServiceTypeTrader. Sebas is working on a way to get
> the plugin list without KServiceTypeTrader.
> 
> 
> Diffs
> -----
> 
>   staging/kemoticons/src/core/kemoticons.h 57912b550c5e941f751d93c084b37764635e11c7 
>   staging/kemoticons/autotests/kemoticontest.cpp 0ca1671d26970998c13022daa839e1aae4907220 
>   staging/kemoticons/src/core/kemoticons.cpp 317089ed94179aac2b7e448414df930dcfd0c0dd 
>   staging/kemoticons/src/core/kemoticonsprovider.h 3f43ca4c8a1fe74ce01a1aac1abdeb84b9da08d2 
>   staging/kemoticons/src/core/kemoticonsprovider.cpp 1fcad79bd49919058bb21b262b57b7a9bb6ac5c9 
>   staging/kemoticons/src/core/kemoticonstheme.h d0e1a899e2f8a89b34e6337848c1aeab7f60ef88 
>   staging/kemoticons/src/core/kemoticonstheme.cpp cd385fde6095b2da930de9ec03a501ae3d1ab0f7 
>   staging/kemoticons/src/providers/adium/adium_emoticons.cpp f465b64e230639f16ca073ed7ab08a79f04afd4a 
>   staging/kemoticons/src/providers/kde/kde_emoticons.cpp 7d777b5aa0a869a0009814bb43f84f802444d5d6 
>   staging/kemoticons/src/providers/pidgin/pidgin_emoticons.cpp 65b2c5e979eeccbc98986432962e3ab05f39ca57 
>   staging/kemoticons/src/providers/xmpp/xmpp_emoticons.cpp 2535b71263f468dacc830e7cf92fead5e61528e8 
>   staging/kemoticons/tests/main.cpp f46265e97243aa776f901120d832368154c5c2ed 
> 
> Diff: http://git.reviewboard.kde.org/r/112527/diff/
> 
> 
> Testing
> -------
> 
> It compiles and test passes
> 
> 
> Thanks,
> 
> David Gil Oliva
> 
>

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