cockpit/dialog: Support asynchronous updates#23081
cockpit/dialog: Support asynchronous updates#23081mvollmer wants to merge 6 commits intocockpit-project:mainfrom
Conversation
89bec14 to
6ca46aa
Compare
cockpituous
left a comment
There was a problem hiding this comment.
There are more than 10 code coverage comments, see the full report here.
6ca46aa to
56bcfc1
Compare
cockpituous
left a comment
There was a problem hiding this comment.
There are more than 10 code coverage comments, see the full report here.
pkg/lib/cockpit/dialog.tsx
Outdated
| if (Array.isArray(container) && typeof this.#state.tag == "number") { | ||
| if (this.#state.tag >= 0) | ||
| return container[this.#state.tag]; |
There was a problem hiding this comment.
These 3 added lines are not executed by any test. Details
| update_async(debounce: number, func: (val: T, task: DialogTask) => Promise<void>): void { | ||
| const val = this.get(); | ||
| this.#dialog._validate_value_async(this.#path, val, debounce, () => func(val)); | ||
| this.#dialog._update_value_async( | ||
| this.#state, | ||
| debounce, | ||
| task => func(val, task) |
There was a problem hiding this comment.
These 6 added lines are not executed by any test. Details
| set_cancel(cancel: (() => void) | null) { | ||
| this.#on_cancel = cancel; |
There was a problem hiding this comment.
These 2 added lines are not executed by any test. Details
| debug("cancellig task", this.#name); | ||
| window.clearTimeout(this.#timeout_id); | ||
| if (this.#on_cancel) | ||
| this.#on_cancel(); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| if (state.validation_task) | ||
| state.validation_task.start_now(); | ||
| for (const task of state.update_tasks.values()) | ||
| task.start_now(); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| for (const task of state.update_tasks.values()) { | ||
| await task.wait(); | ||
| awaited = true; |
There was a problem hiding this comment.
These 3 added lines are not executed by any test. Details
| }); | ||
| } while (awaited && n_rounds < 100); | ||
| if (n_rounds >= 100) | ||
| console.warn("task cycle in dialog"); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
| if (state.validation_task) | ||
| state.validation_task.cancel(); | ||
| for (const task of state.update_tasks.values()) | ||
| task.cancel(); |
There was a problem hiding this comment.
This added line is not executed by any test. Details
pkg/lib/cockpit/dialog.tsx
Outdated
| if (!state.relevant && state.validation_task) { | ||
| debug("cancelling irrelevant validation task", state_path(state)); | ||
| state.validation_task.cancel(); |
There was a problem hiding this comment.
These 3 added lines are not executed by any test. Details
| _update_value_async( | ||
| state: DialogValidationState, | ||
| val: unknown, | ||
| prom: Promise<void>, | ||
| ) { | ||
| state.cached_value = val; | ||
| state.cached_result = undefined; | ||
| state.timeout_id = 0; | ||
| state.promise = prom; | ||
| state.round_id = this.#get_current_validation_round_id(); | ||
| } | ||
|
|
||
| /* Unlike with the timeout, we can not cancel the old promise when | ||
| installing a new one. Instead we check at the end whether it is | ||
| still really us that is supposed to deliver the result, by | ||
| comparing promises. | ||
|
|
||
| To summarize: | ||
|
|
||
| - The round id check will fail if the value is no longer | ||
| relevant to the dialog. For example, say there is a text | ||
| input that can be toggled in and out of the dialog via a | ||
| checkbox. Now a validation round is started while the text | ||
| input is part of the dialog. During the debounce timeout or | ||
| while the asynchronous validation function runs, the user | ||
| toggles the checkbox (which triggers a new validation round) | ||
| and the text input is no longer part of the dialog. Now when | ||
| the timeout or validation for the text input concludes, the | ||
| round id check fails and the result is ignored, as it should. | ||
|
|
||
| - The promise check will fail when a asynchronous validation | ||
| takes longer than the debounce timeout. Let's say there is a | ||
| text input with a debounce timeout of 1 second and a | ||
| validation function that takes 2 seconds. The user makes a | ||
| change that triggers validation and then remains idle for | ||
| more than a second. After one second, the timeout expires and | ||
| the promise is created and starts running. It will finish at | ||
| second 3, but we are not there yet. At second 1.5 the user | ||
| makes another change, a new timeout expires at 2.5 and a new | ||
| promise is created. At second 3 the original promise finally | ||
| comes to a conclusion, and the path is still relevant to the | ||
| dialog, but this promise is no longer the current | ||
| promise. Its result will be ignored, as it should. | ||
| */ | ||
|
|
||
| #validation_state_is_current(state: DialogValidationState, prom?: Promise<void>): boolean { | ||
| return ( | ||
| (!prom || Object.is(state.promise, prom)) && | ||
| this.#is_current_validation_round_id(state.round_id) | ||
| ); | ||
| } | ||
|
|
||
| /* _validate_value_async puts this all together. | ||
| */ | ||
|
|
||
| _validate_value_async(path: string, val: unknown, debounce: number, func: () => Promise<string | undefined>): void { | ||
| const state = this.#get_validation_state(path); | ||
| if (!this.#probe_validation_state_cache(state, val)) { | ||
| debug("async validate start debounce", state.path, val); | ||
| this.#set_validation_state_timeout( | ||
| state, | ||
| val, | ||
| debounce: number, | ||
| func: (task: DialogTask) => Promise<void> | ||
| ): void { | ||
| state.update_tasks.add( | ||
| new DialogTask( | ||
| state_path(state) + ":update", | ||
| debounce, | ||
| () => { | ||
| debug("async validate start promise", state.path, val); | ||
| const prom = | ||
| func() | ||
| .catch( | ||
| ex => { | ||
| console.error(ex); | ||
| return undefined; | ||
| } | ||
| ) | ||
| .then( | ||
| result => { | ||
| if (this.#validation_state_is_current(state, prom)) { | ||
| debug("async validate done", state.path, result); | ||
| this.#set_validation_state_result(state, val, result); | ||
| } else { | ||
| debug("promise outdated", state.path); | ||
| } | ||
| } | ||
| ); | ||
| this.#set_validation_state_promise(state, val, prom); | ||
| async ctxt => { | ||
| try { | ||
| await func(ctxt); | ||
| } catch (ex) { | ||
| console.error(ex); |
There was a problem hiding this comment.
These 14 added lines are not executed by any test. Details
56bcfc1 to
75d71e6
Compare
Previously, a handle to an array element would always refer to the same index in the array, even it the array is modified via "handle.remove", for example. Now a handle will follow such array modifications. This was not really a problem since handles are short-lived. But with the upcoming asynchronous updates, we want to have handles that remain correct also for the duration of the update computation. Also, the code has gotten cleaner, in my opinion. It is tempting now to make handles and field states the same object. Maybe we do this later.
This is really not necessary.
This doesn't change the API, but factors the debouncing etc out of the validation machinery. We will use it also for asynchronous updates. Cancelling is now more explicit, which allows the validation function to take action when it happens, such as killing processes on the host.
fe2167c to
b52798b
Compare
This is possible now that running the action function blocks modifications only after validation and updates are done. It's also quite useful.
b52798b to
ae54610
Compare
Uh oh!
There was an error while loading. Please reload this page.