D20029: Capture counting corrected

[Eric Dejouhanet]

TallFurryMan requested changes to this revision.
TallFurryMan added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> scheduler.cpp:1455
>      {
>          appendLogText(i18n("No jobs left in the scheduler queue."));
>          jobEvaluationOnly = false;

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?

> scheduler.cpp:5061
>              if (oneSeqJob->getFrameType() == FRAME_LIGHT)
> -                lightFramesRequired += oneSeqJob->getCount() - newFramesCount[signature];
> +                lightFramesRequired += oneSeqJob->getCount()*oneJob->getRepeatsRequired() - newFramesCount[signature];
>          }

This fixes the count correctly, OK.

Side note: I think SchedulerJob cannot just be a model (with the latest changes it's less and less a model actually). We need to move the repeat count management to that class at some point. That's another story I'd like to have a look at.

> scheduler.cpp:5212
>          }
>          // Else rely on the captures done during this session
>  

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...

> scheduler.cpp:5268
> +    // only in case we remember the job progress, we change the completion count
> +    if (rememberJobProgress)
> +        schedJob->setCompletedCount(totalCompletedCount);

If the algorithm is OK, I would expect totalCompletedCount == captures_completed if rememberJobProgress is false.
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.

REPOSITORY
  R321 KStars

REVISION DETAIL
  https://phabricator.kde.org/D20029

To: wreissenberger, mutlaqja, TallFurryMan
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190325/92e3392c/attachment.html>