<table><tr><td style="">wreissenberger requested review of this revision.<br />wreissenberger 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/D20029">View Revision</a></tr></table><br /><div><div><p>Please check my comments, from my point of view everything should be fine.</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/D20029#inline-112519">View Inline</a><span style="color: #4b4d51; font-weight: bold;">TallFurryMan</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:1455</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">A null pointer exception here? I see no direct reason for it (in setCurrentJob), nor for the removal of the current job if no job is left to run, could you elaborate on the consequences?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">The problem occurs in line 4675 (sic!):<br />
<tt style="background: #ebebeb; font-size: 13px;">if (currentJob->getRepeatsRemaining() == 0)</tt><br />
when trying to find the next job. <tt style="background: #ebebeb; font-size: 13px;">evaluateJobs()</tt> is called before and sets <tt style="background: #ebebeb; font-size: 13px;">currentJob</tt> to <tt style="background: #ebebeb; font-size: 13px;">nullptr</tt> if no jobs are left over to be executed.</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/D20029#inline-112522">View Inline</a><span style="color: #4b4d51; font-weight: bold;">TallFurryMan</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:5212</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I don't understand why you removed that line. Variable "captures_completed" can't always equal zero when not remembering progress: runtime events may abort the job, but still number of captures must be retained. This would cause areJobCapturesComplete to flip to true immediately...</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is irrelevant, since the number of completed frames is only changed in the case that we remember the job progress (see lines 5266ff). In theory we could leave it, but it is not used in the case that we do not remember the job progress.</p>

<p style="padding: 0; margin: 8px;">This together with lines 5266ff resolve the second bullet point of 1. In the old implementation, the number of completed frames has been added for each of the sequence job entries and counted the same value multiply.</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/D20029#inline-112523">View Inline</a><span style="color: #4b4d51; font-weight: bold;">TallFurryMan</span> wrote in <span style="color: #4b4d51; font-weight: bold;">scheduler.cpp:5268</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">If the algorithm is OK, I would expect totalCompletedCount == captures_completed if rememberJobProgress is false.<br />
I see the "optimization" andI agree on the principle, but I'd prefer a debug assertion here in a else. This change is introducing a dependency between the end and beginning of the algorithm, while we strive to clarify it.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">An assertion does not make sense for the else branch, since in this case the number of completed frames is already set in the SequenceJob.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R321 KStars</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D20029">https://phabricator.kde.org/D20029</a></div></div><br /><div><strong>To: </strong>wreissenberger, mutlaqja, TallFurryMan<br /><strong>Cc: </strong>kde-edu, narvaez, apol<br /></div>