<table><tr><td style="">davidedmundson requested changes to this revision.<br />davidedmundson added inline comments.<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/D2345" 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/D2345#inline-9602" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">shellcorona.cpp:321</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">QString</span> <span class="n">dumpconfigGroupJS</span><span class="p">(</span><span style="color: #aa4000">const</span> <span class="n">KConfigGroup</span> <span style="color: #aa2211">&</span><span class="n">rootGroup</span><span class="p">,</span> <span style="color: #aa4000">const</span> <span class="n">QString</span> <span style="color: #aa2211">&</span><span class="n">prefix</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; "><span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">If we go with this patch</p>
<p style="padding: 0; margin: 8px;">you should filter out ItemGeometries and AppletOrder here as you're making a special case out of them.<br />
Otherwise you're saving garbage data in the config which could conflict; one of the new IDs could clash.</p>
<p style="padding: 0; margin: 8px;">Also what's going to happen to activityId</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/D2345#inline-9600" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">shellcorona.cpp:383</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: #aa4000">int</span> <span class="n">i</span> <span style="color: #aa2211">=</span> <span style="color: #601200">0</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">foreach</span> <span class="p">(</span><span class="n">DesktopView</span> <span style="color: #aa2211">*</span><span class="n">view</span><span class="p">,</span> <span class="n">m_desktopViewforId</span><span class="p">.</span><span class="n">values</span><span class="p">())</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">Plasma</span><span style="color: #aa2211">::</span><span class="n">Containment</span> <span style="color: #aa2211">*</span><span class="n">cont</span> <span style="color: #aa2211">=</span> <span class="n">view</span><span style="color: #aa2211">-></span><span class="n">containment</span><span class="p">();</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">No!!!!</p>
<p style="padding: 0; margin: 8px;">you need to loop through containments() rather than the views<br />
(same for panel section)</p>
<p style="padding: 0; margin: 8px;">Otherwise:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">you're only saving the currently plugged in screens.</li>
<li class="remarkup-list-item">this won't save the configuration for any applets in a system tray.</li>
</ul>
<p style="padding: 0; margin: 8px;">and when you do do the system tray you're going to have a huge problem:<br />
system tray writes out SystrayContainmentId... you aren't going to know what that is.</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/D2345#inline-9596" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">mart</span> wrote in <span style="color: #4b4d51; font-weight: bold;">shellcorona.cpp:394</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I'm doing what i can there.<br />
every way to get items geometries is actually an hack, i think reading the config file is a bit more reliable, even if assumes how the config file is.<br />
otherwise it could get to the applet graphics object then map its geometry with containment graphics object geometry (however implementation detail, it would introduce an error as the geometry of the margins of the framesvg applet background wouldn't be included then)</p>
<p style="padding: 0; margin: 8px;">i can go for either of the approaches, none of them is perfect, but don't have strong preference there (while I do for the panel, as i think is the only way to ensure proper applets order)</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">If every way is a hack, then maybe this feature shouldn't go in at all.</p>
<p style="padding: 0; margin: 8px;">So the root issue is:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">for saving/restory applet geometry is handled by the containment which is in completely arbitrary as it's done by that containment plugin.</li>
<li class="remarkup-list-item">they use the ID of the applet for an index</li>
<li class="remarkup-list-item">ID won't be the same</li>
</ul>
<p style="padding: 0; margin: 8px;">Brainstorming, there is an option.<br />
*if* we assume dump and resume is always going to be from a clean setup we could just expose setting initial id to applet scripting. It's already in Plasma::Containment. it would fix all the problems without any hacks.</p>
<hr class="remarkup-hr" />
<p style="padding: 0; margin: 8px;">I can imagine this patch will destroy PMC if someone switched LNF twice as you're hardcoding stuff in plasma-workspace based on behaviour of plasma-desktop.</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/D2345#inline-9601" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">shellcorona.cpp:467</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">//enumerate config keys for containment</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">KConfigGroup</span> <span class="n">contConfig</span> <span style="color: #aa2211">=</span> <span class="n">cont</span><span style="color: #aa2211">-></span><span class="n">config</span><span class="p">();</span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">script</span> <span style="color: #aa2211">+=</span> <span style="color: #766510">" //Panel containment configuration</span><span style="color: #bb6622">\n</span><span style="color: #766510">"</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">what about globalConfig()?</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>rPLASMAWORKSPACE Plasma Workspace</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D2345" rel="noreferrer">https://phabricator.kde.org/D2345</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>mart, davidedmundson, Plasma<br /><strong>Cc: </strong>davidedmundson, ivan, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas<br /></div>