> 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.
> 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. - Benjamin ----------------------------------------------------------- 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 > >
_______________________________________________ rekonq mailing list [email protected] https://mail.kde.org/mailman/listinfo/rekonq
