<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124369/">https://git.reviewboard.kde.org/r/124369/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 24th, 2015, 9:15 a.m. UTC, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the dialog UI needs to be revamped, the functionality and code is nice though - good work!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a) the line edit for the field to encapsulate, why is it a line edit? or is it read only? what happens if the user changes the contents?
b) the dialog structure is really awkward, I have to say. please use a form layout with labels left and widgets right. Insteaqd of the radio boxes, use combo boxes, and don't use a sentence structure to layout the widgets, that will break horribly once the dialog is translated (think about RTL languages!). And maybe, it's best to let the user configure the signature in pure line edits? Adding combo boxes for noexcept etc. pp. will never scale. Or maybe even a text edit so the bodies can be edited directly as well? My current approach would be something like this: http://i.imgur.com/wWHOIHd.png the sources you can find at https://paste.kde.org/pdrz1iqo6 . I'm only unsure whether we can easily get the name of the setter argument to create the body. And should we also allow configuration about inlining the bodies? Probably a good idea for the future?</p></pre>
</blockquote>
<p>On July 24th, 2015, 12:06 p.m. UTC, <b>Maciej Poleski</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ad a) line edit is read only. it is information for user which field is going to be encapsulated. user cannot change contents.
ad b) i'm very bad GUI designer, that's fact... radio boxes were used because need one click instead of two in combo boxes. scaling is an issue, i like idea to provide text edits with bodies (prefilled with default generated body). Currently implementation is placed in (all) RecordDecl (class or structure). I'm still thinking how to find appropriate place in appropriate implementation file. I can get "path" of record decls up to TU record decl (root), but then how to find the same scope (place) in another TU (and prove that this TU will be available available after compilation to all uses)?</p></pre>
</blockquote>
<p>On July 24th, 2015, 5:05 p.m. UTC, <b>Maciej Poleski</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are two more reviews waiting for attention:
- https://git.reviewboard.kde.org/r/124300/
- https://git.reviewboard.kde.org/r/124285/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">just sayin... :)</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Would be cool if you could take some ideas from my GUI approach and mix and match it with your approach to get something useable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Regarding your problem to find the implementation file, there are helper functions to figure out whether you are in a header or not. If you are not, then just add it to the current file. Otherwise, use the buddy feature to find the corresponding implementation file, so just reuse that code please.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Then, I don't know what you mean with the "record decls" stuff at the end - do you mean external users of the field you just wrapped in a getter/setter? Like:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #408080; font-style: italic">// file 1:</span>
<span style="color: #008000; font-weight: bold">struct</span> foo {
<span style="color: #B00040">int</span> bar;
};
<span style="color: #408080; font-style: italic">// file 2:</span>
<span style="color: #BC7A00">#include "file1.h"</span>
{
foo f;
f.bar <span style="color: #666666">=</span> <span style="color: #666666">1</span>; <span style="color: #408080; font-style: italic">// translate that to f.setBar(1);</span>
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">? This is going to be a hard problem. I'd be <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">very</em> find with tackling the "simple" issue first of creating some code. Then, maybe, we should concentrate first on more local changes to get some useable stuff, like I mentioned before. What do you think? Tackling these hard problems we can still do later, no?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sorry for lagging behind, my day job and now Akademy often keeps me distracted. Please continue to ping me about things I'm missing or not responding to quickly enough.</p></pre>
<br />
<p>- Milian</p>
<br />
<p>On July 23rd, 2015, 2:07 p.m. UTC, Maciej Poleski wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Maciej Poleski.</div>
<p style="color: grey;"><i>Updated July 23, 2015, 2:07 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdev-clang
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Encapsulate field refactoring</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Displays dialog</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Transforms "use" site - replaces direct uses of encapsulated variable by calls to getter and setter method/function.</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Generates getter and setter implementation</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Attached image presents GUI with its default content (prefilled by heuristics on DeclaratorDecl AST node).</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Manual testing, unit testing</p></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>refactoring/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefielddialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring_changepack.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/encapsulatefieldrefactoring_changepack.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/redeclarationchain.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/refactoringmanager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/tudecldispatcher.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/tudecldispatcher.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/utils.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>refactoring/utils.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(20d17efae9a2a77cd7ef76bf7484ccfcf12e4cd8)</span></li>
<li>tests/test_encapsulatefield.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tests/test_encapsulatefield.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124369/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2015/07/15/ea15c55b-cc4d-4dfa-ac34-8c568a93701d__snapshot3.png">Encapsulate field dialog</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>