[Kde-pim] Review Request 109825: Display KNotification and KStatusNotifier for Facebook notifications

[Martin Klapetek]

> On April 2, 2013, 5:53 p.m., Kevin Krammer wrote:
> > resources/facebook/CMakeLists.txt, line 18
> > <http://git.reviewboard.kde.org/r/109825/diff/1/?file=127841#file127841line18>
> >
> >     the resource's identifier is akonadi_facebook_resource, any reason this is _agent?

Not really, I copied it from other resource, which is actually agent. I'll change to _resource.


> On April 2, 2013, 5:53 p.m., Kevin Krammer wrote:
> > resources/facebook/serializer/akonadi_serializer_socialnotification.cpp, line 101
> > <http://git.reviewboard.kde.org/r/109825/diff/1/?file=127845#file127845line101>
> >
> >     this is actually a bugfix, right?
> >     maybe do this as a separate commit to 4.10 branch and merge to master so it is properly independent from the feature commit?

Yes, that was my plan :)


> On April 2, 2013, 5:53 p.m., Kevin Krammer wrote:
> > resources/facebook/facebookresource_notifications.cpp, line 231
> > <http://git.reviewboard.kde.org/r/109825/diff/1/?file=127844#file127844line231>
> >
> >     this also looks like something that would make sense as a plural form string

This string will never be displayed if .size() == 1, does it still make sense to use i18np()?


> On April 2, 2013, 5:53 p.m., Kevin Krammer wrote:
> > resources/facebook/facebookresource_notifications.cpp, line 265
> > <http://git.reviewboard.kde.org/r/109825/diff/1/?file=127844#file127844line265>
> >
> >     if you remove an element at i, shouldn't you check i again?
> >

Yeah, you're right. I'll replace it with iterator.


- Martin


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


On April 2, 2013, 4:47 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109825/
> -----------------------------------------------------------
> 
> (Updated April 2, 2013, 4:47 p.m.)
> 
> 
> Review request for KDEPIM and Plasma.
> 
> 
> Description
> -------
> 
> This patch displays a KNotification whenever an unread notification exists on Facebook. This KNotification groups at most 3 notifications into one popup and then say "...and N more" if there is more. It also keeps track of which notifications were already displayed and does not display them again unless they were updated on the server. This is all stored in a separate config file. 
> 
> Then it creates a KSNI for the notifications where it always show the newest three notifications in the tooltip (regardless if it was displayed before or not) and creates a menu with the notifications, which opens browser with the notification link.
> 
> 
> Diffs
> -----
> 
>   resources/facebook/CMakeLists.txt e8c6381 
>   resources/facebook/facebookresource.h 4a16c0c 
>   resources/facebook/facebookresource.cpp 67e8f3b 
>   resources/facebook/facebookresource_notifications.cpp 7f6b8c4 
>   resources/facebook/serializer/akonadi_serializer_socialnotification.cpp a261e14 
>   resources/facebook/settingsbase.kcfg 9f8e4b5 
>   resources/facebook/settingsdialog.cpp bfb7826 
>   resources/facebook/settingsdialog.ui 68b6a24 
> 
> Diff: http://git.reviewboard.kde.org/r/109825/diff/
> 
> 
> Testing
> -------
> 
> Yes.
> 
> 
> File Attachments
> ----------------
> 
> KSNI
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/04/02/facebook_notifications.png
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/