<table><tr><td style="">amantia added inline comments.
</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/D8493" rel="noreferrer">View Revision</a></tr></table><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/D8493#inline-37451" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">foldermodel.cpp:1316</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">adding some comment here would help people reading the code in the future. I.e. something like "exclude items that are shown on a different screen"</p>

<p style="padding: 0; margin: 8px;">also, I personally think this code is not pretty at all. When no mapping is found, the first model that encounters the item in this <tt style="background: #ebebeb; font-size: 13px;">filterAcceptsRow</tt> will take up the mapping. But... Isn't that too random? Shouldn't the mapping be done elsewhere? Why can there be items encountered here that have no mapping? At the very least, can you add a comment here to explain why "first comes, first served" is the right approach?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Added comments and made it less random (folder views on the first available screen will own the new items). There is a TODO though, as we seem to not react correctly for the case when a folder view is removed from being a desktop containment. Will handle it in a followup diff.</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/D8493#inline-37457" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">foldermodel.cpp:1691</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">shorten this to:</p>

<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">if (containment && containment->corona())
    m_screenMapper->registerCorona(containment->corona());</pre></div>

<p style="padding: 0; margin: 8px;">? Or is asking for the corona too expensive?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I don't like to call the same method twice, and in corona() call usually is is an if and qobject_cast, but can be also a recursive call.</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/D8493#inline-37462" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">screenmapper.cpp:96</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">shouldn't you reset m_firstScreen and potentially the other maps, too?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Actually that clear() there was wrong. Available screens are either added via registerScreen or when the screen setup changes, via signals.</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/D8493#inline-37463" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mwolff</span> wrote in <span style="color: #4b4d51; font-weight: bold;">screenmapper.cpp:121</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">count -> size</p>

<p style="padding: 0; margin: 8px;">newMap.reserve(size / 2);</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">count() and size() is the same, no?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R119 Plasma Desktop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8493" rel="noreferrer">https://phabricator.kde.org/D8493</a></div></div><br /><div><strong>To: </strong>amantia, Plasma, ervin, mlaurent, dvratil, hein, aacid, davidedmundson, apol<br /><strong>Cc: </strong>mwolff, davidedmundson, broulik, mart, plasma-devel, ZrenBot, progwolff, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol<br /></div>