> 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. > > Andrea Diamantini wrote: > 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?
I think I wouldn't go for 2), I believe it'd make sense to have more than one manager instance in some cases (e.g. private browsing could use a temporary one to throw away and not save to file.) - Pierre ----------------------------------------------------------- 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
