> On Feb. 27, 2013, 5:34 p.m., David Edmundson wrote: > > main-widget.h, line 101 > > <http://git.reviewboard.kde.org/r/109188/diff/1/?file=116082#file116082line101> > > > > confugures -> configures > > > > > > if we convert this into > > > > KAction * createAction(const QString& text, QObject *signalReceiver, > > const char *slot, bool isChecked); > > > > which is the same as it is now, but calls "new KAction(this)" at the > > start > > > > > > It's basically the same as the ctor of ActionOptions but keeping logic > > grouped together
That was the first version I've written, exactly. I rejected it. I think, it's really a bad practice to allocate objects implicitly. We cannot say, where exactly the actions is taken from, untill we look into realization. This one is clear: it takes an action, applies options to it and returns configured action back. You don't need to read through sources to find it out. The second goal: we need to set some actions chechable before setupAction is called. According to Qt documentation, setCheckable(true) must be called before setChecked(). > On Feb. 27, 2013, 5:34 p.m., David Edmundson wrote: > > main-widget.h, line 102 > > <http://git.reviewboard.kde.org/r/109188/diff/1/?file=116082#file116082line102> > > > > this then becomes QList<KAction*> and gets simplified. Then we need to create actions outside. We also need to make it checkable outside. And the purpose of this function - avoiding 6 copy-paste code pieces in one method - will be completely ruined. > On Feb. 27, 2013, 5:34 p.m., David Edmundson wrote: > > main-widget.h, line 90 > > <http://git.reviewboard.kde.org/r/109188/diff/1/?file=116082#file116082line90> > > > > Making a struct that mimics all the properties of a KAction doesn't > > make a lot of sense to me. We're duplicating data, data duplication leads > > to problems. > > I really tried to avoid that. I know it looks like a crooked nail, but I haven't invented something more reliable. Any ideas? - Roman ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109188/#review28206 ----------------------------------------------------------- On Feb. 27, 2013, 10:29 a.m., Roman Nazarenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109188/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2013, 10:29 a.m.) > > > Review request for Telepathy. > > > Description > ------- > > There was 6 identical routines creating actions for "Contact List Type" and > "Shown contacts" menus. > I made one universal routine to do that, one routine to build those two > groups and one struct as an options tuple. > > > Diffs > ----- > > main-widget.h be7004d > main-widget.cpp 39442de > > Diff: http://git.reviewboard.kde.org/r/109188/diff/ > > > Testing > ------- > > Tried to toggle all those actions in both standard and global menus. > Have not tested "Shown contacts" since I have no blocked ones. > > > Thanks, > > Roman Nazarenko > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
