OpenNBS / OpenNoteBlockStudio

An open-source Minecraft music maker.
https://noteblock.studio
MIT License
746 stars 51 forks source link

Transfer the autosave status/interval byte to user storage rather than within the NBS file #445

Open CreeperPookie opened 6 months ago

CreeperPookie commented 6 months ago

Describe the enhancement you'd like One thing I noticed when debugging a partially corrupt NBS file (not the program's fault, don't worry lol) is that the autosave status is stored in the file. Since it's already deprecated and unused, this would be a great opportunity to remove it entirely for NBS v6. Also, if the byte(s) were ever to be re-used in the future, it could cause a conflict with the user's own autosave setting, and could raise the question of which should take priority.

I realize this should probably be in the NBS v6 discussion #402, but given this is such a minor change it felt more appropriate to make a feature request for this.

Bentroen commented 6 months ago

Hi, thanks for your suggestion!

The reason auto-save and auto-save interval were not removed from the format, although deprecated, is to avoid changing the internal song format structure as much as possible, unless it is absolutely necessary. When adding a new feature, one has no choice but to add a new field somewhere. But it causes no harm to keep a feature that's no longer used.

If anything, it should make it easier for parsers to implement full version range compatibility, since 1) it avoids one more conditional if statement to not encode the information based on the version, and 2) their internal representation can keep track of the auto-save value for the purpose of saving the song in older version, even if the song in question comes from a recent version. Although the field is deprecated in modern versions, songs can export the user's global auto-save setting, so that, should the song ever be saved in an older version, the behavior they had set up the moment the file was saved will be preserved in earlier versions of the program.

It also makes the documentation simpler to mark the field as deprecated rather than, somehow, indicating that it is simply absent. Additions to a specification are somewhat "permanent", i.e. once you add it, there's no going back — we'll always carry the burden of having to maintain information regarding that choice, whether it's to tell it was deprecated or removed.

On the other hand, I agree keeping unused information around can litter the file and make the song's structure confusing, so I'll consider adding this topic to the NBS version 6 proposal when the time comes. Thanks again for your careful consideration! :)

Bentroen commented 6 months ago

And just to clarify, the auto-save and auto-save duration settings are already stored in user preferences – the values written to saved songs are just redundance.