[rekonq] Re: Review Request: A bit of cleanup in Application.

[Andrea Diamantini]

> On Jan. 8, 2011, 10:13 p.m., Benjamin Poulain wrote:
> > You are gonna kill me. The previous patch was more correct actually :)
> > 
> > I did not notice those objects are parented to the instance of Application. One have to wonder why those methods are static at all.
> 
> Pierre Rossi wrote:
>     Huh indeed, that might actually crash on exit now! 
>     The static methods still seem more convenient than something like Application::instance()->historyManger() or similar calls...
>     Hard to decide which approach to keep for cleanup, I'm tempted to drop the instance() as a parent in favor of a scoped pointer because it's more generic, so if some day someone wants to add something that's not a QObject in the same fashion, it can still be done in the same way.
> 
> Andrea Diamantini wrote:
>     The static methods + members has been maintained from the "big switch" to be a singleton. I'm not actually sure on what are the implications of moving from the QWeakPointer (deleting them in dtor) to the QScopedPointer class (deleting them... when?).
>     I just think that, for cleaness of the Application class code, they should be completely removed. This anyway will force the other classes to use something like "Application::instance()->historyManager()" or similar.
>     That's it. Tell me what you think it's better.
> 
> Benjamin Poulain wrote:
>     > I'm not actually sure on what are the implications of moving from the QWeakPointer (deleting them in dtor) to the QScopedPointer class (deleting them... when?).
>     
>     Static variables are destroyed in reverse order of their creation. Which mean the objects would be destroyed after main().
>     
>     > I just think that, for cleaness of the Application class code, they should be completely removed. This anyway will force the other classes to use something like > "Application::instance()->historyManager()" or similar.
>     > That's it. Tell me what you think it's better.
>     
>     Sounds good.
>     
>     Make them explicitly dependent on the object application. We could add rApp as a shortcut to Application::instance(), similar to qApp.
>     
>     Or I guess those classes could be singleton themselves.

So, to summarize:
1) remove ALL static methods from Application
2) Let the manager classes be singleton (this way you can call them without using the instance call)
3) add an rApp shortcut (nice) to get rid of Application::instance() in the code.

Right?


- Andrea


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


On Jan. 8, 2011, 9:51 p.m., Pierre Rossi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100316/
> -----------------------------------------------------------
> 
> (Updated Jan. 8, 2011, 9:51 p.m.)
> 
> 
> Review request for rekonq and Benjamin Poulain.
> 
> 
> Summary
> -------
> 
> I believe we don't need static members in QWeakPointers for all the *Managers, static getter functions would do the job.
> 
> 
> This addresses bug N/A.
>     /show_bug.cgi?id=N/A
> 
> 
> Diffs
> -----
> 
>   src/application.h b30e337 
>   src/application.cpp 466a0a4 
> 
> Diff: http://git.reviewboard.kde.org/r/100316/diff
> 
> 
> Testing
> -------
> 
> "compiles and runs" ™
> 
> 
> Thanks,
> 
> Pierre
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/rekonq/attachments/20110109/5e66e34b/attachment.htm