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

Milian Wolff mail at milianw.de
Tue Apr 10 14:06:34 UTC 2012 


> On April 8, 2012, 10:41 p.m., Milian Wolff wrote:
> > ugh I've finnaly found time and I really have to say no to the whitespace patch - it is *so* huge *and* even breaks a few lines like
> > 
> > foreach()
> >   foo;
> > 
> > turns into
> > 
> > foreach()
> > foo;
> > 
> > I'll try to rebase your "real" patch to make the first one not required but I still need something from you:  could you please use one of the "official" kde licenses? see http://techbase.kde.org/Policies/Licensing_Policy e.g. either gpl v2+ or  v2 + v3 + any later by kde e.v. - that would help me greatly.
> > 
> > I'm now going to actually test your code and take a look it - cheers and happy easter
> 
> Milian Wolff wrote:
>     oh and kcm_kdevgenerateaccessors_settings.desktop is missing in your patch!
> 
> Milian Wolff wrote:
>     on the actual usage:
>     
>     - the default should (imo) not create inlined getter/setters but rather separate ones
>     - there is no related action in the "code" menu and hence also no way to set a user-defined short cut to show the fancy dialog
>     - I put a class into a "test.cpp" and added a few data members, then triggered your assistant via the fancy dialog and selected the "separate" option. the getter/setter implementations where then put *on top* of the class instead of behin the class
>     
>     in the fancy dialog:
>     - "method names start with" has no effect on either getter/setter prefix
>     - it's not really obvious that you can edit each entry by clicking on it. your display method is pretty fancy of course but maybe something simpler would do? or always show the edit-view?
>     
>     all in all:
>     I'm pretty amazed, you even cared to properly remember the settings and that PODs don't get a const& in the setter :) really good job!
>     
>     so to get this stuff *finally* merged, we need to get rid of the huge number of whitespace changes, get the kcm file in, fix the licenses. the above nuisances I found could be tended to later then maybe?
>
> 
> Milian Wolff wrote:
>     I also think it should be configurable to ignore a few members (esp. for code completion), and set the default to "d, self" (i.e. d-ptr and singletons). the latter might be fixed by preventing setter/getters for static members which is also done right now but probably not wanted.
> 
> Jonas Jacobi wrote:
>     > could you please use one of the "official" kde licenses?
>     Mhh i think i used the default GPL 3 header provided by kdevelop on all files, what do you mean exactly?
>     
>     > - the default should (imo) not create inlined getter/setters but rather separate ones
>     ok
>     
>     > - there is no related action in the "code" menu and hence also no way to set a user-defined short cut to show the fancy dialog
>     ok, i'll check how to add that action
>     
>     > - I put a class into a "test.cpp" and added a few data members, then triggered your assistant via the fancy dialog and selected the "separate" option. the getter/setter implementations where then put *on top* of the class instead of behin the class
>     That's probably a bug in the cppduchain SourceCodeInsertion class then.
>     I can look into it.
>     
>     > - "method names start with" has no effect on either getter/setter prefix
>     Yes, i was unsure about it. It is meant to create upper case method names with _no_ prefix e.g. from member m_foo to method Foo() for all those weird M$ API style lovers.
>     It has no effect on prefixes, because you can choose your prefix to have an upper case first letter anyway.
>     Do you think it should affect prefixes, too?
>     
>     > - it's not really obvious that you can edit each entry by clicking on it. your display method is pretty fancy of course but maybe something simpler would do? or always show the edit-view?
>     The first idea was to have an always edit style, but i haven't found a way to do it with the Qt mvc stuff, so i chose to show the edit stuff on focus.
>     Do you know a way to "force" the edit view?
>     Another way could probably be to have the first item selected by default, so the edit-view is shown on that one, if that is possible.
>     
>     > I also think it should be configurable to ignore a few members (esp. for code completion), and set the default to "d, self" (i.e. d-ptr and singletons). the latter might be fixed by preventing setter/getters for static members which is also done right now but probably not wanted.
>     The filter is a very good idea.
>     What do you mean with preventing setter/getters for static members right now?
>     I just checked and can create getters/setters for static members just fine.
>     
>     I would prefer if we could get the stuff merged (after missing files included and without whitespace patch) and change the other stuff mentioned afterwards, so we don't need to use this slightly cumbersome reviewboard workflow.
>

gpl3: yes thats it, I'd rather have one of the gpl headers from our policy page (see above), and not the plain gpl3 version

prefixes: I found it inconsistent but then again I'd never use that anyways, so leave it as-is

editing: yes, selecting the first one always would be already a good idea I think

static: yes, you can create getters/setters for static members but that is imo not ever wanted. for singletons e.g. you often call the member "self" or "instance" but there is of course no setter yet still you'd now always see an code completion item to create a setter for that which is annoying. hence it should either be filtered or better yet - just ignore static members in code completion. you could still show them in the dialog if you really want to.

merging: yes, I agree. but as you said, first get rid of the huge amount of unrelated whitespace changes and add the missing files.

cheers


- Milian


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


On April 2, 2012, 1:41 p.m., Jonas Jacobi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103613/
> -----------------------------------------------------------
> 
> (Updated April 2, 2012, 1:41 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> UPDATE: I also added the possibility to create getters/setters via code completion (can be turned off in the options)
> The settings can now also be edited via the project configuration dialog.
> ---
> 
> 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
> 
> Regards,
> Jonas
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt 1577a7b 
>   languages/cpp/codecompletion/context.h a5fdea7 
>   languages/cpp/codecompletion/context.cpp 33dcad1 
>   languages/cpp/codecompletion/generateaccessorshelperitem.h PRE-CREATION 
>   languages/cpp/codecompletion/generateaccessorshelperitem.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/cppgenerateaccessors.ui PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessors.kcfg PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessors.kcfgc PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsconfiguration.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsconfiguration.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsdialog.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsdialog.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorseditor.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorseditor.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsitem.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorsitem.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorspreferenceskcmodule.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorspreferenceskcmodule.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorssettings.h PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorssettings.cpp PRE-CREATION 
>   languages/cpp/codegen/accessors/generateaccessorssettingsdialog.ui PRE-CREATION 
>   languages/cpp/codegen/simplerefactoring.h b2187c3 
>   languages/cpp/codegen/simplerefactoring.cpp 8ae4e46 
>   languages/cpp/cppduchain/sourcemanipulation.h 6f8a79b 
>   languages/cpp/cppduchain/sourcemanipulation.cpp 3f40eb2 
>   languages/cpp/cppduchain/tests/test_duchain.cpp 02ed83e 
>   languages/cpp/cppduchain/typeutils.h a686c3b 
>   languages/cpp/cppduchain/typeutils.cpp a801d47 
>   languages/cpp/tests/CMakeLists.txt 24d9597 
>   languages/cpp/tests/testaccessorsettings.h PRE-CREATION 
>   languages/cpp/tests/testaccessorsettings.cpp PRE-CREATION 
>   languages/cpp/tests/testcppgenerateaccessors.h PRE-CREATION 
>   languages/cpp/tests/testcppgenerateaccessors.cpp PRE-CREATION 
>   pics/mini/CMakeLists.txt c3316e7 
>   pics/mini/accessor_reader.png PRE-CREATION 
>   pics/mini/accessor_writer.png PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103613/diff/
> 
> 
> Testing
> -------
> 
> 
> Screenshots
> -----------
> 
> 
>   http://git.reviewboard.kde.org/r/103613/s/412/
> 
>   http://git.reviewboard.kde.org/r/103613/s/413/
> 
> 
> Thanks,
> 
> Jonas Jacobi
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120410/5f090ede/attachment.html>

More information about the KDevelop-devel mailing list