Review Request: Social About Dialog implementation for KDElibs

Teo Mrnjavac teo at kde.org
Tue Nov 9 21:05:00 GMT 2010 


> On 2010-11-09 00:33:12, Raphael Kubo da Costa wrote:
> > * It would be nice to have unit tests for the new code.
> > 
> > * See the documentation for KAboutData::setTranslator -- it is currently not possible to add any data besides the names and emails of the translators (there's even a bug report in Bugzilla about not being able to add a translator's website).
> > 
> > * Does it degrade nicely if there is no network connection?
> 
> Teo Mrnjavac wrote:
>     Yup, now I see what goes on in setTranslator - that's a whole other kind of issue, it would require at least some more API additions. Should I do it now or leave it for another occasion?
>     
>     If there is no network connection (that is, the get job fails) the behavior is the same as if the user didn't specify an OCS username i.e. the entries show only the data provided in KAboutData and look similar to how they did in the old KAboutDialog.
> 
> Raphael Kubo da Costa wrote:
>     As for the API additions for translators, I think they're not strictly related to this commit, and probably involves talking to the i18n guys.
>     
>     How about the unit tests? Every time you add new code without unit tests, dfaure kills a kitten :)

We sure don't want the kittens to die. I've never really written unit tests from scratch so it might take me a while to figure it out and do them for all the classes but I'll do my best. I'm guessing the PersonModel is the most important class to write a test for.


> On 2010-11-09 00:33:12, Raphael Kubo da Costa wrote:
> > trunk/KDE/kdelibs/kdeui/CMakeLists.txt, line 5
> > <http://svn.reviewboard.kde.org/r/5794/diff/4/?file=40868#file40868line5>
> >
> >     I did not understand the need for this -- attica is a required kdelibs dependency anyway, isn't it?
> 
> Teo Mrnjavac wrote:
>     It is, but I've been told that there's a plan to make it optional depending on the form factor of the KDElibs deployment and that it's a plus if I make the whole thing ready for that change.
> 
> Raphael Kubo da Costa wrote:
>     Hmm, but in this case maybe it's better to wait for it to happen? I mean, they may come up with patches in other cmake files, and use a different name for this.

I'm not sure what you mean, as far as I can tell KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION is already set up in CreateKDEPlatformProfile.cmake and used in Plasma's CMakeLists.txt. Maybe I'm not understanding you right. Should I just remove all that, set HAVE_ATTICA to true in kdeui's CMakeLists.txt and leave it there for later?


- Teo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5794/#review8574
-----------------------------------------------------------


On 2010-11-09 09:22:50, Teo Mrnjavac wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5794/
> -----------------------------------------------------------
> 
> (Updated 2010-11-09 09:22:50)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> The present review request contains the implementation of a Social About Dialog feature for KDE apps, and is a rewrite of Amarok's Social About Dialog (See http://teom.wordpress.com/2009/08/23/social-desktop-integration-in-kaboutdialog/ ). For those who don't have time to read through that link, the Social About Dialog is an evolution of our old KAboutDialog which adds optional Open Collaboration Services features to complete the contributors' data.
> The only changes that concern the API are in kaboutdata.{h,cpp}, and I've tried to keep it binary compatible to the best of my abilities by following the instructions on our Techbase, I hope I did it right.
> The only new dependency is the Attica library, except if we are building with KDE_PLATFORM_FEATURE_BINARY_COMPATIBLE_FEATURE_REDUCTION, in that case all social features are #ifdef'ed out and the dialog behaves very much like the old KAboutDialog.
> The dialog itself uses model/view/delegate, not very different from KNewStuff3, and most of the action happens in the model (jobs to get data from Attica/OCS) and in the delegate.
> As we're getting dangerously close to the string freeze, I've decided to put this up for review even though there are still a couple TODOs in the code. These are:
>  * The icons are placeholders, we are going to need some real icons for links and social networks. We already have some of those for Amarok's about dialog but this needs to be cleanly solved in a way that would not make us liable for IP infringement in any way. IANAL here so a discussion as to how to proceed is welcome.
>  * In the optional translators tab there's a short but not very short text about the translation team. I've added it as a label but it tends to take a bit more room than I'd like, so I'm requesting for comments on what to do with it. Also, I don't know how translators' names are added. If translators can't add their OCS usernames anyway then I'll easily revert to the old about view for them and the problem of the tall translation team text is automatically solved.
>  * Since kdeui does not depend on KIO, I couldn't use KIO::get and had to use QNAM instead. For the same reason I had to use QDesktopServices instead of KRun::runUrl() to launch URLs. As it is now, stuff works, but to make it nicer the plan could be to add an Attica::PixmapFromUrlJob (which I've been told would satisfy other unrelated needs too) to Attica to remove the single instance where QNAM is used instead of KIO.
> 
> Except maybe for the translators tab issues, none of the TODOs require string changes.
> 
> To test the feature you only need to compile KDElibs with the patch and run any KDE app which has an about dialog, however you won't be able to try the social features unless you also add OCS usernames (by default the provider is openDesktop.org) to one or more contributors in the KAboutData of a KDE app of your choice.
> For example: aboutData.addAuthor( ki18n("Random Dude") ,  ki18n("Random stuff"), "randomdude at kde.org", "http://example.com", "random-dude's-oD.o-username");
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdelibs/kdecore/kernel/kaboutdata.h 1194020 
>   trunk/KDE/kdelibs/kdecore/kernel/kaboutdata.cpp 1194020 
>   trunk/KDE/kdelibs/kdeui/CMakeLists.txt 1194020 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationconfigattica_p.h.cmake PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.h 1194020 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.cpp 1194020 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistdelegate_p.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistview_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonlistview_p.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.h PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp PRE-CREATION 
>   trunk/KDE/kdelibs/kdeui/dialogs/thumb_frame.png UNKNOWN 
> 
> Diff: http://svn.reviewboard.kde.org/r/5794/diff
> 
> 
> Testing
> -------
> 
> Testing during development, both with Attica enabled and disabled.
> 
> 
> Screenshots
> -----------
> 
> The social about dialog as it appears when OCS usernames for openDesktop.org are provided in addAuthor()
>   http://svn.reviewboard.kde.org/r/5794/s/552/
> 
> 
> Thanks,
> 
> Teo
> 
>

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

More information about the kde-core-devel mailing list