Review Request: Activities support for Gwenview

[Aurélien Gâteau]

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


Thanks for this contribution! I have a few remarks however:

1. Make libkactivity a mandatory dependency. It is now widespread enough IMO and I prefer to avoid adding too many optional dependencies: it helps reducing the number of possible compilation setups and avoid breaking build in corner-cases.

2. Instead of finding out the QGraphicsView from the scene, I would prefer you to add a "QWidget* parent" argument to DocumentView constructor and use this to initialize the ResourceInstance. You can then get rid of most of the "if (ptr)" blocks (To save you some time, DocumentView are instantiated by the DocumentViewContainer class)

3. Can you rename mResourceInstance to mActivityResource? Outside of the domain of KActivities, "ResourceInstance" sounds too generic.

4. Final nitpick, Gwenview coding style for pointers is "Foo* var;", ie: no space before '*', space after '*'. Can you fix the few "Foo * var;"?

- Aurélien Gâteau


On Oct. 2, 2012, 11:21 a.m., Ivan Čukić wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106687/
> -----------------------------------------------------------
> 
> (Updated Oct. 2, 2012, 11:21 a.m.)
> 
> 
> Review request for Gwenview and Plasma.
> 
> 
> Description
> -------
> 
> Gwenview reports the open/close document events to activity manager daemon.
> Side-effect - support for Share-Like-Connect applet.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt a73e4d3 
>   config-gwenview.h.cmake 634c677 
>   lib/CMakeLists.txt 2474ec1 
>   lib/documentview/documentview.cpp 5451062 
> 
> Diff: http://git.reviewboard.kde.org/r/106687/diff/
> 
> 
> Testing
> -------
> 
> Yes
> 
> 
> Thanks,
> 
> Ivan Čukić
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20121002/6c653118/attachment.html>