Review Request 109188: Moved actions and groups creation into separate functions in main widget

[Roman Nazarenko]

> 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
> 
> Roman Nazarenko wrote:
>     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().
> 
> David Edmundson wrote:
>     > really a bad practice to allocate objects implicitly
>     Sort of, it's bad practice to have items leaking everywhere. 
>     
>     As the KAction* takes MainWidget* as the parent it's handled.
>     QMenu has addAction() methods that create action objects, for example.
>     
>     
>     and a method called "createAction" isn't really implicit :)
>     
>     you could have two versions of the createAction() one that takes the isChecked and one that doesn't.
>     
>     createAction(const QString& text, QObject *signalReceiver, const char *slot)
>     {
>        KAction *action = new KAction(text);
>        connect(....)
>        return action;
>     }
>     
>     createAction(const QString& text, QObject *signalReceiver, const char *slot, bool isChecked)
>     {
>        KAction* action = createAction(text, signalRecevier, slot);
>        action->setCheckable(true);
>        action->setChecked(isChecked);
>        return action;
>     }
>     
>     (especially as if I read this correctly right now now isChecked is uninitialised for a lot of the actions. Not a problem, but not good either)

Sounds reasonable, I updated diff


> 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.
> 
> Roman Nazarenko wrote:
>     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.
> 
> David Edmundson wrote:
>     We're adding more code in order to remove code. There's more green lines than red lines in the diff.
>     That implies we can improve it.

Sources size decrease is not our goal. We all have large storages, so, the main goals are readability and maintainability, sources size really doesn't matter.


- Roman


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


On Feb. 28, 2013, 7:19 a.m., Roman Nazarenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109188/
> -----------------------------------------------------------
> 
> (Updated Feb. 28, 2013, 7:19 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130228/3c38b43e/attachment-0001.html>