> On 2010-11-08 19:05:15, Albert Astals Cid wrote: > > Just a quick review, did not actually look at the code, sorry.
Any pair of eyes is greatly appreciated :) > On 2010-11-08 19:05:15, Albert Astals Cid wrote: > > trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.cpp, line 25 > > <http://svn.reviewboard.kde.org/r/5794/diff/1/?file=40765#file40765line25> > > > > Why kdeui includes have "" and kdecore <>? I adopted that rule because kdeui is the library the dialog belongs to, and I've interpreted the policy from http://techbase.kde.org/Policies/Library_Code_Policy#As_library_developer as I figured it would apply here. > On 2010-11-08 19:05:15, Albert Astals Cid wrote: > > trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationdialog.cpp, line 174 > > <http://svn.reviewboard.kde.org/r/5794/diff/1/?file=40765#file40765line174> > > > > kdelibs coding style says { should be on the same line than if Good catch, fixing. I've been following KDElibs policies but sometimes the power of habit kicked in :) > On 2010-11-08 19:05:15, Albert Astals Cid wrote: > > trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.cpp, line 240 > > <http://svn.reviewboard.kde.org/r/5794/diff/1/?file=40771#file40771line240> > > > > Why two lines here? I realize it's against policy, it was just to avoid a very long line. There's a lot of that in kaboutdata.cpp as well, with the return type on its own row, it was like this before I touched it. Not sure what to do about it, should I fix it all? > On 2010-11-08 19:05:15, Albert Astals Cid wrote: > > trunk/KDE/kdelibs/kdeui/dialogs/kaboutapplicationpersonmodel_p.h, line 79 > > <http://svn.reviewboard.kde.org/r/5794/diff/1/?file=40770#file40770line79> > > > > Seems like this "Other" should be translateable. Definitely! Also "Blog" and "Homepage". Fixing. - Teo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5794/#review8558 ----------------------------------------------------------- On 2010-11-08 20:58:33, Teo Mrnjavac wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://svn.reviewboard.kde.org/r/5794/ > ----------------------------------------------------------- > > (Updated 2010-11-08 20:58:33) > > > 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"), "randomd...@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 > >
