tmedwards / sugarcube-2

SugarCube is a free (gratis and libre) story format for Twine/Twee.
https://www.motoslave.net/sugarcube/2/
BSD 2-Clause "Simplified" License
177 stars 41 forks source link

Land v3-style save API #190

Open tmedwards opened 1 year ago

tmedwards commented 1 year ago

What it says on the tin.

tmedwards commented 1 year ago

Should I update the Saves dialog?

Pros:

Cons:

Obviously, this would be a breaking change, which is bad. But it exposes new features, which is, probably, good.

If this were an entirely optional update, I wouldn't be too worried. Unfortunately, Twine users would be forced into this by default, so….

Decisions, decisions.

ChapelR commented 1 year ago

Should I update the Saves dialog?

My knee-jerk is that this is too breaking of a change, as I do know several users who have modified the saves dialog, either via CSS or JavaScript, and I've even written some code to do various things with the saves dialog for users that would probably break.

Maybe a guide or example of a new saves dialog in the docs would be more appropriate? I think the features you're talking about here are good, and I do get why you'd want to make them available to users in a more accessible way.

Another slightly grosser option could be to use a config to opt in (or even out, I guess) of the new saves dialog.

greyelf commented 1 year ago

Does the new Save API. and its UI component, allow the Author to configure the "Delete All Saves" option to exclude the Auto Save slot(s) when it deletes all existing saves?

BawdyInkSlinger commented 1 year ago

Is this a change to the existing API and the existing UI? Would there be any benefit to splitting these up into separate issues? I'm kind of assuming a new and improved API could provide value even if the UI stays the same.

tmedwards commented 1 year ago

@ChapelR

My knee-jerk is that this is too breaking of a change, as I do know several users who have modified the saves dialog, either via CSS or JavaScript, and I've even written some code to do various things with the saves dialog for users that would probably break.

Too bad. 🤪

I've decided to implement the new functionality as much as possible, because the alternative is crazy.

That said, I've also decided to leave the actual markup as close to the original as possible—e.g., it still uses a <table> with all the same fields. Hopefully, that will avoid breaking most third-party modifications while still getting the new functionality in there.

Maybe a guide or example of a new saves dialog in the docs would be more appropriate?

Not really what you meant, but I'm more than willing to add a Saves dialog section in the v2.37.0 upgrade guide if it becomes necessary.

Since v2.37.0 will be, at least internally, a fairly significant version update. Once it's finished, save for issues raised therein, I do intend to have a public testing period to work out any kinks.

Someone, please, remind me to look into possible Saves dialog issues with third-party mods during that time.


@greyelf

Does the new Save API. and its UI component, allow the Author to configure the "Delete All Saves" option to exclude the Auto Save slot(s) when it deletes all existing saves?

No, and I don't really want to add another button to that part of the UI. It's crowded enough as-is.

Not that I need to keep parity with the Joneses of the world, but I've rarely, if ever, seen that feature in other games. IME, if there is such a button, it does exactly what the current Saves dialog button does—i.e., it clears all saves.

That said, the new Saves API does have APIs specifically for such a thing, so it would be fairly simple to:

It might be a good candidate for a third-party mod. /shrug


@BawdyInkSlinger

Is this a change to the existing API and the existing UI?

Initially the former, but I did sort of push the latter in.

Would there be any benefit to splitting these up into separate issues? I'm kind of assuming a new and improved API could provide value even if the UI stays the same.

Not at this point. It's largely a done deal.

That said, if the pending public testing period raises issues with either, or both, I'll probably create a new issue to track each separately.

tmedwards commented 1 year ago

Leaving this tagged as todo for the moment, even though the new Save API should be stable, since its documentation is still unfinished.

Blarg. 🤢

ChapelR commented 1 year ago

To clarify, I don't really think this is actually a breaking change. I can see an argument for why it is but, given that save UI editing is like dialog event abuse hell, it's not like a documented api is changing. The new API looks awesome so I think it's worth it. I just personally have provided code to people I know will now break, and that code is publicly out there in the wild, skulking around like nodebob once did, ready to haunt me.

tmedwards commented 1 year ago

[…] The new API looks awesome so I think it's worth it. I just personally have provided code to people I know will now break, and that code is publicly out there in the wild, skulking around like nodebob once did, ready to haunt me.

I suppose it depends on the various pieces of code in question, but:

I'm also planning for a testing phase where, hopefully, some compatibility issues will be caught. Either allowing adjustments or, at least, giving a definite idea of where pain points may lie. 🤞🏻

tmedwards commented 1 year ago

Okay. I think both the documentation and API are finally complete—aside from any oopsies.

Until I rebuild the full documentation, here's the current Save API docs if anyone wants to peruse them: docs/api/api-save.md

EDIT: Oh. There are now explicit values/returns and throws sections, so there's less need to parse the description for details like those. That's an upgrade I need to bring to the rest of the documentation.

BawdyInkSlinger commented 1 year ago

@tmedwards

Typos:

Deletes all exisring browser saves, both auto and slot.

Deermines whether any browser saves are enabled, either auto, slot, or both.

HTH

tmedwards commented 1 year ago

Fixed.