search

klembot / twinejs

Twine, a tool for telling interactive, nonlinear stories
https://twinery.org
GNU General Public License v3.0
2k stars 295 forks source link

[Twine 2.1] Removal of formats may leave default preferences in a bad state #374

Closed klembot closed 6 years ago

klembot commented 7 years ago

Originally reported by: Thomas M. Edwards (Bitbucket: tmedwards, GitHub: tmedwards)

Removal of formats in repairFormats() may leave the default story and proofing format preferences in a bad state as they do not get updated in the event that the formats they reference are deleted.

I think the correct course of action in this case would be to:

  1. If possible, upgrade the preference to the current version of the previously selected format within the same major version.
  2. Elsewise, if no similar major version exists, reset the preference to the current version of the builtin default within the same major version—not simply the builtin default as that itself may have been deleted via upgrade.

Test Case

  1. Set the default story format to SugarCube v2—e.g. 2.14.0.
  2. Upgrade to a newer/higher version of SugarCube v2, by manually installing a newer version or upgrading to a newer version of Twine 2 which is bundled with a newer version—e.g. 2.16.0.
  3. The version of SugarCube v2 currently set as the default preference is removed, but the preference does not get updated.
klembot commented 7 years ago

Original comment by Chris Klimas (Bitbucket: klembot, GitHub: klembot):

Update stories to latest format versions in repairStores()

Resolves #374

klembot commented 7 years ago

Original comment by Chris Klimas (Bitbucket: klembot, GitHub: klembot):

Update default/proofing format versions to latest

See #374

klembot commented 7 years ago

Original comment by Chris Klimas (Bitbucket: klembot, GitHub: klembot):

Prevent users from removing the default story format

See #374

klembot commented 7 years ago

Original comment by Thomas M. Edwards (Bitbucket: tmedwards, GitHub: tmedwards):

It seems like the most sensible thing to do is to prevent the user from deleting the format set as default.

Not a bad idea, though we should ensure that it doesn't violate the principle of least surprise—i.e. users must know why they cannot delete the the format, elsewise rage will likely ensue—so no silently hiding the delete button.

My first instinct is to change updatePrefs to update versions in the other part of your bug, but we would also need to fix it in the stories themselves, since format versions are stored there too.

Sounds good to me.

klembot commented 7 years ago

Original comment by Chris Klimas (Bitbucket: klembot, GitHub: klembot):

It seems like the most sensible thing to do is to prevent the user from deleting the format set as default. repairFormats runs before the UI has initialized so it's going to be awkward to try to ask questions then.

My first instinct is to change updatePrefs to update versions in the other part of your bug, but we would also need to fix it in the stories themselves, since format versions are stored there too.

Other alternative is to fix the dialogs instead, but I'd rather nip it in the bud at the data layer.

klembot commented 7 years ago

Original comment by Thomas M. Edwards (Bitbucket: tmedwards, GitHub: tmedwards):

Yes, they would silently get v2.16.0, however….

The point is that no valid story format is selected as the default preference in the Formats dialog, which is not an ideal situation for users regardless of the fact that Twine 2 will silently push v2.16.0 on them—the Change Story Format dialog has the same issue. Showing users dialogs without valid settings is a bad practice to get into and makes for a terrible UI.

Beyond that, new projects created while in this state will have the outdated preference set as their initial format, which is particularly bad because now we're pushing outdated prefs down the line.

Finally, if no similar format exists—e.g. the user installed a non-builtin format, made it default, and then uninstalled it—then things break hilariously as Twine 2 doesn't have a clue what to do when the user attempts to Play, Test, or Publish.

If we don't silently update the prefs as we silently muck with the formats in repairFormats(), then we should either force the user to make a selection or, at the very least, display some kind of warning/error to let them know that their prefs are hosed—especially in the final case I outlined, where their story will be completely broken until some action is taken.

klembot commented 7 years ago

Original comment by Chris Klimas (Bitbucket: klembot, GitHub: klembot):

I follow your test case, but I don't understand how this would affect an end user. In that scenario, wouldn't Twine silently give the end user 2.16.0?