Improvements to the upgrade routines #5660
Conversation
This commit starts the improvements on the upgrade methods. We are going to change both the DataFile and ConfigManager to use separate version values from LMMS release versions, so we can easily bump those versions for new upgrade routines. This first commit starts implementing a new version value for the ConfigManager.
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8788-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgb7f869f-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8788?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8787-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgb7f869f66-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8787?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8786-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgb7f869f66-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8786?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://8785-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-715%2Bgb7f869f66-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8785?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "eb6a66b456baf60d1417a78da322534c1280fea9"} |
There was a problem hiding this comment.
I would prefer to handle m_configVersion a little differently:
- Explicitly bump the version by setting it to a named constant. This makes it easy to know how one should bump the version in the future.
- Make the function currently known as configVersion cleaner. By (1) removing all side effects and (2) narrowing it's responsibility. We can use m_version directly throughout the code as long as we ensure that it's set correctly on load and upgrade.
|
|
As requested, the "configVersion" method was replaced by "legacyConfigVersion" instead, which is only used to return a configuration version if none is present in the configuration file. The configuration version of the current build is stored in a local variable called CONFIG_VERSION, making version bumping easier. Uses a switch statement instead of if-else to be able to make use of case-cascading. TODO: - Change the CONFIG_VERSION variable to a unsigned int? - Start working on the DataFile.cpp.
|
I tried to cover everything you mentioned on this last commit. I'll be starting I like the function vector solution for the DataFile upgrade. I'll try to implement it there. After I'm done I can do the same on ConfigManager for consistency between both, and also to leave it ready for when we have more upgrade routines. |
Starts refactoring the upgrade logic on DataFile.cpp. Now the "version" attribute is used to indicate which fileVersion we are loading. If the value of version is "1.0", we have a legacy file and use the legacyFileVersion method to retrieve the integer version using the LMMS version. If the value of version is an integer, we just read it. The integer indicates the position in a list of upgrade methods where we should start from and run the upgrade methods. The file version of the build is held in the FILE_VERSION const on DataFile.h. It HAS TO match the number of upgrade methods that we have on our list. One of the versions had 2 upgrade routines (upgrade_1_2_0_rc3 and upgrade_1_2_0_rc2_42). They were merged into a single one (upgrade_1_2_0_rc3) because they were both called from a single version check, meaning that they are both part of a single fileVersion. Two fatal errors were added (which can later be improved to show an error messagebox): One if the version attribute doesn't exist, and another one if we are using a FILE_VERSION that doesn't match the number of upgrade methods (to avoid mistakes while coding new upgrades later). The configVersion variables and methods were changed to use unsigned int instead of int. TODO: - Make the list of upgrade methods static. - Add debug messages for each upgrade routine for testing purposes.
On DataFile.cpp, we now use the vector list of upgrade methods as a static const variable, so it only has to be constructed once.
Reorganize vector lists so they are more easily readable. Revert changes on upgrade method names from ConfigManager.cpp.
|
Reverted the changes on the upgrade method names from I'm still in favor of changing the upgrade method names on both |
The file version bumping on DataFile.cpp is now automatic (using the size of the m_upgradeMethods vector as a reference). FILE_VERSION constant was removed, and with it the qFatal error when it doesn't match the size of the methods vector.
|
I'm in favor of naming the upgrade routines more descriptively than as simple number, at least going forward. Some of the existing ones do a lot, or are tricky to decipher, so I think I'd prefer leaving them alone unless you can think of a good name.
|
|
Code LGTM. I lean towards slightly more compact lists of upgrades, and we could rename some of the old routines to be more descriptive, but I don't think that should hold up a merge. As for things that should hold up a merge: testing. Have you tested yet? Do you want assistance with testing this, and if so what kind? |
I agree. This could be the same with the
I'd be okay with two per line with descriptive rename. 👍 The previous way was just a huge near unreadable eyesore. |
Last week time for coding has been a bit scarce, so I haven't tested it yet, if you happen to be available it would be great! I had the following idea for testing:
I could add the warnings to each upgrade routine tonight and update the PR to generate the testing builds if that sounds good, then I'd just remove the warnings when everything is tested. When the project files and configuration files are done they could be posted here to make it easier for others to test too. Not sure I'll be able to do them tonight before going to sleep though.
I can also format the lists that way while I'm on it! |
Improves the formatting of the vector lists of upgrade routines and LMMS versions (2 upgrade routines per line and 3 LMMS versions per line). Adds a qWarning for every upgrade routine for testing purposes, plus a qWarning that tells the current fileVersion/configVersion when upgrade is called. Removes extra space characters after the opening bracket of ConfigManager::upgrade_1_1_91.
|
I got ahead and updated the PR with the mentioned changes:
|
|
I've created a set of test files by saving an .mmp and replacing the creatorversion: I'll try testing with the buildbot AppImage and see how it goes. |
|
From my testing, it appears that the upgrade routines are being called correctly. @IanCaio now seems like a good time to update ConfigManager to use the same vector-of-function-pointers method, if you still want to do that:
After that I'd vote that we merge this. |
|
@Spekular Thanks for uploading the test files! I've been on a bit of a rush lately, but I'll try to change |
Changes ConfigManager to use a vector of upgrade methods, just like DataFile. The new Config Version can be calculated automatically now, so the CONFIG_VERSION constant was removed. Corrects a small comment on Datafile.h.
|
Changed |
|
Since both of our tests indicate that this is working and i don't see any outstanding issues, I'll merge this on Friday (UTC+2) if no one objects before then. If someone else does a passing review, maybe we can merge earlier. |
|
No problem! 😄 |
- Changes legacyConfigVersion and legacyFileVersion from const unsigned int to just unsigned int, since the const is irrelevant in this context. - Changes the type alias for upgrade methods to start with an uppercase as per the code style guidelines. Moves the aliasing of the type to the class declaration so it can be used on both the method and on the vector list declaration. - Changes the vector list names from m_upgradeMethods to UPGRADE_METHODS, so it's more visible they are a static constant. - Move the upgradeVersions list from the legacyFileVersion method to the DataFile class, as an static const called UPGRADE_VERSIONS. - Uses std::upper_bound instead of std::find_if for the legacyFileVersion method.
|
Thanks for the review @DomClark ! Requests were addressed on this last commit. I've done a quick test for the configuration files upgrades and loaded a couple of the test files Spekular posted, it seems like it's still working fine. The debugging warnings are still there in case someone else wish to test. |
Uses the UpgradeMethod type alias when defining the upgrade methods vector from both ConfigManager and DataFile.
|
Remove debug calls and merge? |
|
Is it worth testing this with #5636 merged first? |
Tested! Everything still works :) |
Removes the qWarning() calls that were placed for debugging purposes.
* First commit This commit starts the improvements on the upgrade methods. We are going to change both the DataFile and ConfigManager to use separate version values from LMMS release versions, so we can easily bump those versions for new upgrade routines. This first commit starts implementing a new version value for the ConfigManager. * Change code as per requested on review As requested, the "configVersion" method was replaced by "legacyConfigVersion" instead, which is only used to return a configuration version if none is present in the configuration file. The configuration version of the current build is stored in a local variable called CONFIG_VERSION, making version bumping easier. Uses a switch statement instead of if-else to be able to make use of case-cascading. TODO: - Change the CONFIG_VERSION variable to a unsigned int? - Start working on the DataFile.cpp. * Changes the upgrade logic on DataFile.cpp Starts refactoring the upgrade logic on DataFile.cpp. Now the "version" attribute is used to indicate which fileVersion we are loading. If the value of version is "1.0", we have a legacy file and use the legacyFileVersion method to retrieve the integer version using the LMMS version. If the value of version is an integer, we just read it. The integer indicates the position in a list of upgrade methods where we should start from and run the upgrade methods. The file version of the build is held in the FILE_VERSION const on DataFile.h. It HAS TO match the number of upgrade methods that we have on our list. One of the versions had 2 upgrade routines (upgrade_1_2_0_rc3 and upgrade_1_2_0_rc2_42). They were merged into a single one (upgrade_1_2_0_rc3) because they were both called from a single version check, meaning that they are both part of a single fileVersion. Two fatal errors were added (which can later be improved to show an error messagebox): One if the version attribute doesn't exist, and another one if we are using a FILE_VERSION that doesn't match the number of upgrade methods (to avoid mistakes while coding new upgrades later). The configVersion variables and methods were changed to use unsigned int instead of int. TODO: - Make the list of upgrade methods static. - Add debug messages for each upgrade routine for testing purposes. * Make method vector a static const variable On DataFile.cpp, we now use the vector list of upgrade methods as a static const variable, so it only has to be constructed once. * Reorganize vector lists Reorganize vector lists so they are more easily readable. Revert changes on upgrade method names from ConfigManager.cpp. * Makes the file version bumping automatic The file version bumping on DataFile.cpp is now automatic (using the size of the m_upgradeMethods vector as a reference). FILE_VERSION constant was removed, and with it the qFatal error when it doesn't match the size of the methods vector. * Improve formatting of version and upgrades lists Improves the formatting of the vector lists of upgrade routines and LMMS versions (2 upgrade routines per line and 3 LMMS versions per line). Adds a qWarning for every upgrade routine for testing purposes, plus a qWarning that tells the current fileVersion/configVersion when upgrade is called. Removes extra space characters after the opening bracket of ConfigManager::upgrade_1_1_91. * Changes ConfigManager to use a vector of methods Changes ConfigManager to use a vector of upgrade methods, just like DataFile. The new Config Version can be calculated automatically now, so the CONFIG_VERSION constant was removed. Corrects a small comment on Datafile.h. * Addresses Dom's review requests - Changes legacyConfigVersion and legacyFileVersion from const unsigned int to just unsigned int, since the const is irrelevant in this context. - Changes the type alias for upgrade methods to start with an uppercase as per the code style guidelines. Moves the aliasing of the type to the class declaration so it can be used on both the method and on the vector list declaration. - Changes the vector list names from m_upgradeMethods to UPGRADE_METHODS, so it's more visible they are a static constant. - Move the upgradeVersions list from the legacyFileVersion method to the DataFile class, as an static const called UPGRADE_VERSIONS. - Uses std::upper_bound instead of std::find_if for the legacyFileVersion method. * Uses type alias on vector assignment Uses the UpgradeMethod type alias when defining the upgrade methods vector from both ConfigManager and DataFile. * Removes debugging warnings Removes the qWarning() calls that were placed for debugging purposes.
As previously discussed on the Discord server, both DataFile and ConfigManager should use a different version number than the release versions to make it easier to add new upgrade routines and bump new files. This draft PR is a work in progress to implement those version numbers and work on the backwards compatibility with the old upgrading methods.