<table><tr><td style="">dhaumann added a comment.
</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/D16054">View Revision</a></tr></table><br /><div><div><p>Hi Gregor, I just did a post-review again, and noticed several issues I think we should address. I have a patch ready for most of the stuff, but did not yet post it on Phabricator, since I indeed use KTextEditor::Document as member pointer in the FileListItem, and its constructor now reads the icon etc from the document, which destroys the tsttestapp demo. So the comments are just here to make you aware of the issues. We can have the real discussion when I post the patch.</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/D16054#inline-88542">View Inline</a><span style="color: #4b4d51; font-weight: bold;">tabswitcher.cpp:144</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 style="color: #74777d">// add to model</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span style="color: #aa4000">auto</span> <span class="n">item</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="bright"></span><span class="n"><span class="bright">QStandardItem</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">iconForDocument</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span><span class="bright"></span><span class="n"><span class="bright">document</span></span><span class="bright"></span><span class="p"><span class="bright">),</span></span><span class="bright"> </span><span class="n"><span class="bright">document</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">-></span></span><span class="bright"></span><span class="n"><span class="bright">documentName</span></span><span class="bright"></span><span class="p"><span class="bright">());</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">auto</span> <span class="n">item</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="bright"></span><span class="n"><span class="bright">detail</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">FilenameListItem</span></span><span class="bright"></span><span class="p"><span class="bright">(</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">                                            <span class="n">iconForDocument</span><span class="p">(</span><span class="n">document</span><span class="p">),</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do you see the memory leak? item is never deleted, since you create a copy in ::insertRow().</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/D16054#inline-88539">View Inline</a><span style="color: #4b4d51; font-weight: bold;">tabswitcher.h:142</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="n">KTextEditor</span><span style="color: #aa2211">::</span><span class="n">MainWindow</span> <span style="color: #aa2211">*</span><span class="n">m_mainWindow</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">    <span class="bright"></span><span class="n"><span class="bright">QStandardItem</span>Model</span> <span style="color: #aa2211">*</span> <span class="n">m_model</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="bright"></span><span class="n"><span class="bright">detail</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">TabswitcherFiles</span>Model</span> <span style="color: #aa2211">*</span> <span class="n">m_model</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; ">    <span class="n">QSet</span><span style="color: #aa2211"><</span><span class="n">KTextEditor</span><span style="color: #aa2211">::</span><span class="n">Document</span> <span style="color: #aa2211">*></span> <span class="n">m_documents</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Since we also have a TabSwitcherTreeView which is not in the detail namespace, I think putting the model into the detail namespace adds more inconsistency than it solves. In fact, Given the tabswitcher plugin is just 3 files, it is arguable what is a "detail" and what not. In other words: Overengineering? :-)</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/D16054#inline-88541">View Inline</a><span style="color: #4b4d51; font-weight: bold;">tabswitcherfilesmodel.cpp:33</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: rgba(151, 234, 151, .6);"><span style="color: #74777d">     */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">QString</span> <span class="n">longestCommonPrefix</span><span class="p">(</span><span class="n">std</span><span style="color: #aa2211">::</span><span class="n">vector</span><span style="color: #aa2211"><</span><span class="n">QString</span><span style="color: #aa2211">>&</span> <span class="n">strs</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #aa4000">int</span> <span class="n">n</span> <span style="color: #aa2211">=</span> <span class="n">INT_MAX</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">What license is this? Since we seem to reuse logs of code, this is important :-)</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/D16054#inline-88540">View Inline</a><span style="color: #4b4d51; font-weight: bold;">tabswitcherfilesmodel.h:33</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: rgba(151, 234, 151, .6);"><span style="color: #74777d"> */</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #aa4000">struct</span> <span style="color: #a0a000">FilenameListItem</span> <span class="p">:</span> <span class="n">public</span> <span class="n">QStandardItem</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Indeed using a QStandardItem when NOT using a QStandardItemModel is more confusing than it helps, since some data is fetched directly from the item's member variables, and some data via the data() accessor. I strongly suggest to not do this: Since we derive from QAbstractTableModel now, I'd argue that using QStandardItem is wrong in terms of "that's not how it was designed".</p>

<p style="padding: 0; margin: 8px;">Mixing ->data() and direct member access is rather confusing.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R40 Kate</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D16054">https://phabricator.kde.org/D16054</a></div></div><br /><div><strong>To: </strong>gregormi, Kate, VDG, cullmann<br /><strong>Cc: </strong>cullmann, abetts, thsurrel, dhaumann, ngraham, kwrite-devel, michaelh, demsking, sars<br /></div>