WIP: Reuse thing boxes when editing

[ChrisRenfrow]

This pull request aims to address the enhancement put forward in #190.

The approach is as follows:

1. Have /core/src/world/{npc,place}/view.rs append the uuid of the Thing as a property of the thing-box element called data-uuid. (9b1391c)
2. In /web/js/terminal.js, intercept the output and check to see if the most recent thing-box element has a matching data-uuid property.
3. If it's a match, perform a diff of the two elements, flagging the changed fields with a class.
4. Use the class to add some styles for visual feedback.

[ChrisRenfrow]

Making a note here that the initial instance of any Thing does not appear to have a UUID. e.g. if I create something and then edit it immediately afterwards, the preceding thing-box will not have a UUID to compare against. Perhaps this could be covered as an edge-case with something like:

if (last thing-box name (h1 contents) EQ new thing box name AND last thing box type (npc or place) EQ new thing box type)
OR the UUIDs are present and match:
> do the thing

[MikkelPaulson]

I thiiink this is doable with format_args!(), thereby avoiding a redundant allocation.

[ChrisRenfrow]

There may be a more elegant way to express this but this is what I've come up with to avoid the allocation.

let uuid = npc.uuid.as_ref();

writeln!(
    f,
    "<div class=\"thing-box npc\"{}>\n",
    (uuid.is_some())
        .then_some(format_args!(" data-uuid=\"{}\"", uuid.unwrap()))
        .unwrap_or(format_args!(""))
)?;

There is a borrowing/lifetime issue with just dropping in format_args!() with the original solution, this manages to get around all of that. Inspired by this thread.

[MikkelPaulson]

In fact, the type will probably change from Option<Uuid> to Uuid with the UUID-on-create change, rendering all of this moot.

[ChrisRenfrow]

This is true!

[MikkelPaulson]

Making a note here that the initial instance of any Thing does not appear to have a UUID. e.g. if I create something and then edit it immediately afterwards, the preceding thing-box will not have a UUID to compare against.

Oh, the UUID isn't generated until the entity is saved to the journal? If so it seems reasonable to change that logic, making sure that there are no lingering cases like if thing.uuid.is_some() as shorthand for "has been saved".

Your proposed logic could run into problems with a command sequence like

generate npc
load SomeSavedNpc

Where the load command would load the info into the box that was just generated (but not saved).

[ChrisRenfrow]

Where the load command would load the info into the box that was just generated (but not saved).

Perhaps I'm missing something, but according to my proposed logic, wouldn't this only be the case if the loaded npc shares the same name as the generated one? In which case it seems this would trigger a "name already in use" error.

[MikkelPaulson]

Ah, good point. Still, it seems a bit fragile when the UUID is right there (or could be easily added).

[ChrisRenfrow]

Oh, the UUID isn't generated until the entity is saved to the journal? If so it seems reasonable to change that logic, making sure that there are no lingering cases like if thing.uuid.is_some() as shorthand for "has been saved".

Am I correct in assuming that this is something that should be handled in a separate PR? Seems like an involved task at a glance but maybe you have a simpler fix in mind.

[MikkelPaulson]

Maybe. I'm away from a computer this weekend, but I'll take a look next week. I think it's a simple change but it'll probably touch quite a few files. And it'll make some tests non-deterministic.

[MikkelPaulson]

I think it's a simple change

-- Past @MikkelPaulson

[ChrisRenfrow]

No review necessary at the moment as I still need to wrap-up the "diff" code, but feedback is welcome.

[ChrisRenfrow]

With this latest round of changes I found it would be easiest if the fields that could change were pre-wrapped with spans rather than adding them on receipt just to style them.

For communicating the change, I settled with a slight luminosity shift of the background color paired with a simple underline.

There is one loose end that I think needs to be wrapped-up. With multiple edits, the command elements begin stacking-up the point that eventually they will completely displace the thing being edited. It might be preferred to also collapse consecutive edit commands issued on the same thing. What do you think @MikkelPaulson?

Aside from that observation I feel this is ready for review and feedback!

[MikkelPaulson]

I think it's a bit confusing both from a UX and technical standpoint to collapse edit commands, but the CSS was designed with the expectation of a command-output-command-output cycle. Part of the reason you're running out of space is because consecutive commands have too much vertical margin. A maximum of one line spacing between them seems appropriate, and we could even experiment with no spacing at all.

Maybe it's a result of the UUID weirdness that will be resolved when my branch merges, but my initial testing resulted in two boxes, the latter being the one that was updated when data changed.

Have you experimented with CSS animations? I feel like some sort of flash or fade would better draw attention to the fact that a box is changing contents, which is otherwise not expected behaviour. I'm also curious what impact this will have on screen readers, which are otherwise well supported by the site. That might be an investigation for another issue, though.

Be warned that you're going to run into some challenges with the tutorial. It is trivially broken in your draft PR because it includes some fragile string parsing that was affected by your formatting changes, no big deal to fix. However, I actually have no idea how the "Editing Characters" step is going to interact with these changes. Despite my best efforts, the tutorial is a horrible mass of spaghetti, but at least it is one of the best covered by integration tests, and tends to uncover particularly weird corner cases (eg. #302) since by design it interacts with most of the parts of the application in non-obvious ways.

I do recommend that you wait until the UUID change merges and rebase on my branch before touching the tutorial, since it has a bunch of trivial code changes that will nevertheless leave you buried under a pile of merge conflicts.

[MikkelPaulson]

I do recommend that you wait until the UUID change merges and rebase on my branch before touching the tutorial, since it has a bunch of trivial code changes that will nevertheless leave you buried under a pile of merge conflicts.

Done! Time for a rebase party.