Clean up the outline view #19
glen
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "outline-cleanup_on_main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Clean up the source code and interface of the outline view. In addition, fix a bug that could cause
Assembly::realizeto react to itself under certain circumstances. Those circumstances arose, making the bug noticeable, while this branch was being written.Source code
Outlinecomponent into smaller components.Element::indexprivate, as discussed here.Interface
Assembly::realizefrom reacting to itself d223df869cElement::indexprivate b3470b597dThis mostly looks fairly straightforward to my eyes. Don't forget to add issues for the items we planned in discussing the last PR (e.g. moving reactive string reps of constraints out of the assembly). Otherwise, just some cosmetic stuff here.
@ -34,2 +35,4 @@}/* KLUDGE */#add-remove > button.emoji {Just the word "KLUDGE" is not helpful to a future maintainer (who might not be you). Please explain the concern, say a word about how this sidesteps it, and say a word about what a better solution might look like.
Done (in commit
2b083be). Would it be helpful to think about what a better solution might look like? I started using these temporary emoji icons precisely because it felt too early to think about polishing the icons.No need. Emoji and/or appropriate Unicode characters could be perfectly OK long-term solutions, as they are becoming part of society's visual vernacular.
@ -104,17 +111,38 @@ details[open]:has(li) .elt-switch::after {font-style: italic;}.cst.invalid {CSS class names, like variable names, should ideally be self-documenting. The class
.cstdoesn't say anything to me. Ideally, choose a different class name with clearer semantics. If that's not feasible for some reason, make sure to clearly comment the first use of the.cstclass.Done! I've expanded "cst" to "constraint," "elt" to "element," and "rep" to "representation" in the HTML element classes (commits
882286cand3f3c173).Yes, much more intelligible! Thanks, resolving.
@ -105,2 +112,4 @@}.cst.invalid {color: #f58fc2;I don't really mind
#222and#f0f0f0sort of thing for various greys, because they are pretty easy to understand, but my mental hex-to-hue converter is otherwise not particularly good. Colors also often carry semantic content and/or play specific roles. So please don't use non-grey bare hex colors in css. Give them symbolic names that convey role (presuming there is one) and/or hue as well if feasible (or if not, comment the hue where the symbol is defined) and then use them symbolically, presumably withvar(...)CSS references. For example, in Numberscope several colors are defined in App.vue and then used throughout the components withvar(...)references. Thanks.Done (in commit
0c8022d). I've named the color variables according to their roles, with cues about the intended color limited to brightness descriptions. There are comments describing the colors on the lines where the color variables are defined. I'm hoping that this will make it easier to keep the color descriptions up to date with the actual colors.Those are excellent symbolic names and comments. If we are ever pulling in an external package that has css variables of its own, we will need to add a prefix (e.g., --text-bright becomes --dyna-text-bright), but they're good for now.
@ -7,64 +6,52 @@ use crate::{engine, AppState, assembly::{Assembly, Constraint, Element}};/* DEBUG */fn load_gen_assemb(assembly: &Assembly) {Shouldn't these debug functions be in some kind of a test and/or conditionally compiled only if one is testing? They look a bit, um, specific for functions that would actually be compiled into a webpage...
These functions build the test assemblies we've been playing with: the "General" assembly that appears when you load the page, and the "Low-curvature" assembly that you can pick from the drop-down menu. They're always compiled because I've never wanted to build the app with the test configurations removed.
It would useful to build a more formal test assembly system at some point. That task seems pretty self-contained, so I think it should be its own PR. I also think it'll be easier once we have a system for saving and loading assemblies.
Ah, so they are not debug functions; in fact they are the current "Assembly gallery" or "Assembly examples". So just rename/re-comment things to clarify their current role, and add a way to easily/quickly get to the empty assembly as well (or put an issue to do that in a future PR), and all should be well.
Done (in commit
a48fef3).It's also quite plausible to me that some examples will move into the dyna3 equivalent of a "featured gallery", but that idea may not deserve to be in a comment at this point.
@ -203,3 +174,4 @@) { "+" }button(class="emoji", /* KLUDGE */disabled={Please elaborate all instances of KLUDGE
Done (in commit
2b083be).@ -35,0 +81,4 @@class=class.get(),on:keydown={move |event: KeyboardEvent| {match event.key().as_str() {What's the "native" form of event.key()? Seems a pity to convert it to a str just to match it against three options. Can't constants of the actual values of the three options (before str conversion), perhaps defined above if need be, be used in place?
The
keymethod returns aString(a heap-allocated, variable-length string object). Each of the string literals we're matching with is a&str(a reference to a "string slice," which seems to be a pointer to a fixed-length string of bytes). An explicit conversion is required, andas_strseems to be the standard way to do it.Oh, gotcha. A Rustism I was not yet used to. Hope you can see why it looked odd.
Yup. Stuff like
"hello".to_string()looked even weirder to me at first, but the Rust book authors seem to consider it the most idiomatic way of going in the other direction...Well yes in fact that looks so weird that I believe that in husht we will implement a labeled-literal syntax like
s"hello"for exactly this. Note that room has been left in the Rust syntax for such a thing, it's just not part of the language (yet, anyway).I think I recall you saying—maybe during a meeting—that you'd like
Element::indexto be renamed to something more descriptive, likecol_indexorconfig_col. If you do want that, let me know, and I'll add it to this PR.Since it is private, it's less critical, but yes, index is pretty uninformative. Either 'column' or 'columnIndex' would be better. Not sure I quite get the drift of 'config_col'...
I've switched to
column_index(in commit7d6a394). To reduce clutter, I've keptindexin the parts ofAssembly::realizewhere the column index gets assigned to a local variable. The localindexvariable always gets assigned to or from thecolumn_indexfield of an element at some point, which for me makes its meaning clear enough.Commenting the example assembly chooser reminded me that the branch to be pulled has one outstanding bug: the element list doesn't always update properly when you choose a different assembly. For example, if you switch from the "General' example to the "Low-curvature" example immediately after loading the page, you'll see that most of the element labels stay the same. The two examples don't actually have any labels in common.
I'd be equally happy to fix this in the current PR (with or without an issue) or to make an issue and fix it later.
I'm guessing this is hard to add a test for at this point in the testing scheme, as the ideal would be to add a test and a fix. But just a fix is fine. I think best to fix in this PR, on the principle of not merging known buggy states of the code.
Naming for local variables is much less critical. You only need to keep in mind what they are, well, locally. So I routinely use variable names like
ixfor index etc. in local variables. Compactness is helpful, too.OK, let me know when the bug fix is in and I will do a final review. Changes look good now otherwise.
Great—I'll try to think about this before our meeting, and we can talk about it if it turns out to be tricky.
You're right: I don't know how I'd test this without browser automation.
We've decided to leave the element list update bug (#20) in this PR, and fix it in a subsequent PR.