Review Request: Generate accessors (getters/setters) for C++

Wed Jan 11 10:17:18 UTC 2012

> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.h, line 62
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46366#file46366line62>
> >
> >     on_foo_clicked() is not very Qt-like, call it:
> >     
> >     selectAllClicked()
> >     
> >     same for the functions below
> 
> Jonas Jacobi wrote:
>     the naming is for the qt auto signal slot connections, so i don't have to do them manually
> 
> Milian Wolff wrote:
>     qt auto signal slot connections? what is that? can you please show me a link? I still don't like the names though.
> 
> Jonas Jacobi wrote:
>     http://developer.qt.nokia.com/doc/qt-4.8/designer-using-a-ui-file.html#automatic-connections

nice, something new to learn every day :)

yet still, this is so far nowhere used in kdevelop/kdevplatform and as such I'd refrain from using it. It might be a bit faster to write down, but the manual way allows for much nicer slot names and is more explicit and probably easier for others (like me :)) to comprehend

> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.cpp, line 60
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46367#file46367line60>
> >
> >     please change the ui names, don't put the type (txt, chk, rdo, ...) into the name
> >     
> >     stripPrefixes alone is fine enough
> 
> Jonas Jacobi wrote:
>     What is the reasoning behind this?
>     If you read unknown code (without having to take a look at the ui file), you get a (simple) idea of the ui, without actually looking at it.
>     The semantics of for example setChecked() depends on the widget being a checkbox or radiobutton and you know what happens, just by reading the name.
>     But, if KDevelops policy is to not have such prefixes, i'll remove them, of course.

indeed you are right - even I did this - sorry!

I just want to ask: what is rdo? This might need a better name (at least I don't get it - i.e. it doesn't seem to be intuitive)

My personal naming scheme is more like this:

fooBarLabel (for QLabel)
fooBarCombo (for QComboBox)
asdfLayout (for any layout)
...

this way, when writing the code you just have to remember the name, not the type. I like this better, but you can also use your own style here, just a suggestion.

> On Jan. 10, 2012, 1:58 p.m., Milian Wolff wrote:
> > languages/cpp/codegen/generateaccessorsdialog.cpp, line 369
> > <http://git.reviewboard.kde.org/r/103613/diff/2/?file=46367#file46367line369>
> >
> >     this looks wrong, why don't you just use ClassDeclaration::isPublicBaseClass ?
> >     
> >     also, this should be done somewhere else already - just grep the code. for signal/slot support e.g. we also need to know whether we have a QObject or not. So please share code, i.e. use stuff from cppduchain helpers or push existing code there and then reuse it
> 
> Jonas Jacobi wrote:
>     I don't use isPublicBaseClass, because i don't have the actual QObject type, i search the hierarchy only by name.
>     If you could tell me, how i can get the type, or at least, where to look for it, i'll gladly change the code.
> 
> Milian Wolff wrote:
>     see e.g. ExpressionVisitor::qObjectPtrType() - please move this code and the related other method(s) to cppduchain/typeutils for reuse
> 
> Jonas Jacobi wrote:
>     Ah i didn't know i could just create a new instance of a class type, i just assumed object identity is required.
>     So then i don't move code to typeutils, but just create the qobject type and use isPublicBaseClass.

no, please move the code to create the qobject type to typeutils and create a helper there - this is much cleaner. People should just be able to call isQObject(someClassType) without having to first create the QObject type manually.

- Milian

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

On Jan. 10, 2012, 12:23 p.m., Jonas Jacobi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103613/
> -----------------------------------------------------------
> 
> (Updated Jan. 10, 2012, 12:23 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> Generate accessors (getters/setters) for C++.
> 
> This patch adds the possibility to create accessor methods for class member variables via the gui.
> The dialog offers the following features:
> - shows all members of a selected class in a tree view (with getters/setters as children), if it was opened on a class member that one is pre-selected
> - only shows setters if applicable (non-const member)
> - accessor method definitions can be created inline or separate (in the fitting source file, if applicable)
> - configurable automatic creation of method names with manual override for each accessor method
> - access policy (visibility) setting global and individualy for each accessor method
> - configurable default setter parameter name with manual override for each setter (void setFoo(int myParamterName))
> - selectable getter return type (e.g. Type, Type&, const Type& for a member of type Type)
> - create setters as slots for QObject subclasses
> 
> The code is in a usable state and i would like to get some feedback on the dialog, especially on usability improvements.
> 
> Regards,
> Jonas
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt a1fb97c 
>   languages/cpp/codegen/generateaccessorsdialog.h PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsdialog.cpp PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsmodel.h PRE-CREATION 
>   languages/cpp/codegen/generateaccessorsmodel.cpp PRE-CREATION 
>   languages/cpp/codegen/simplerefactoring.h b2187c3 
>   languages/cpp/codegen/simplerefactoring.cpp fd62ccf 
>   languages/cpp/codegen/ui/cppgenerateaccessors.ui PRE-CREATION 
>   languages/cpp/cppduchain/sourcemanipulation.h 6f8a79b 
>   languages/cpp/cppduchain/sourcemanipulation.cpp 3f40eb2 
>   languages/cpp/tests/CMakeLists.txt 40d07b0 
>   pics/mini/private_slot.png PRE-CREATION 
>   pics/mini/protected_slot.png PRE-CREATION 
>   pics/mini/public_slot.png PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103613/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/103613/s/413/
> 
>   http://git.reviewboard.kde.org/r/103613/s/412/
> 
> 
> Thanks,
> 
> Jonas Jacobi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120111/687cf750/attachment.html>