On 27.05.2025 05:20, ichthyo wrote:
...
as expected, going through all the numerous usages of XMLwrapper war a
tedious
task -- but in the end all went well; now I'd like to present the reworked
state on my "mxml"-branch (see https://github.com/Ichthyostega/yoshimi.git)
Most important for me now is the question: does the new interface
and usage patterns look plausible for a developer working on Yoshimi?
Can you roughly follow the logic of the implementation in Misc/
XMLStore.h|cpp ?
I have added some documentation into dev_notes/XML-Storage.txt and also
moved my developer-test into this directory (which could serve as a
self-contained example how to use this code).
The most notable change is that I have split the functionality of the
former class "XMLwrapper" into two distinct front-end classes:
- XMLStore is the access point and allows to create, load and save
tree structured data. And it manages the metadata
- XMLtree is a ligthweight smart-pointer, which refers to some
sub-tree in the XML. It can /not/ be "navigated" (as was the
usage, with the old XMLwrapper); rather, you /retrieve/ nested
sub-elements on an existing XMLtree (functions addElm / getElm)
I like this interface. XMLtree in particular is much better, IMHO. The
navigation that XMLwrapper offered meant that you constantly had to do
`exitbranch()` everywhere. As someone who tries to use modern C++
practices, I'm strongly in favor of getting rid of all such manual
cleanup code.
All the data manipulation functions were relocated to XMLtree.
Please note that lib-MXML is now solely imported into XMLStore.cpp
and usage of its data types and functions is confined to this
compilation unit.
Also great! Dependency isolation tends to make it easier to manage upgrades.
This was the whole point of the exercise and will allow us (in a second
step)
to add a switch and an adapted implementation for lib-MXML v4.0
As part of the changes, I had to understand some aspects of the usage
made by Yoshimi's core code, and especially the version and compatibilty
checks turned out somewhat difficult to understand, since this logic
has evolved over a long time and the implementation was partially
in Config, partially in the XMLwrapper.
First to mention, to test the changed behaviour, one can load an
existing file, save it back and compare both with a diff tool.
For this to work, you need to set the "gzip compression level" to 0,
so that it becomes possible to read the actual XML data on disk.
You will notice some minor changes, like line wrap, and a small
number of attributes which I have rearranged (waveshaping before
filter, and the bypass of effects). Furthermore, I have changed
some toggles in the config storage to bool parameters (which
is now implemented backward compatible, but obviously is not
forward compatible, i.e. older versions of Yoshimi will fail
back to the default settings of these toggles)
I looked through a diff that I produced with the old and the new
version, and all the differences look reasonable (and indeed, like an
improvement) to me.
Please consider these changes as proposal and feel free
to question and discuss anything...
Regarding the compatibility check, I tried to re-create the
existing behaviour, as can be inferred from code and Git commits.
But I have not automated it a bit more, and removed the build variables
MIN_CONFIG_MAJOR |MINOR
However, I am not sure that this behaviour is actually what we want,
because it will warn on each regular upgrade of Yoshimi. This is
probably a very defensive approach, because usually we care very
much to provide a soft migration whenever we introduce changes.
- right now the rule is: warn if major or minor version differs
- an alternative rule would be: tolerate one minor version back
and on upgrade of the major version, tolerate all minor versions
from previous releases, but always warn when loading config
from a newer version into an older Yoshimi.
This logic could be easily implemented in code
- yet another solution would be, to handle compatibility slightly
more manually; we could add a new build var COMPATIBLE_CONFIG_VER
with a string spec into the CMakeList.txt -- as you may have
noticed, I have implemented a parsing function for version
specs, and I have added equality and ordering predicates
(see Misc/VerInfo.h and the implementation at the top of Config.cpp)
Any thoughts?
I think I prefer the first option, except that for minor version I would
not warn on just any difference, only when the loaded minor version is
higher than the software minor version, since this is the only
"dangerous" case. When bumping the minor version, things are supposed to
stay backwards compatible, so the opposite case should be safe.
Another thing which I've notice (but not changed) is the fact
that Bank::generateSingleRoot() always stores the new files
in Zyn-format. Was this done intentionally so?
This was added in a single commit in 2020 and has not been changed
since. Will can perhaps comment further, but my gut feeling is that this
was simply overlooked.
--
Kristian
_______________________________________________
Yoshimi-devel mailing list
Yoshimi-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/yoshimi-devel
