xilem_web: Add a `MemoizedAwait` view #448

Philipp-M · 2024-07-24T18:11:11Z

This is the rerun_on_change view described in #440, I named it MemoizedAwait as I think that fits its functionality.

It got also additional features debounce_ms (default 0) and reset_debounce_on_update (default true) as I think that's quite useful, in case a lot of updates are happening.

When reset_debounce_on_update == false, debounce is more a throttle than an actual debounce.

This also adds a more heavily modified version of the example added in #427, also showing OneOf in xilem_web (which didn't have an example yet).

This could be considered as alternative to #440 (at the very least the example) but those approaches could also well live next to each other.

) Co-authored-by: Markus Kohlhase <[email protected]>

Philipp-M

I can add the same Api (or different depending on discussion on this PR?) for xilem itself, as I think it can be ported mostly 1:1 to xilem (with additional Send + Sync bounds I think, debounce could probably also be a Duration).

Philipp-M · 2024-07-24T18:11:31Z

xilem_web/src/attribute_value.rs

@@ -11,6 +11,7 @@ pub enum AttributeValue {
    True, // for the boolean true, this serializes to an empty string (e.g. for <input checked>)
    I32(i32),
    U32(u32),
+    Usize(usize),


I don't think that change is worth its own PR, I might add more types in a different PR though, as I think all values that make sense here should be supported IMO.

Philipp-M · 2024-07-24T18:12:32Z

xilem_web/src/lib.rs

@@ -53,6 +54,7 @@ pub use attribute_value::{AttributeValue, IntoAttributeValue};
 pub use class::{AsClassIter, Class, Classes, ElementWithClasses, WithClasses};
 pub use context::ViewCtx;
 pub use element_props::ElementProps;
+pub use memoized_await::{memoized_await, MemoizedAwait};


I'm still not sure about the actual namespacing in xilem_web (and I think this is certainly also relevant for xilem), this exports MemoizedAwait which is probably nice for explicitely naming types, though I doubt that will likely happen (mostly because of the countless generic params necessary), So I'm not sure whether that type should even be exported, as it pollutes the namespace, but I slightly lean to exporting it.

I'm not sure, whether I should just create a new mod view and put all things that implement a View + things relevant for those into it, that means also elements, but I could also imagine just namespacing views in the root, e.g. with this view in xilem_web::concurrent...

Continuing loud thoughts: I've moved the view now in xilem_web::concurrent, as I think that this cleans it up a little bit and is more future proof.

Philipp-M · 2024-07-24T18:12:49Z

xilem_web/src/memoized_await.rs

+{
+    /// Debounce the `init_future` function, when `data` updates,
+    /// when `reset_debounce_on_update == false` then this throttles updates each `millisecconds`
+    pub fn debounce(mut self, milliseconds: usize) -> Self {


So this doesn't debounce when this view is spawned (e.g. as in example (state.cats_to_fetch < TOO_MANY_CATS).then_some(memoized_await(..))), I'm not sure whether it actually should debounce in View::build, so that it has the same behavior when it's in the view tree, as well as when it's being spawned.

Yeah, I'm not sure here.

I've did the debounce now in View::build and slightly refactored the View impl as well, as I think this should be able to be controlled.

When this behavior is not desired, it could be disabled by the user with .debounce(0) at the beginning and reenabled in the future callback (not that this is super awesome, but it at a least allows the user to decide). In the example it "feels" better with having the debounce in the beginning (when going over the TOO_MANY_CATS boundary).

Philipp-M · 2024-07-24T18:17:11Z

xilem_web/src/memoized_await.rs

+
+    /// When `reset` is `true`, everytime `data` updates, the debounce timeout is cleared until `init_future` is invoked.
+    /// This is only effective when `debounce > 0`
+    pub fn reset_debounce_on_update(mut self, reset: bool) -> Self {


Thinking about it, mabye this should be:

Suggested change

pub fn reset_debounce_on_update(mut self, reset: bool) -> Self {

pub fn reset_debounce_timeout_on_update(mut self, reset: bool) -> Self {

DJMcNab

Only given a cursory review as this is web code, but it's looking ok.

I feel like the fetching state management is more imperative than I'd like it to be, but I'm not sure how that can be resolved.

DJMcNab · 2024-07-25T08:09:57Z

xilem_web/src/memoized_await.rs

+
+/// Await a future returned by `init_future` invoked with the argument `data`, `callback` is called with the output of the future. `init_future` will be invoked again, when `data` changes. Use [`memoized_await`] for construction of this [`View`]
+pub struct MemoizedAwait<State, Action, OA, InitFuture, Data, Callback, F, FOut> {
+    init_future: InitFuture,


Ha, this is a better name than future_future

DJMcNab · 2024-07-25T08:18:27Z

xilem_web/src/memoized_await.rs

+    init_future: InitFuture,
+    data: Data,
+    callback: Callback,
+    debounce: usize,


Surely this should be a Duration?

I guess it should indeed be (I guess I was optimizing at the wrong location), I think that answers the question above for a potential xilem implementation.

The only thing, I don't really like about having Duration here, apart from minimal more boilerplate, is that it's not accurate below 1millisecond and is an integer value representing the time in milliseconds in setTimeout, so usize is at least from a technical perspective more correct I think.

DJMcNab · 2024-07-25T08:19:22Z

xilem_web/src/memoized_await.rs

+{
+    /// Debounce the `init_future` function, when `data` updates,
+    /// when `reset_debounce_on_update == false` then this throttles updates each `millisecconds`
+    pub fn debounce(mut self, milliseconds: usize) -> Self {


Yeah, I'm not sure here.

DJMcNab · 2024-07-25T12:56:32Z

xilem_web/web_examples/fetch/src/main.rs

+                tr((
+                    td(
+                        label("Reset fetch debounce timeout when updating the cat count:")
+                            .for_("reset-debounce-update"),
+                    ),
+                    td(input(())
+                        .id("reset-debounce-update")
+                        .attr("type", "checkbox")
+                        .attr("checked", state.reset_debounce_on_update)
+                        .on_input(|state: &mut AppState, event: web_sys::Event| {
+                            state.reset_debounce_on_update = input_target(&event).checked();
+                        })),
+                )),
+                tr((
+                    td(label("Debounce timeout in ms:").for_("debounce-timeout-duration")),
+                    td(input(())
+                        .id("debounce-timeout-duration")
+                        .attr("type", "number")
+                        .attr("min", 0)
+                        .attr("value", state.debounce_in_ms)
+                        .on_input(|state: &mut AppState, ev: web_sys::Event| {
+                            state.debounce_in_ms = input_target(&ev).value().parse().unwrap_or(0);
+                        })),
+                )),


I'm not sure that this adds much to the example.

You mean the amount of code for the controls, or the ability to change these variables? I think it shows the whole API of the new MemoizedAwait view in an interactive way to quickly see what those variables are about, and directly serves as (yet another) slightly bigger example in action?

It was not very obvious what changing the parameters actually did, and it's quite a lot of code for things which are peripheral to the actual logic.

Maybe the right solution is to move this further down the file?

DJMcNab · 2024-07-25T12:58:27Z

xilem_web/web_examples/fetch/src/main.rs

+                        .attr("value", state.cats_to_fetch)
+                        .on_input(|state: &mut AppState, ev: web_sys::Event| {
+                            if !state.cats_are_being_fetched {
+                                state.cats.clear();


Clearing the state in code which launches a request is rarely what you want, as it causes jumpiness

Yeah you're right, though I rather wanted to have a clear visual state when images are fetched (i.e. I consider that a feature not a bug^^), to see when a new request is started, so that they don't slowly pop in over the previous images, when they're being loaded, I think the "weirdness" will be further amplified, when setting width/height on each.

Ideally, I think the fetch_cats prefetches the images, and when it's resolved all images are changed at once in one frame. But that requires quite a bit more code (I actually tried just something like the equivalent of js: Promise.all([fetch(), fetch()]) but that didn't seem to be enough, as the images were redownloaded, probably different headers or something like that).

xilem_web/web_examples/fetch/src/main.rs

…`MemoizedAwait`. Also set `width` and `height` for the images to avoid popping.

Philipp-M · 2024-07-25T18:32:29Z

I feel like the fetching state management is more imperative than I'd like it to be, but I'm not sure how that can be resolved.

Hmm you mean in the example, like cats_are_being_fetched? Maybe with more sugarized and specialized views, but I'm also not sure, I probably look into more sugarized views too, which may help for common use-cases. But I think there needs to be some state for the user that they know when the future has started, and when it has ended...

Any thoughts on a similar API regarding the MemoizedAwait for xilem itself?

DJMcNab · 2024-07-26T08:55:40Z

It would make sense to have a similar API in Xilem.

Restructure the fetch example a little bit and add an explaining comment

…date` (which is now true as default)

Philipp-M · 2024-07-26T16:08:49Z

Ok, I've adjusted the changes according to the latest review comments and cleaned up/restructured the example a little bit, it should now be more focused on the actual interesting part (MemoizedAwait), which is further explained with a comment inside the example. I'm merging, thanks for the review.

flosse · 2024-07-26T16:58:18Z

@Philipp-M Great work!

One small idea: If the fetch state logic is separated from the view, then the code is easier to read for me.
Here is how I'd do this:

main...slowtec:xilem:use-fetch-state

If you like it, I can open it as a PR 😉

Philipp-M · 2024-07-26T22:19:43Z

Sure that's probably slightly more readable, I agree, so you could indeed upstream that if you want.

flosse · 2024-07-28T09:52:19Z

Here it is: #454

Philipp-M and others added 3 commits July 24, 2024 19:40

xilem_web: Support usize as AttributeValue

18f1f80

xilem_web: Add a new view MemoizedAwait to declaratively run futures

ba8efb7

xilem_web: Add a new example fetch (modified version of linebender#427

0e84aca

) Co-authored-by: Markus Kohlhase <[email protected]>

Philipp-M commented Jul 24, 2024

View reviewed changes

Philipp-M added 2 commits July 25, 2024 09:47

xilem_web: fetch example: Make it Either and actually make sense

fbcf96b

fix clippy lints

fab586a

DJMcNab approved these changes Jul 25, 2024

View reviewed changes

Philipp-M added 2 commits July 25, 2024 20:15

Debounce in View::build, and slight refactor of the View impl of …

3d0909a

…`MemoizedAwait`. Also set `width` and `height` for the images to avoid popping.

Move memoized_await in new concurrent mod

0a5ac87

fix doc test

0579718

flosse mentioned this pull request Jul 26, 2024

xilem_web: Add fetch example #427

Closed

Philipp-M added 2 commits July 26, 2024 18:01

Rename debounce to debounce_ms to be more clear what the value is,

54056b8

Restructure the fetch example a little bit and add an explaining comment

Add default documentation for debounce_ms and `reset_debounce_on_up…

ed26b34

…date` (which is now true as default)

Philipp-M enabled auto-merge July 26, 2024 16:09

Philipp-M added this pull request to the merge queue Jul 26, 2024

Merged via the queue into linebender:main with commit 981fcc4 Jul 26, 2024
16 checks passed

Philipp-M deleted the xilem_web-memoized-await branch July 26, 2024 16:19

RagibHasin mentioned this pull request Aug 6, 2024

Fix non-standard textbox behavior in Windows #489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xilem_web: Add a `MemoizedAwait` view #448

xilem_web: Add a `MemoizedAwait` view #448

Philipp-M commented Jul 24, 2024 •

edited

Loading

Philipp-M left a comment

Philipp-M Jul 24, 2024

Philipp-M Jul 24, 2024

Philipp-M Jul 25, 2024

Philipp-M Jul 24, 2024

DJMcNab Jul 25, 2024

Philipp-M Jul 25, 2024

Philipp-M Jul 24, 2024 •

edited

Loading

DJMcNab left a comment

DJMcNab Jul 25, 2024

DJMcNab Jul 25, 2024

Philipp-M Jul 25, 2024

DJMcNab Jul 25, 2024

DJMcNab Jul 25, 2024

Philipp-M Jul 25, 2024

DJMcNab Jul 26, 2024

DJMcNab Jul 25, 2024

Philipp-M Jul 25, 2024

Philipp-M commented Jul 25, 2024

DJMcNab commented Jul 26, 2024

Philipp-M commented Jul 26, 2024

flosse commented Jul 26, 2024

Philipp-M commented Jul 26, 2024

flosse commented Jul 28, 2024

	pub fn reset_debounce_on_update(mut self, reset: bool) -> Self {
	pub fn reset_debounce_timeout_on_update(mut self, reset: bool) -> Self {

xilem_web: Add a MemoizedAwait view #448

xilem_web: Add a MemoizedAwait view #448

Conversation

Philipp-M commented Jul 24, 2024 • edited Loading

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Philipp-M commented Jul 25, 2024

DJMcNab commented Jul 26, 2024

Philipp-M commented Jul 26, 2024

flosse commented Jul 26, 2024

Philipp-M commented Jul 26, 2024

flosse commented Jul 28, 2024

xilem_web: Add a `MemoizedAwait` view #448

xilem_web: Add a `MemoizedAwait` view #448

Philipp-M commented Jul 24, 2024 •

edited

Loading

Philipp-M Jul 24, 2024 •

edited

Loading