<table><tr><td style="">poboiko requested changes to this revision.<br />poboiko 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/D28957">View Revision</a></tr></table><br /><div><div><p>I've tested it a bit too, on couple of Google accounts.<br />
Apart from couple nitpicks, I think it's good to go.</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/D28957#inline-165185">View Inline</a><span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:27</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);">    KF5::Wallet
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    KPim::GAPICore # dependency of Google Settings
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    migrationshared
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Is it really needed? It seems like the code is using just the DBus interface</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/D28957#inline-165173">View Inline</a><span style="color: #4b4d51; font-weight: bold;">googleresourcemigrator.cpp:32</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: #304a96">#include</span> <span class="cpf"><KWallet/kwallet.h></span><span style="color: #304a96"></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#include</span> <span class="cpf"><QSettings></span><span style="color: #304a96"></span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Duplicated include</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/D28957#inline-165188">View Inline</a><span style="color: #4b4d51; font-weight: bold;">googleresourcemigrator.cpp:78</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: #aa4000">const</span> <span style="color: #aa4000">auto</span> <span class="n">configPath</span> <span style="color: #aa2211">=</span> <span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">locate</span><span class="p">(</span><span class="n">QStandardPaths</span><span style="color: #aa2211">::</span><span class="n">ConfigLocation</span><span class="p">,</span> <span class="n">configFile</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span style="color: #aa4000">return</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">unique_ptr</span><span style="color: #aa2211"><</span><span class="n">QSettings</span><span style="color: #aa2211">></span><span class="p">{</span><span style="color: #aa4000">new</span> <span class="n">QSettings</span><span class="p">{</span><span class="n">configPath</span><span class="p">,</span> <span class="n">QSettings</span><span style="color: #aa2211">::</span><span class="n">IniFormat</span><span class="p">}};</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;">What would happen here if only one of the resources was present (e.g. only contacts)? Could <tt style="background: #ebebeb; font-size: 13px;">instance</tt> be invalid 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/D28957#inline-165170">View Inline</a><span style="color: #4b4d51; font-weight: bold;">googleresourcemigrator.cpp:202</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">// Just to be sure, check that there are no left-over legacy instances</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span class="n">removeLegacyInstances</span><span class="p">(</span><span class="n">instances</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Although this should not happen, it will probably remove tokens from Wallet (as the result of cleanup procedure for legacy instances). As they store tokens in the same place, it will affect the new instance too. <br />
If so, we would like to have <tt style="background: #ebebeb; font-size: 13px;">backupKWalletData / restoreKWalletData</tt> here too.</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/D28957#inline-165187">View Inline</a><span style="color: #4b4d51; font-weight: bold;">googleresourcemigrator.cpp:282</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 class="n">resourceSettings</span><span class="p">.</span><span class="n">setEnableIntervalCheck</span><span class="p">(</span><span class="n">calendarSettings</span><span style="color: #aa2211">-></span><span class="n">value</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"EnableIntervalCheck"</span><span class="p">),</span> <span style="color: #304a96">false</span><span class="p">).</span><span class="n">toBool</span><span class="p">());</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">    <span class="n">resourceSettings</span><span class="p">.</span><span class="n">setIntervalCheckTime</span><span class="p">(</span><span class="n">calendarSettings</span><span style="color: #aa2211">-></span><span class="n">value</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"IntervalCheckTime"</span><span class="p">),</span> <span style="color: #601200">60</span><span class="p">).</span><span class="n">toInt</span><span class="p">());</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Interval checking options could in principle also come from contacts resource. We can probably merge somewhat it like that:</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);">const auto contactSettings = settingsForResource(oldInstances.contactResource);
[...]
resourceSettings.setEnableIntervalCheck(calendarSettings->value(QStringLiteral("EnableIntervalCheck"), false) || contactSettings->value(QStringLiteral("EnableIntervalCheck"), false));
resourceSettings.setIntervalCheckTime(qMin(calendarSettings->value(QStringLiteral("IntervalCheckTime"), 60).toInt(), contactSettings->value(QStringLiteral("IntervalCheckTime"), 60).toInt()));</pre></div></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R44 KDE PIM Runtime</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D28957">https://phabricator.kde.org/D28957</a></div></div><br /><div><strong>To: </strong>dvratil, poboiko, vkrause<br /><strong>Cc: </strong>kde-pim, fbampaloukas, dcaliste, dvasin, rodsevich, winterz, vkrause, mlaurent, knauss, dvratil<br /></div>