pharo-spec / Spec

Spec is a framework in Pharo for describing user interfaces.
MIT License
61 stars 63 forks source link

New jobs do not refresh if UI is blocking #1517

Open guillep opened 5 months ago

guillep commented 5 months ago

Although I understand the reason behind the current changes, it would be nice to keep progress bars updating while a new architecture is put in place. Otherwise, this gives the impression that Pharo is frozen during long operations (such as Iceberg cloning/fetching/checking out).

In the case above, the UI thread is blocked in an ffi callout. The callout launches eventually a callback that is executed in a separate process, which in turn triggers an announcement (hoping it updates the UI). Now, since the UI process is blocked in the callout, a UI cycle will never happen.

To see the issue, compare the two following scripts:

total := 200.
job := Job new title: 'x'; min: 0; max: total; yourself.
Job jobAnnouncer announce: (JobStart on: job).
1 to: total do: [ :completed |
    [
    job current: completed.
    Job jobAnnouncer announce: (JobChange on: job).
    ] forkAt: 61.
    10 milliSeconds wait.
].
Job jobAnnouncer announce: (JobEnd on: job).

vs

total := 200.
job := Job new title: 'x'; min: 0; max: total; yourself.
Job jobAnnouncer announce: (JobStart on: job).
1 to: total do: [ :completed |
    [
    job current: completed.
    Job jobAnnouncer announce: (JobChange on: job).
    ] value.
    10 milliSeconds wait.
].
Job jobAnnouncer announce: (JobEnd on: job).

The first one executes the annoucements in a separate thread, but since the UI thread is blocked (executing the script), the progress bar does not refresh. The second one executes the annoucements in the (current) UI thread and this is refreshed in SpMorphicProgressBarAdapter>>updateState

I propose we change SpMorphicProgressBarAdapter>>updateState as follows, commenting the check for the current UI thread, updating always?

updateState
    | isTimeForRefresh |

    (self progressBarState progressBarMorph = progressBar class)
        ifFalse: [ self updateProgressBar ].

    self progressBarState
        customizeMorphicLabel: progressLabel;
        customizeMorphicBar: progressBar.

    isTimeForRefresh := Time millisecondClockValue - lastRefresh >= refreshRateInMs.
    isTimeForRefresh 
        ifFalse: [ ^ self ].
    lastRefresh := Time millisecondClockValue.
>>  "(Processor activeProcess = UIManager default uiProcess) 
>>      ifTrue: [  "
            self widgetDo: [ :w |
                World doOneCycle ]
>>  "]".
    refreshBlock ifNotNil: [ refreshBlock value ].

I understand this could bring even more race conditions, but the current situation is not good either. How can we have a good compromise? Maybe there is another solution in the oven?