Adds support for local paths and project bundles #5735
Conversation
Adds a new Base for paths called "local:", which will translate to the dir where the currently opened project file is at. In the future this will allow us to make project bundles and make it easier to export and transfer projects to other people without breaking the paths to samples, presets, plugins and others.
For now, to make a bundle LMMS has to be run through CLI with the makeBundle/--makeBundle command, followed by an input file and an output file ('lmms --makeBundle input.mmp output.mmp'). DataFile::writeBundle() is then called. For now, it only saves the mmp/mmpz file normally and also creates a "resources" folder if it doesn't exists. Later it will also manipulate the DataFile so all paths are local and copy all files to the resources folder.
TODO:
-Remove warnings.
-Implement the logic to manipulate the DataFile and copy files.
Starts implementing the logic that will go through all the resources of the project file and add them to the bundle. We use a std::map of QString to std::vector<QString>: The first string is the DOM element tagname that is going to be searched for. The vector of strings holds all attributes this element can have that accesses resources. For now we just print those to the screen.
The raw logic for creating the bundle is finished. It now copies the resource files and update the project to use "local:" paths to the new file now. Now it's a matter of organizing things and adding safety checks for file operation errors basically.
Improves comments and debugging warnings to make the writeBundle a bit more organized for review.
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Windows
Linux
macOS🤖{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://13307-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.122%2Bg6e8a6366c-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13307?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://13310-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.122%2Bg6e8a6366c-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13310?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/krfxi0nc8dqkt1mt/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38501254"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/rjg09g99lnehwvxw/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/38501254"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://13309-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.122%2Bg6e8a636-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13309?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://13308-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.122%2Bg6e8a6366c-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/13308?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "91509bda26461f966cb89a2e7be6d62dda7a632d"} |
Adds a project bundle folder, inside which the bundles will be created. Instead of receiving an output project file name, the makeBundle command now receives a bundle name that will be used as the name of the bundle's folder. Uses a typedef for the std::map with the tags and attributes with resources. TODO: - Fix the local: prefix so it works when we don't have the project file open (for CLI usage, or find another way to deal with it). - Sanitize the bundle name. - Allow overwriting bundles?
The PathUtil base prefix conversion for "local:" uses the loaded song file name. When we are running the makebundle command from the CLI there isn't a loaded project, so those prefixes aren't converted properly. Now, when there isn't a project open PathUtil will return "local:" again when it tries to convert this base prefix. DataFile can then check if the base prefix is still there, and if it is it knows the conversion wasn't possible, so it does the conversion itself. To do that, a member called m_fileName was added to DataFile, which will hold the file being manipulated if that's where the DataFile originated from, and the local path can be retrieved from it.
|
With the latest commits there were a couple of changes to the way project bundles are built:
To do list:
|
The bundle name is now sanitized. Since it's going to be used as a folder name, we need to keep the user from giving invalid folder names as a bundle's name. The rules for the name are: 1) It must start with a word character (either a digit or letter) 2) It can be followed by any number of letters, digits, whitespaces or hyphens 3) It must end with a word character (either a digit or letter) A Regexp is used to check for the name validity.
This commit regresses some functionality. Project bundles will be saved just as any other project, except they will also have the resources folder. It will be up to the user to organize the bundles on their own folders. It's currently not allowed to save a bundle on a folder where there's one already though (if there's a resources folder already). Later it might be allowed to overwrite bundles in that case. The projectbundles folder was dropped. The user can save project bundles anywhere in the system. The DataFile::writeBundle was removed. It's functionality was merged into the DataFile::writeFile method, by adding a boolean on the parameters defining whether it should be saved with resources. The logic of copying the resource files and changing the paths inside the project DataFile was moved to DataFile::copyResources, making the methods a little bit less dense.
The "Save As" dialog now has an option to save project as a project bundle (with resources), which will save the file as a bundle. Known bug: - Because the "local:" base prefix is translated to the filename from the Engine::getSong(), it breaks when Song::guiSaveProjectAs is called, because that method changes the project name before saving. Urgent fix!
There was a bug where "local:" prefixes weren't resolved properly during saving because Song::guiSaveProjectAs() changed the project name to the destiny file name before saving. This resulted in the local paths using the destination file as a reference. Both Song::guiSaveProject() and Song::guiSaveProjectAs() were rewritten, and now they only rename the project after it's saved.
|
I probably have changed the way this PR works too much by now, but I was trying to figure out the best way in terms of simplicity to introduce in the code base. The project bundle folder idea would require lots of extra coding compared to the one used now:
|
When the user tries to save a project bundle on a folder that already has a project bundle (contains a resources folder) a message box pops up telling the user it's not permitted and that another path should be chosen.
|
I think the basic functionality is finished. A little different from the original concept but I had to change things along the way trying to make the feature as simple as possible. Marking as ready for review. Main comment was updated with some more accurate information on the current state of the PR. |
Forgot to remove <QRegExp> header when I removed the code that used it.
For safety reasons, remove the possibility to bundle VSTs loaded through vestige. Also runs a safety check during the project being loaded (Song::loadProject) to check if either Vestige plugins or effect plugins are using local paths, and abort the project load if so. That is to avoid malicious code being run because of bad DLLs being shipped with a project file.
Extracts code that checks if a DataFile contains local paths to plugins to another method inside DataFile.
|
As per discussed with @DomClark on the discord channel about the safety of allowing plugins to be bundled (or accessed through local paths), I removed the Vestige plugins from the project bundle, which now only bundle samples. I also changed |
|
Sorry, I can't test this functionality. Mingw links are broken, and MSVC version crashes when a sample is drag-and-drop to the song-editor. |
Removes warnings previously used for debugging. Improves a warning message on PathUtil.
Fixes small bug, where a QMessageBox was being used to prompt an error without checking if the gui is loaded first. Now we check for the GUI and if we are in CLI mode we use a QTextStream instead.
|
PR updated to latest |
Fixed! But we have a big problem, because there are other places in the codebase where the doxygen comments were written that way, which was where I took reference from. This is probably why |
Yeah, I get 473 kB of warnings, this is indeed a "big" problem 😃 The only left comment is writing an optional test. I'm OK with the code. Should we already call out for testers? Or do you think it does not need further testing (there have been tests a while ago)? |
|
@Gumichan01 has tested recently and I tried to manually test after changes like the I think it's working fine though, I tried to continuously check it during the commits. Btw, CircleCI seems to have failed but not sure why, maybe something went wrong in the middle of the tests? |
|
About the CI error: I get that one all the time (because my system has |
It's good that it triggered an error though, because it uncovered an issue: I'm thinking about removing that assert though, as it seems that no other part of the codebase asserts for the conversion to element, and if the node is not an element, |
Some assert statements were being done wrong and are probably even unnecessary for that piece of code, so they were removed.
PathUtil::toShortestRelative() was including the local paths on the candidate paths, which could lead to a unallowed resource (i.e.: vst plugin) to be assigned a local path even on a regular save. The local paths are now skipped when looking for the shortest relative path, since they should only be used by the bundle save on the allowed resources.
|
Last commit fixes a bug where in certain situations a |
|
Some comments from a user point of view.
Also (and I think that unfortunately it's not possible to solve, but maybe you guys have a solution) is that if you use a VST that uses samples (like a sampler -vitala or poise, for example-) those samples are not included in the bundle, so you have to keep track of those samples and include them on the bundle if you wanna share it. |
|
Thanks for testing @superpaik ! I agree some details might be a bit confusing from the user perspective, maybe clarifying some behavior with documentation would help for now. This is more of an initial implementation but the idea is to continuously improve it and who knows, maybe even transition the regular "Save" to that approach. It's just that there are lots of changing parts, so it's better done incrementally IMO. Sorry about the big reply, just trying to answer some of your observations with some details from the coding side 😅
On the "Save" dialog we have the option of choosing a file or a folder IIRR. If I used the folder selection, the user would probably have to create the folder and then choose it to save the bundle inside. I used the filename to make it more convenient (avoiding that creating folder step), but it ended up causing this weird behavior where the dialog still thinks the save will happen over the file when in reality it will create a folder with the same name (minus the extension). I'll take a look to see if I can omit the "Overwrite" dialog when the "Save with resources" option is selected, but from a quick look on Qt's
Yeah, that was purposefully. For now the bundle save is just meant for backup/sharing (some things would have to change for the user to be able to continuously work and save over the same bundle). So the project still points to the original file and keeps marked as modified to force the user to do a regular save too and not lose its work.
Since the bundle is (at least now) meant to be a backup, I ended up not bothering about making it able to overwrite automatically, that not being very common when saving backups. It's also a bit safer to obligate the user to manually delete a bundle before writing over it, because if the user picks a project name that would match an existing folder without the extension they could unwillingly overwrite a folder that wasn't a bundle to begin with. If we decide to implement the overwriting despite that, there are some things to decide, as for example: Should it erase everything inside the previous bundle before overwriting or should it only overwrite the project file and samples? If there are samples with the same name should those be overwritten too or assigned a name with a increased number? That change could maybe use a separate PR to decide those things and find the best approach 😄
That could be done! I'll later check how that change would look like on code, if it turns out to be simple I can include in this same PR!
I've to confirm, but I think except for plugins that ship with LMMS the others might store their information encoded in their XML encoded data, making it harder to access and change the paths, at least with the approach used right now. I wonder if those plugins use relative paths themselves though? That would make it easier to workaround the fact that those samples are not included in the bundle, but would still force you to copy by hand to the bundle folder 😕 |
|
It's taken me quite a while to get around to, but I'd like to review this again tomorrow. |
|
I fell behind schedule again so I won't actually do as much of a review as I hoped today, but as I suspected we don't need to hack getBaseDir. Ian wrote on Discord:
But Secondly, I don't think we should blindly ignore the local path in |
Having PathUtil return an invalid path (not removing the
Only a few elements are allowed to have local paths and LMMS will not load any project that has local paths outside that list. If we simply allow local paths to be considered on The way local paths works right now is not set in stone, so we can slowly change and improve it in the future to transition our projects to use it too (besides the bundles), but there are things to consider while doing so that I believe are out of the scope of this PR, we'd probably be able to cover them better by doing it incrementally. |
In either scenario the caller has to be aware of the behavior, which is only specified in the actual method implementation. It makes sense to me that if there's no file saved yet, we assume the local directory is as high up in the hierarchy as possible. I find
Incremental improvement is good and all, but I think we need to be careful to avoid "it gets worse before it gets better" changes. Again, |
|
I didn't see it much as "it gets worse before it gets better", but "it's just decent, but it will get better" 😆 I like the ideas you suggested though, would you be good with the changes if I make |
Absolutely! |
Changes some of the PathUtil methods to allow a boolean pointer to be used to return the status of the method, setting it to false if it failed somewhere. Also adds a parameter to toShortestRelative to either allow or forbid local paths in the search for the shortest relative path.
|
Awesome! If nobody's against it I'm merging it tomorrow. @superpaik Just letting you know, I was working on the UI change to have a separate menu for the bundle save, but it involved changing the PR further and would delay the reviewing a little bit. We'll still take it into consideration and improve it, just in a separate PR so we can get to the reorg sooner! |
|
Perfect! Thanks! |
* Adds a baseDir for the local path
Adds a new Base for paths called "local:", which will translate to the dir where the currently opened project file is at. In the future this will allow us to make project bundles and make it easier to export and transfer projects to other people without breaking the paths to samples, presets, plugins and others.
* Starts implementing the makeBundle functionality
For now, to make a bundle LMMS has to be run through CLI with the makeBundle/--makeBundle command, followed by an input file and an output file ('lmms --makeBundle input.mmp output.mmp'). DataFile::writeBundle() is then called. For now, it only saves the mmp/mmpz file normally and also creates a "resources" folder if it doesn't exists. Later it will also manipulate the DataFile so all paths are local and copy all files to the resources folder.
TODO:
-Remove warnings.
-Implement the logic to manipulate the DataFile and copy files.
* Starts implementing logic to go through resources
Starts implementing the logic that will go through all the resources of the project file and add them to the bundle. We use a std::map of QString to std::vector<QString>: The first string is the DOM element tagname that is going to be searched for. The vector of strings holds all attributes this element can have that accesses resources. For now we just print those to the screen.
* Adds logic to copy files and update the project
The raw logic for creating the bundle is finished. It now copies the resource files and update the project to use "local:" paths to the new file now.
Now it's a matter of organizing things and adding safety checks for file operation errors basically.
* Makes the writeBundle method more organized
Improves comments and debugging warnings to make the writeBundle a bit more organized for review.
* Adds a project bundle folder
Adds a project bundle folder, inside which the bundles will be created. Instead of receiving an output project file name, the makeBundle command now receives a bundle name that will be used as the name of the bundle's folder.
Uses a typedef for the std::map with the tags and attributes with resources.
TODO:
- Fix the local: prefix so it works when we don't have the project file open (for CLI usage, or find another way to deal with it).
- Sanitize the bundle name.
- Allow overwriting bundles?
* Handles local paths when a project isn't open
The PathUtil base prefix conversion for "local:" uses the loaded song file name. When we are running the makebundle command from the CLI there isn't a loaded project, so those prefixes aren't converted properly. Now, when there isn't a project open PathUtil will return "local:" again when it tries to convert this base prefix. DataFile can then check if the base prefix is still there, and if it is it knows the conversion wasn't possible, so it does the conversion itself. To do that, a member called m_fileName was added to DataFile, which will hold the file being manipulated if that's where the DataFile originated from, and the local path can be retrieved from it.
* Sanitizes the bundle name
The bundle name is now sanitized. Since it's going to be used as a folder name, we need to keep the user from giving invalid folder names as a bundle's name. The rules for the name are:
1) It must start with a word character (either a digit or letter)
2) It can be followed by any number of letters, digits, whitespaces or hyphens
3) It must end with a word character (either a digit or letter)
A Regexp is used to check for the name validity.
* Moves away from projectbundle folder concept
This commit regresses some functionality. Project bundles will be saved just as any other project, except they will also have the resources folder. It will be up to the user to organize the bundles on their own folders. It's currently not allowed to save a bundle on a folder where there's one already though (if there's a resources folder already). Later it might be allowed to overwrite bundles in that case.
The projectbundles folder was dropped. The user can save project bundles anywhere in the system.
The DataFile::writeBundle was removed. It's functionality was merged into the DataFile::writeFile method, by adding a boolean on the parameters defining whether it should be saved with resources. The logic of copying the resource files and changing the paths inside the project DataFile was moved to DataFile::copyResources, making the methods a little bit less dense.
* Adds an option to save project as bundle
The "Save As" dialog now has an option to save project as a project bundle (with resources), which will save the file as a bundle.
Known bug:
- Because the "local:" base prefix is translated to the filename from the Engine::getSong(), it breaks when Song::guiSaveProjectAs is called, because that method changes the project name before saving. Urgent fix!
* Fix local: prefix saving bug
There was a bug where "local:" prefixes weren't resolved properly during saving because Song::guiSaveProjectAs() changed the project name to the destiny file name before saving. This resulted in the local paths using the destination file as a reference. Both Song::guiSaveProject() and Song::guiSaveProjectAs() were rewritten, and now they only rename the project after it's saved.
* Adds a warning message box
When the user tries to save a project bundle on a folder that already has a project bundle (contains a resources folder) a message box pops up telling the user it's not permitted and that another path should be chosen.
* Removes unused header
Forgot to remove <QRegExp> header when I removed the code that used it.
* Removes Vestige plugins bundling
For safety reasons, remove the possibility to bundle VSTs loaded
through vestige. Also runs a safety check during the project being
loaded (Song::loadProject) to check if either Vestige plugins or effect
plugins are using local paths, and abort the project load if so. That is
to avoid malicious code being run because of bad DLLs being shipped with
a project file.
* Extracts code from loadProject to another method
Extracts code that checks if a DataFile contains local paths to
plugins to another method inside DataFile.
* Removes debug warnings
Removes warnings previously used for debugging. Improves a
warning message on PathUtil.
* Fixes small bug with error logging
Fixes small bug, where a QMessageBox was being used to prompt an
error without checking if the gui is loaded first. Now we check for the
GUI and if we are in CLI mode we use a QTextStream instead.
* Saves the bundle in a newly created folder
Now a folder with the project name is created inside which the
bundle will be saved. This makes the process more convenient.
Some save errors that previously only triggered qWarnings now
trigger message boxes to warn the user of what happened (using a lambda
function that either shows message boxes or trigger qWarnings depending
whether a gui is present).
Makes it so saving a bundle doesn't change the loaded project
path, that way the user won't be able to accidentally "Save" over a
bundle which should not be done for now.
* Enhances the name conflict workaround
Now, instead of replacing the resource names with meaningless
numbers, the bundle save will just append a counter to the end of
filenames that have been repeated.
* Starts addressing Johannes review
* Adds makebundle action to bash completion file
Adds the bash completion code for the made bundle action.
* Improves safety check on project files
Now, instead of checking certain XML tags for local paths,
DataFile::hasLocalPlugin() will return true if ANY tag that isn't on the
RESOURCE_ELEMENTS list contains an attribute that starts with "local:".
The method is now recursive so it can go through all XML tags
during this check.
* Addresses Spekular change request
Uses basePrefix(Base::LocalDir) instead of "local:" on the
return of unresolved local paths.
* Makes hasLocalPlugins method const
* Replaces literal uses of "local:"
Instead of using "local:" we are now retrieving the base prefix
from PathUtil, so if we change the prefix on the future we don't need to
replace every mention to it as well.
* Fix some comments on the header and cpp file
* Changes variable on PathUtil to const
Changes the retrieved pointer to the song object to a const
pointer.
* Leave doxygen comment on CPP file only
There was 2 doxygen comments for the same method, on the header
and CPP file. The latter was kept since it goes into more details about
the functionality of the method.
* Fix doxygen comment @param
Fixes the doxygen comment from hasLocalPlugin().
* Remove assert statements
Some assert statements were being done wrong and are probably
even unnecessary for that piece of code, so they were removed.
* Skips local paths when looking for shortest path
PathUtil::toShortestRelative() was including the local paths on
the candidate paths, which could lead to a unallowed resource (i.e.:
vst plugin) to be assigned a local path even on a regular save.
The local paths are now skipped when looking for the shortest
relative path, since they should only be used by the bundle save on the
allowed resources.
* Address Spekular's review
Changes some of the PathUtil methods to allow a boolean pointer
to be used to return the status of the method, setting it to false if it
failed somewhere.
Also adds a parameter to toShortestRelative to either allow or
forbid local paths in the search for the shortest relative path.
* Replaces "ok" with "error"
* Adds a baseDir for the local path
Adds a new Base for paths called "local:", which will translate to the dir where the currently opened project file is at. In the future this will allow us to make project bundles and make it easier to export and transfer projects to other people without breaking the paths to samples, presets, plugins and others.
* Starts implementing the makeBundle functionality
For now, to make a bundle LMMS has to be run through CLI with the makeBundle/--makeBundle command, followed by an input file and an output file ('lmms --makeBundle input.mmp output.mmp'). DataFile::writeBundle() is then called. For now, it only saves the mmp/mmpz file normally and also creates a "resources" folder if it doesn't exists. Later it will also manipulate the DataFile so all paths are local and copy all files to the resources folder.
TODO:
-Remove warnings.
-Implement the logic to manipulate the DataFile and copy files.
* Starts implementing logic to go through resources
Starts implementing the logic that will go through all the resources of the project file and add them to the bundle. We use a std::map of QString to std::vector<QString>: The first string is the DOM element tagname that is going to be searched for. The vector of strings holds all attributes this element can have that accesses resources. For now we just print those to the screen.
* Adds logic to copy files and update the project
The raw logic for creating the bundle is finished. It now copies the resource files and update the project to use "local:" paths to the new file now.
Now it's a matter of organizing things and adding safety checks for file operation errors basically.
* Makes the writeBundle method more organized
Improves comments and debugging warnings to make the writeBundle a bit more organized for review.
* Adds a project bundle folder
Adds a project bundle folder, inside which the bundles will be created. Instead of receiving an output project file name, the makeBundle command now receives a bundle name that will be used as the name of the bundle's folder.
Uses a typedef for the std::map with the tags and attributes with resources.
TODO:
- Fix the local: prefix so it works when we don't have the project file open (for CLI usage, or find another way to deal with it).
- Sanitize the bundle name.
- Allow overwriting bundles?
* Handles local paths when a project isn't open
The PathUtil base prefix conversion for "local:" uses the loaded song file name. When we are running the makebundle command from the CLI there isn't a loaded project, so those prefixes aren't converted properly. Now, when there isn't a project open PathUtil will return "local:" again when it tries to convert this base prefix. DataFile can then check if the base prefix is still there, and if it is it knows the conversion wasn't possible, so it does the conversion itself. To do that, a member called m_fileName was added to DataFile, which will hold the file being manipulated if that's where the DataFile originated from, and the local path can be retrieved from it.
* Sanitizes the bundle name
The bundle name is now sanitized. Since it's going to be used as a folder name, we need to keep the user from giving invalid folder names as a bundle's name. The rules for the name are:
1) It must start with a word character (either a digit or letter)
2) It can be followed by any number of letters, digits, whitespaces or hyphens
3) It must end with a word character (either a digit or letter)
A Regexp is used to check for the name validity.
* Moves away from projectbundle folder concept
This commit regresses some functionality. Project bundles will be saved just as any other project, except they will also have the resources folder. It will be up to the user to organize the bundles on their own folders. It's currently not allowed to save a bundle on a folder where there's one already though (if there's a resources folder already). Later it might be allowed to overwrite bundles in that case.
The projectbundles folder was dropped. The user can save project bundles anywhere in the system.
The DataFile::writeBundle was removed. It's functionality was merged into the DataFile::writeFile method, by adding a boolean on the parameters defining whether it should be saved with resources. The logic of copying the resource files and changing the paths inside the project DataFile was moved to DataFile::copyResources, making the methods a little bit less dense.
* Adds an option to save project as bundle
The "Save As" dialog now has an option to save project as a project bundle (with resources), which will save the file as a bundle.
Known bug:
- Because the "local:" base prefix is translated to the filename from the Engine::getSong(), it breaks when Song::guiSaveProjectAs is called, because that method changes the project name before saving. Urgent fix!
* Fix local: prefix saving bug
There was a bug where "local:" prefixes weren't resolved properly during saving because Song::guiSaveProjectAs() changed the project name to the destiny file name before saving. This resulted in the local paths using the destination file as a reference. Both Song::guiSaveProject() and Song::guiSaveProjectAs() were rewritten, and now they only rename the project after it's saved.
* Adds a warning message box
When the user tries to save a project bundle on a folder that already has a project bundle (contains a resources folder) a message box pops up telling the user it's not permitted and that another path should be chosen.
* Removes unused header
Forgot to remove <QRegExp> header when I removed the code that used it.
* Removes Vestige plugins bundling
For safety reasons, remove the possibility to bundle VSTs loaded
through vestige. Also runs a safety check during the project being
loaded (Song::loadProject) to check if either Vestige plugins or effect
plugins are using local paths, and abort the project load if so. That is
to avoid malicious code being run because of bad DLLs being shipped with
a project file.
* Extracts code from loadProject to another method
Extracts code that checks if a DataFile contains local paths to
plugins to another method inside DataFile.
* Removes debug warnings
Removes warnings previously used for debugging. Improves a
warning message on PathUtil.
* Fixes small bug with error logging
Fixes small bug, where a QMessageBox was being used to prompt an
error without checking if the gui is loaded first. Now we check for the
GUI and if we are in CLI mode we use a QTextStream instead.
* Saves the bundle in a newly created folder
Now a folder with the project name is created inside which the
bundle will be saved. This makes the process more convenient.
Some save errors that previously only triggered qWarnings now
trigger message boxes to warn the user of what happened (using a lambda
function that either shows message boxes or trigger qWarnings depending
whether a gui is present).
Makes it so saving a bundle doesn't change the loaded project
path, that way the user won't be able to accidentally "Save" over a
bundle which should not be done for now.
* Enhances the name conflict workaround
Now, instead of replacing the resource names with meaningless
numbers, the bundle save will just append a counter to the end of
filenames that have been repeated.
* Starts addressing Johannes review
* Adds makebundle action to bash completion file
Adds the bash completion code for the made bundle action.
* Improves safety check on project files
Now, instead of checking certain XML tags for local paths,
DataFile::hasLocalPlugin() will return true if ANY tag that isn't on the
RESOURCE_ELEMENTS list contains an attribute that starts with "local:".
The method is now recursive so it can go through all XML tags
during this check.
* Addresses Spekular change request
Uses basePrefix(Base::LocalDir) instead of "local:" on the
return of unresolved local paths.
* Makes hasLocalPlugins method const
* Replaces literal uses of "local:"
Instead of using "local:" we are now retrieving the base prefix
from PathUtil, so if we change the prefix on the future we don't need to
replace every mention to it as well.
* Fix some comments on the header and cpp file
* Changes variable on PathUtil to const
Changes the retrieved pointer to the song object to a const
pointer.
* Leave doxygen comment on CPP file only
There was 2 doxygen comments for the same method, on the header
and CPP file. The latter was kept since it goes into more details about
the functionality of the method.
* Fix doxygen comment @param
Fixes the doxygen comment from hasLocalPlugin().
* Remove assert statements
Some assert statements were being done wrong and are probably
even unnecessary for that piece of code, so they were removed.
* Skips local paths when looking for shortest path
PathUtil::toShortestRelative() was including the local paths on
the candidate paths, which could lead to a unallowed resource (i.e.:
vst plugin) to be assigned a local path even on a regular save.
The local paths are now skipped when looking for the shortest
relative path, since they should only be used by the bundle save on the
allowed resources.
* Address Spekular's review
Changes some of the PathUtil methods to allow a boolean pointer
to be used to return the status of the method, setting it to false if it
failed somewhere.
Also adds a parameter to toShortestRelative to either allow or
forbid local paths in the search for the shortest relative path.
* Replaces "ok" with "error"
This PRs introduces 2 changes:
local:to be used on the project file paths. That prefix translates to a local path in relation to that project file path.lmms --makeBundle ~/lmms/projects/input.mmp ~/lmms/projets/bundle/output.mmp) or theSave Asdialog (by markingSave as Project Bundle (with resources)).A project bundle is basically a project file and a resources folder, containing all external files used by the project (i.e.: samples, presets, plugins). The logic for the creation of the bundle goes as following:
std::mapon DataFile.cpp calledELEMENTS_WITH_RESOURCES. It maps a QString (the TagName of the element) to astd::vectorof QStrings (the attributes of that element that are paths to resources). New elements can be easily added to the map so they are also addressed when creating the bundle.resourceson the same path as the output bundle projectELEMENTS_WITH_RESOURCES, check if any of the attributes that links to paths are present. If they are, get an absolute path to the resource, copy that resource to theresourcesdirectory created earlier and update the attribute to point to that copied file (local:resources/filename).To avoid name conflicts between files inside the resource folder, all of them are renamed to a number followed by the extension of the file (i.e.:
1.wav,2.dll).It's not allowed to save a bundle on the same path where there's also another bundle. I first thought of allowing bundles being overwritten, but I'm aiming towards simplicity on this PR: Overwritting requires that I remove the existing resources folder, which is fine. Except that there's the possibility that the project being saved is that bundle itself, so you remove its resources before copying them to the new resources folder. A temporary folder could be used for the operation, but then there's the issue of guaranteeing an unique name for the temporary folder. So initially I'm submitting this PR with this operation not allowed (and the appropriate feedback to the user explaining it). After it goes through testing and code review and it merged, we can work towards allowing the overwrites if it's a wanted feature (personally I see project bundles as a way to make backups and share projects between different systems, so overwritting doesn't sound like a very important operation on this context).
For testers
Basically test the overall functionality. Try to save project bundles and test them, check if any resources are missing, try to overwrite existing bundles, saving a project bundle from a project that is also a project bundle. Be careful to work with backup files and in a safe folder.
For code reviewers
The logic behind this feature is:
local:base prefix by checking the project file name. If a project isn't open (when running from CLI for example)local:is returned again so the method calling the PathUtil method can resolve the path itself.DataFile::writeFilewas rewritten to allow saving with resources depending on the value of a boolean on its parameters. The method that copies the files to the resources folder and updates the paths on the DataFile isDataFile::copyResources.main.cppchecks for the--makeBundlecommand to create a project bundle from the CLI.MainWindow.cppandSong.cppwere edited so they account for the possibility of saving with resources. For the Save As dialog, a newsaveOptionwas added to save as a project bundle.There are warnings that I left on
DataFile::writeFilefor debugging purposes. Most will be either removed or replaced by an appropriate user warning.