<table><tr><td style="">romangg 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/D14364">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/D14364#inline-75943">View Inline</a><span style="color: #4b4d51; font-weight: bold;">gladhorn</span> wrote in <span style="color: #4b4d51; font-weight: bold;">serializertest.cpp:163</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I was hoping the comment above is enough, repeated explanations are not that sensible either in my opinion. I need the constant configId to compare against it later, the point is to confirm that the configId is independent of the order of outputs in the hash for example.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You can leave the comment out if you want to.</p>

<p style="padding: 0; margin: 8px;">My core "complain" is this: In your code the line 163</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);">QCOMPARE(Serializer::configId(config), configId);</pre></div>

<p style="padding: 0; margin: 8px;">checks, that the config file names stay the same. For that it compares the config id value at the time the test is run to a static one, which you have fixated at an arbitrary point in time (which was when you wrote this change). This test is to make sure, that config id value stays the same over time.</p>

<p style="padding: 0; margin: 8px;">Below you have in line 172 again a comparison. But this time it makes sure that on changes to output ids the config id stays the same. This is not dependent on how the static value was at the time you wrote the patch but only on the value of the current one when the test is run.</p>

<p style="padding: 0; margin: 8px;">For example the current value when the test is run could become different to the fixated one. But the current value should (and could) still be constant over output id changes. Of course this does not influence the result of the test, because it fails already above in this case. But in theory it makes more sense to compare config ids on output id changes in regards to a variable holding the config id value when the test is run instead of comparing it with the fixated one.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R104 KScreen</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D14364">https://phabricator.kde.org/D14364</a></div></div><br /><div><strong>To: </strong>gladhorn, Plasma, romangg<br /><strong>Cc: </strong>romangg, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, mart<br /></div>