Generalize constraints to observables #48
|
|
@ -133,28 +133,25 @@ details[open]:has(li) .element-switch::after {
|
|||
font-size: 10pt;
|
||||
}
|
||||
|
||||
.regulator.invalid-constraint {
|
||||
color: var(--text-invalid);
|
||||
}
|
||||
|
||||
.regulator > input {
|
||||
.regulator-input {
|
||||
color: inherit;
|
||||
background-color: inherit;
|
||||
border: 1px solid var(--border);
|
||||
border-radius: 2px;
|
||||
}
|
||||
|
||||
.regulator > input::placeholder {
|
||||
.regulator-input::placeholder {
|
||||
color: inherit;
|
||||
opacity: 54%;
|
||||
font-style: italic;
|
||||
}
|
||||
|
||||
.regulator.valid-constraint > input {
|
||||
.regulator-input.constraint {
|
||||
background-color: var(--display-background);
|
||||
}
|
||||
|
||||
.regulator.invalid-constraint > input {
|
||||
.regulator-input.invalid {
|
||||
color: var(--text-invalid);
|
||||
border-color: var(--border-invalid);
|
||||
}
|
||||
|
||||
|
|
@ -166,7 +163,7 @@ details[open]:has(li) .element-switch::after {
|
|||
font-style: normal;
|
||||
}
|
||||
|
||||
.invalid-constraint > .status::after, details:has(.invalid-constraint):not([open]) .status::after {
|
||||
.regulator:has(.invalid) > .status::after, details:has(.invalid):not([open]) .status::after {
|
||||
content: '⚠';
|
||||
color: var(--text-invalid);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -111,6 +111,7 @@ impl Element {
|
|||
}
|
||||
}
|
||||
|
||||
// `set_point_spec` must always be a valid specification of `set_point`
|
||||
#[derive(Clone, Copy)]
|
||||
pub struct Regulator {
|
||||
pub subjects: (ElementKey, ElementKey),
|
||||
|
|
@ -119,12 +120,6 @@ pub struct Regulator {
|
|||
pub set_point_spec: Signal<String>
|
||||
|
glen marked this conversation as resolved
Outdated
|
||||
}
|
||||
|
||||
impl Regulator {
|
||||
pub fn has_no_set_point_spec(&self) -> bool {
|
||||
self.set_point_spec.with(|spec| spec.is_empty())
|
||||
}
|
||||
}
|
||||
|
||||
// the velocity is expressed in uniform coordinates
|
||||
pub struct ElementMotion<'a> {
|
||||
pub key: ElementKey,
|
||||
|
|
|
|||
|
|
@ -19,17 +19,55 @@ use crate::{
|
|||
// an editable view of a regulator
|
||||
#[component(inline_props)]
|
||||
fn RegulatorInput(regulator: Regulator) -> View {
|
||||
let valid = create_signal(true);
|
||||
let value = create_signal(regulator.set_point_spec.get_clone_untracked());
|
||||
create_effect(move || value.set(regulator.set_point_spec.get_clone()));
|
||||
|
||||
// this closure resets the input value to the regulator's set point
|
||||
// specification, which is always a valid specification
|
||||
let reset_value = move || {
|
||||
batch(|| {
|
||||
valid.set(true);
|
||||
value.set(regulator.set_point_spec.get_clone());
|
||||
})
|
||||
};
|
||||
|
||||
// reset the input value whenever the regulator's set point specification
|
||||
// is updated
|
||||
create_effect(reset_value);
|
||||
|
||||
view! {
|
||||
input(
|
||||
r#type="text",
|
||||
class=move || {
|
||||
|
glen marked this conversation as resolved
glen
commented
Please could you explain to me the three If I am understanding Rust correctly, the But it seems my suggestions in the previous paragraph can't possibly be correct, because And finally, the third Thanks in advance for illuminating these things for me. Please could you explain to me the three `move`s in the three closures in this `view!` macro, the one that specifies the class of the input element, the one that specifies its change handler, and the one that specifies its keydown handler?
If I am understanding Rust correctly, the `move` annotation specifies that the closure should take ownership of any free variables that appear therein. So that would mean that the function that computes the class of the input takes ownership of the regulator. Why is that desired? Is it because the input element may not actually be realized until after the RegulatorInput function returns, and the ownership transfer extends the lifetime of the regulator until the closure has a chance to run and grab the info it needs to set the class (or perhaps actually, it has to persist that regulator indefinitely and then it can react to all future changes to the regulator)? The class-calculating closure of the input element seems like a slightly odd place for the long-term ownership of the regulator to reside, but maybe it doesn't really matter what owns it, as long as it persists indefinitely?
But it seems my suggestions in the previous paragraph can't possibly be correct, because `regulator` also occurs free in the closure passed as `on:change`, but the ownership of one entity can't exist in two places, if I am understanding correctly. So I guess please just explain to me what's going on here.
And finally, the third `move` seems the most confusing. For the life of me, I can't see _any_ free variables in the closure passed as `on:keydown`, so there doesn't seem to be anything _to_ move, and so the `move` shouldn't be there.
Thanks in advance for illuminating these things for me.
glen
commented
Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal. Anyhow, I now get the first two Replying to your latest post here, to keep it in this resolvable conversation. Ah, the fact that Signal has Copy semantics makes this code much clearer. I didn't know that, and it seems a weird quirk of Rust to me that with ownership such a crucial concept to Rust, it is not clear whether copy or ownership transfer is in effect and I don't see any way you can know without consulting the source/docs for Signal.
Anyhow, I now get the first two `move`s. And now I see that `reset_value` is just a local closure, and that's what's being `move`d by the third `move` (for the keydown event). So that's good. Resolving.
|
||||
if valid.get() {
|
||||
match regulator.set_point.get() {
|
||||
Some(_) => "regulator-input constraint",
|
||||
None => "regulator-input"
|
||||
}
|
||||
} else {
|
||||
"regulator-input invalid"
|
||||
}
|
||||
},
|
||||
placeholder=regulator.measurement.with(|result| result.to_string()),
|
||||
bind:value=value,
|
||||
on:change=move |_| {
|
||||
let value_val = value.get_clone_untracked();
|
||||
regulator.set_point.set(value_val.parse::<f64>().ok());
|
||||
regulator.set_point_spec.set(value_val);
|
||||
match value_val.parse::<f64>() {
|
||||
Err(_) if !value_val.is_empty() => valid.set(false),
|
||||
set_pt => batch(|| {
|
||||
regulator.set_point.set(set_pt.ok());
|
||||
regulator.set_point_spec.set(value_val);
|
||||
valid.set(true);
|
||||
})
|
||||
|
glen marked this conversation as resolved
Outdated
glen
commented
This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set P.S. If the code here switched to something like this suggestion, then as this is the only instance of So I guess I am specifically suggesting replacing this block with -- presuming the syntax is OK -- and just removing that This looks like the code is checking the OK-ness of the try_from twice. Is there a concise clear way to bind a variable to the SpecifiedValue payload in the OK return case and set `valid` to true and call the as-yet-unwritten regulator.set with that specified value, in that case, and in the alternative (Error) case, just set `valid` to false and do nothing to the regulator? That organization would seem to read more crisply, as long as it isn't too cumbersome to write... And in fact, if I recall, isn't that sort of thing what `if let OK(blah) = try_from { ... } else {valid.set(false)}` is for?
P.S. If the code here switched to something like this suggestion, then as this is the only instance of `set_if_ok`, you could remove that method in favor of a simpler `set` method, or in fact maybe just a direct call of `regulator.set_point.set(...)`, eliminating the entire `impl Regulator` block at lines 127-135 of assembly.rs, which seems like a win.
So I guess I am specifically suggesting replacing this block with
```
if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
valid.set(true)
regulator.set_point.set(spec)
} else { valid.set(false) }
```
-- presuming the syntax is OK -- and just removing that `impl Regulator` block in assembly.rs altogether.
Vectornaut
commented
That sounds great to me. I'd reorganize the block to avoid the repetition of In the > So I guess I am specifically suggesting replacing this block with
>
> ```rust
> if let Ok(spec) = SpecifiedValue::try_from(value.get_clone_untracked()) {
> valid.set(true)
> regulator.set_point.set(spec)
> } else { valid.set(false) }
That sounds great to me. I'd reorganize the block to avoid the repetition of `valid.set`. Depending on whether you prefer `if let` or `match`, we could do either of these:
```rust
valid.set(
if let Ok(set_pt) = SpecifiedValue::try_from(value.get_clone_untracked()) {
regulator.set_point.set(set_pt);
true
} else { false }
)
```
```rust
valid.set(
match SpecifiedValue::try_from(value.get_clone_untracked()) {
Ok(set_pt) => {
regulator.set_point.set(set_pt);
true
}
Err(_) => false
}
)
```
In the `match` version, the argument of `valid.set` is just an inlined version of the old `Regulator::try_set` method. If we want to go this route, and you don't have a strong preference for inlining, I'd recommend just reverting commit 894931a, which replaced `Regulator::try_set` with `Regulator::set_if_ok`. We can also rewrite `try_set` to use `if let` instead of `match`, of course.
|
||||
};
|
||||
},
|
||||
on:keydown={
|
||||
move |event: KeyboardEvent| {
|
||||
match event.key().as_str() {
|
||||
"Escape" => reset_value(),
|
||||
_ => ()
|
||||
}
|
||||
}
|
||||
}
|
||||
)
|
||||
}
|
||||
|
|
@ -47,15 +85,8 @@ fn RegulatorOutlineItem(regulator_key: RegulatorKey, element_key: ElementKey) ->
|
|||
regulator.subjects.0
|
||||
};
|
||||
let other_subject_label = assembly.elements.with(|elts| elts[other_subject].label.clone());
|
||||
let class = create_memo(move || {
|
||||
match regulator.set_point.get() {
|
||||
None if regulator.has_no_set_point_spec() => "regulator",
|
||||
None => "regulator invalid-constraint",
|
||||
Some(_) => "regulator valid-constraint"
|
||||
}
|
||||
});
|
||||
view! {
|
||||
li(class=class.get()) {
|
||||
li(class="regulator") {
|
||||
div(class="regulator-label") { (other_subject_label) }
|
||||
div(class="regulator-type") { "Inversive distance" }
|
||||
RegulatorInput(regulator=regulator)
|
||||
|
|
|
|||
Tiny writing nit: this is a "garden path" phrasing -- I read "format discussed above" and started looking for the format specification earlier in the file, since I was pretty sure I hadn't seen one. Maybe reword just a bit to avoid that trap I fell into?
Nice catch! I've changed "above" to "at" to resolve the ambiguity (
7cbd926). Does that read all right to you?I agree unambiguous now. And hopefully this will change to an explicit anchor link when we get the doc system set up. So resolving.