fix KDevelop::JobStatus for "short-lived" jobs
AbandonedPublic

Authored by antonanikin on Sep 16 2016, 5:07 AM.

Details

Reviewers
mwolff
kfunk
Summary

The patch fixes showing progress status for "short-lived" jobs. By default StatusbarProgressWidget blocks progress showing for statuses for 1000 msec. This is a good way to hide annoying progress showing for "small" tasks, like background parsing of saved source file. But this behavior also leads to blocking progress for "short-lived" statuses for different KJob's. Such jobs can show some output to OuputView therefore progress blocking is not problem for them. But if job do some "silent" work it will be very useful for users to indicate what the job is finished.

Test Plan

Tested on master branch with different short- (<1 s.) and long-lived (>1 s.) jobs:

  1. Standard builder jobs.
  2. kdev-cppcheck jobs, which emits percent signals during work.
  3. kdev-cppcheck jobs, which don't emits percent signals during work.

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
antonanikin updated this revision to Diff 6768.Sep 16 2016, 5:07 AM
antonanikin retitled this revision from to fix KDevelop::JobStatus for "short-lived" jobs.
antonanikin updated this object.
antonanikin edited the test plan for this revision. (Show Details)
antonanikin added a reviewer: kfunk.
antonanikin set the repository for this revision to R33 KDevPlatform.
antonanikin added a subscriber: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 16 2016, 5:07 AM
mwolff commandeered this revision.Sep 20 2016, 6:10 PM
mwolff added a reviewer: antonanikin.
mwolff added a subscriber: mwolff.

sorry, but can you please clarify again what the actual problem is, and why this patch is required to fix it? to me,it seems as if you change a lot in this patch which is unrelated to what you actually want to fix.

furthermore, if a job emits any percent and then finishes without emitting the 100% before, I think we should handle that gracefully here, but introducing a timer that starts after the first percent comes in, does not sound like the proper solution to me. why not simply handle that in slotFinished? Or I simply misunderstand all of this?

Please give a step by step outline of what happens now, and then show what you want to happen instead

util/jobstatus.cpp
58 ↗(On Diff #6891)

why change this? undo please

util/jobstatus.h
65 ↗(On Diff #6768)

unrelated change which introduces a memory leak - undo

antonanikin edited edge metadata.Sep 21 2016, 5:46 AM

Ok, lets I describe the original problem "step by step".

We have some KJob and associated JobStatus. Job do some work and can send signals, handled by JobStatus. We have two main scenarios:

  1. Job do it's work and send percent signals. After recieving first percent signal from our job StatusbarProgressWidget starts internal timer (1000 ms.) - during this delay widget blocks progress showing. If after this timeout our job is still in "live" state, widget popups progress bar and update it's status with new percent signals. When our job finished JobStatus obect send hideProgress signal, which causes starting second internal timer in StatusbarProgressWidget (for 5000 ms.), after which progress bar is hided. The key point here - time interval between first percent signal and hideProgress signal - if it less than 1000 ms. we don't see any progress status (blocked by StatusbarProgressWidget).
  1. Job do it's work and don't send percent signals. After it's finishing the JobStatus object emit hideProgress signal and therefore we don't see any progress.

This patch tries to fix described problem. If job send percent signal JobStatus starts internal timer and blocks emitting of hideProgress before timer finished. If job don't send percent signal JobStatus emit such signal at slotFinished and after starts timer.

This approach allows to show resulting status in any case. The key idea - provide delay between first percent signal and “final” hideProgress signal. The ugly place is using magic constant for delay, but StatusbarProgressWidget don't provide any mechanisms to avoid this.

util/jobstatus.cpp
58 ↗(On Diff #6891)

fixed

util/jobstatus.h
65 ↗(On Diff #6768)

JobStatusPrivate now is QObject and created as child of JobStatus object - there should be no memory leaks.

antonanikin requested changes to this revision.Sep 21 2016, 5:56 AM
antonanikin edited edge metadata.
This revision now requires changes to proceed.Sep 21 2016, 5:56 AM
antonanikin commandeered this revision.Sep 21 2016, 5:57 AM
antonanikin updated this revision to Diff 6846.
antonanikin edited reviewers, added: mwolff; removed: antonanikin.
antonanikin marked 2 inline comments as done.
antonanikin added inline comments.Sep 21 2016, 6:04 AM
util/jobstatus.h
65 ↗(On Diff #6768)

But ok, fixed.

mwolff edited edge metadata.Sep 22 2016, 2:40 PM

In your step-by-step explanation: Your point #1 sounds good to me - I don't see any issue with what you describe. Rather, it's just what we want. With #2, I also don't see a problem per se - if a job does not emit percentages, we don't show a progress bar.

So is the problem one long running job which should get shown, but smaller jobs hide the progress bar?

antonanikin updated this revision to Diff 6890.Sep 23 2016, 3:27 AM
antonanikin edited edge metadata.
In D2792#52782, @mwolff wrote:

In your step-by-step explanation: Your point #1 sounds good to me - I don't see any issue with what you describe. Rather, it's just what we want. With #2, I also don't see a problem per se - if a job does not emit percentages, we don't show a progress bar.

Ok, you are right. I fix patch - now it start timer only for jobs which emits percent signals.

So is the problem one long running job which should get shown, but smaller jobs hide the progress bar?

No, here already is ok, StatusbarProgressWidget correctly works with "fast" and "slow" statuses.

antonanikin updated this revision to Diff 6891.Sep 23 2016, 6:47 AM

You have no answered my question. Where is the problem? I tried to explain how I understood the problem the way you explained it to me and offered to potential interpretations. Apparently none of these match what you have in mind. So can you try again to explain where something is going wrong currently?

In D2792#53444, @mwolff wrote:

You have no answered my question. Where is the problem?

We have 2 main problems with current JobStatus:

  1. If job emits percent signals and it's ifetime is less then 1 sec. we don't see any status due the design of the StatusbarProgressWidget.
  2. If job emits percent signals too frequently (last diff update) we also don't see any status. This case seems to be a bug(or very strange behavior) of the StatusbarProgressWidget.
In D2792#53444, @mwolff wrote:

You have no answered my question. Where is the problem?

We have 2 main problems with current JobStatus:

  1. If job emits percent signals and it's ifetime is less then 1 sec. we don't see any status due the design of the StatusbarProgressWidget.

Again, this is not a problem but rather by design.

  1. If job emits percent signals too frequently (last diff update) we also don't see any status. This case seems to be a bug(or very strange behavior) of the StatusbarProgressWidget.

This sounds interesting. Can you clarify when this happens? Can you give us a way to reproduce this behavior? Looking at the existing code, it is not clear to me where this arises from. Also, again, I don't see how adding a timer would help in that situation.

Again, this is not a problem but rather by design.

Ok. But how to indicate "finished" status for jobs, which not show output during work? We can add the optional parameter for such "force" status showing , but this changes the API.

This sounds interesting. Can you clarify when this happens? Can you give us a way to reproduce this behavior? Looking at the existing code, it is not clear to me where this arises from. Also, again, I don't see how adding a timer would help in that situation.

https://paste.kde.org/pipaf2s9l

run:

TestJob* testJob = new TestJob(1000, 5000);
core()->uiController()->registerStatus(new KDevelop::JobStatus(testJob, "testJob"));
testJob->start();

If the value of first parameter is less than 1000 - we don't see any status (tested with master branch). I think it's not correct behavior :)

antonanikin planned changes to this revision.Oct 19 2016, 4:13 AM
antonanikin abandoned this revision.Oct 24 2016, 10:52 AM