<table><tr><td style="">mwolff requested changes to this revision.<br />mwolff added a reviewer: mwolff.<br />mwolff added a comment.<br />This revision now requires changes to proceed.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D4232" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>pretty good stuff, awesome work! I have some minor nitpicks. In the future, you could make reviewing easier by not mixing functionality patches with cleanup patches</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16820" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:49</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;">if (OktetaGui_FOUND)
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> add_definitions(-DWITH_MEMVIEW=1)
</div><div style="padding: 0 8px; margin: 0 4px; "> list(APPEND kdevgdb_SRCS
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please use a header file that can be included on demand and gets created with cmake's configure_file</p>
<p style="padding: 0; margin: 8px;">the advantage is that only the files that actually depend on the memview can then include that file and not everything gets the define set</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16824" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.cpp:53</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span class="n">QVBoxLayout</span><span style="color: #aa2211">*</span> <span class="n">l</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QVBoxLayout</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">this</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QVBoxLayout</span><span style="color: #aa2211">*</span> <span class="n">l</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QVBoxLayout</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">consider using a .ui file generated with Qt designer instead as a follow-up cleanup</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16825" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.cpp:68</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span class="n">QVBoxLayout</span><span style="color: #aa2211">*</span> <span class="n">l</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QVBoxLayout</span><span class="p">(<span class="bright"></span></span><span class="bright"></span><span style="color: #aa4000"><span class="bright">this</span></span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QVBoxLayout</span><span style="color: #aa2211">*</span> <span class="n">l</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QVBoxLayout</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">why did you remove the this parameter everywhere?</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16826" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.cpp:86</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span class="n">connect</span><span class="p">(</span><span class="n">startAddressLineEdit</span><span class="p">,</span> <span class="bright"></span><span class="n"><span class="bright">SIGNAL</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">returnPressed<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">()),</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #ffd0d0;"> <span class="bright"> </span><span class="n">okButton<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="n"><span class="bright">SLOT</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="n">animateClick</span><span class="p">(<span class="bright">))</span>);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">connect</span><span class="p">(</span><span class="n">startAddressLineEdit</span><span class="p">,</span> <span class="bright"></span><span style="color: #aa2211"><span class="bright">&</span></span><span class="bright"></span><span class="n"><span class="bright">QLineEdit</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="n">returnPressed<span class="bright"></span></span><span class="bright"></span><span class="p"><span class="bright">,</span></span><span class="bright"> </span><span class="p"><span class="bright">[</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">=</span></span><span class="bright"></span><span class="p"><span class="bright">]()</span></span><span class="bright"> </span><span class="p"><span class="bright">{</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">okButton<span class="bright"></span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="n">animateClick</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">please always use the 4-arg connect, i.e. pass a "this" before a lambda to make sure it gets properly unconnected when this gets deleted</p>
<p style="padding: 0; margin: 8px;">also, prefer explicit capture lists over the catch-all ones: =/&</p>
<p style="padding: 0; margin: 8px;">here, replace = with okButton</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16821" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.h:54</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">class</span> <span style="color: #a0a000">MemoryRangeSelector</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QWidget</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">all of this class is private, is that really your intention? I'd remove the friend below and keep it public like it used to be. also, moving it to the .cpp is better if at all possible, simply forward-declare it here</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16823" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.h:58</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">MemoryRangeSelector</span><span class="p">(</span><span class="n">QWidget</span><span style="color: #aa2211">*</span> <span class="n">parent</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span style="color: #a0a000">private</span><span class="p">:</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">QLineEdit</span><span style="color: #aa2211">*</span> <span class="n">startAddressLineEdit</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">de-indent from here on below</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D4232#inline-16822" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">memviewdlg.h:64</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="n">friend</span> <span class="n">class</span> <span class="n">MemoryView</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: #d0ffd0;"> <span class="p">};</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">maybe make it all public if you only access it from the view?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D4232" rel="noreferrer">https://phabricator.kde.org/D4232</a></div></div><br /><div><strong>EMAIL PREFERENCES</strong><div><a href="https://phabricator.kde.org/settings/panel/emailpreferences/" rel="noreferrer">https://phabricator.kde.org/settings/panel/emailpreferences/</a></div></div><br /><div><strong>To: </strong>volden, KDevelop, mwolff<br /><strong>Cc: </strong>mwolff, kossebau, kdevelop-devel, Pilzschaf, akshaydeo, surgenight, arrowdodger<br /></div>