<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103613/">http://git.reviewboard.kde.org/r/103613/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 8th, 2012, 10:41 p.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On April 8th, 2012, 10:43 p.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">oh and kcm_kdevgenerateaccessors_settings.desktop is missing in your patch!</pre>
</blockquote>
<p>On April 8th, 2012, 10:59 p.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?
</pre>
</blockquote>
<p>On April 9th, 2012, 1:46 p.m., <b>Milian Wolff</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On April 10th, 2012, 8:46 a.m., <b>Jonas Jacobi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.
</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
<br />
<p>- Milian</p>
<br />
<p>On April 2nd, 2012, 1:41 p.m., Jonas Jacobi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Jonas Jacobi.</div>
<p style="color: grey;"><i>Updated April 2, 2012, 1:41 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>languages/cpp/CMakeLists.txt <span style="color: grey">(1577a7b)</span></li>
<li>languages/cpp/codecompletion/context.h <span style="color: grey">(a5fdea7)</span></li>
<li>languages/cpp/codecompletion/context.cpp <span style="color: grey">(33dcad1)</span></li>
<li>languages/cpp/codecompletion/generateaccessorshelperitem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codecompletion/generateaccessorshelperitem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/cppgenerateaccessors.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessors.kcfg <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessors.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsconfiguration.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsconfiguration.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsdialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsdialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorseditor.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorseditor.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsitem.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorsitem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorspreferenceskcmodule.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorspreferenceskcmodule.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorssettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorssettings.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/accessors/generateaccessorssettingsdialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/codegen/simplerefactoring.h <span style="color: grey">(b2187c3)</span></li>
<li>languages/cpp/codegen/simplerefactoring.cpp <span style="color: grey">(8ae4e46)</span></li>
<li>languages/cpp/cppduchain/sourcemanipulation.h <span style="color: grey">(6f8a79b)</span></li>
<li>languages/cpp/cppduchain/sourcemanipulation.cpp <span style="color: grey">(3f40eb2)</span></li>
<li>languages/cpp/cppduchain/tests/test_duchain.cpp <span style="color: grey">(02ed83e)</span></li>
<li>languages/cpp/cppduchain/typeutils.h <span style="color: grey">(a686c3b)</span></li>
<li>languages/cpp/cppduchain/typeutils.cpp <span style="color: grey">(a801d47)</span></li>
<li>languages/cpp/tests/CMakeLists.txt <span style="color: grey">(24d9597)</span></li>
<li>languages/cpp/tests/testaccessorsettings.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/tests/testaccessorsettings.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/tests/testcppgenerateaccessors.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/cpp/tests/testcppgenerateaccessors.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>pics/mini/CMakeLists.txt <span style="color: grey">(c3316e7)</span></li>
<li>pics/mini/accessor_reader.png <span style="color: grey">(PRE-CREATION)</span></li>
<li>pics/mini/accessor_writer.png <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103613/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Screenshots </h1>
<div>
<a href="http://git.reviewboard.kde.org/r/103613/s/412/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/01/10/create_getter_example1_400x100.png" style="border: 1px black solid;" alt="" /></a>
<a href="http://git.reviewboard.kde.org/r/103613/s/413/"><img src="http://git.reviewboard.kde.org/media/uploaded/images/2012/01/10/create_setter_example1_400x100.png" style="border: 1px black solid;" alt="" /></a>
</div>
</td>
</tr>
</table>
</div>
</body>
</html>