Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Pattern import/export #5891

Merged
merged 7 commits into from Mar 21, 2021
Merged

Conversation

cyber-bridge
Copy link
Contributor

Continuation of #5197

This makes it possible to export and import patterns. They will be saved to a XML document. Currently with the file-extension .xpt and .xtpz for compressed files.

Closes #3393

@LmmsBot
Copy link

LmmsBot commented Jan 21, 2021

🤖 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://12628-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12628?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://12629-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12629?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/h28i03249qbosn1j/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37953778"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/1g5o2oboqxetl3cr/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37953778"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12627-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bgff2ec42-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12627?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12626-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12626?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "16852a8084f290e1b8bb9ec8191d6cab527a6b6f"}

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, this review mostly points some code style or formatting minor issues. There's also one suggestion on using bool instead of int as a return type of savePatternXML. I think it makes more sense as a return type, even though what you have now is POSIX compliant 😄

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
include/PianoRoll.h Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
include/PianoRoll.h Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@cyber-bridge
Copy link
Contributor Author

cyber-bridge commented Jan 27, 2021 via email

Copy link
Contributor

@IanCaio IanCaio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick response!

Code LGTM! I'm leaving a last change request: It's a minor thing, code works fine the way it's written but I think it's a better habit for us to use TimePos instead of int where it's expected, even though they can be interchangeable.

I'm approving the PR despite that last change suggestion. Nice work!

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved
@cyber-bridge
Copy link
Contributor Author

cyber-bridge commented Jan 27, 2021 via email

@PhysSong
Copy link
Member

Looks good. Could you look into DataFile class so it's possible to use it for data serialization?

@superpaik
Copy link
Contributor

It works well to me.
Only one unexpected behaviour. The folder that is opened to both import/export is the last folder used (being that by import/export, audiofileprocessor -select sample-, sampletrack -select track-, save project, etc) even if cancel action on selecting the folder is been pressed. I don't know if that's the best option from the user point of view. Maybe the default behaviour should be: if no import/export have been executed, then project folder (if exists, if not current folder whatever that is); if import/export have been executed, then that should be the folder used for the next import/export. Just a suggestion.

@cyber-bridge
Copy link
Contributor Author

cyber-bridge commented Jan 28, 2021 via email

@IanCaio
Copy link
Contributor

IanCaio commented Jan 28, 2021

Recent changes look good to me!

I agree that it would be nice to have a pattern folder, maybe accessible from the sidebar, but could we leave this for a future enhancement? We're running a little bit against time to make the code base refactoring happen, so maybe we can keep the feature in its state, which is perfectly functional, and leave the improvements for after the refactor?

@cyber-bridge
Copy link
Contributor Author

cyber-bridge commented Jan 28, 2021 via email

@PhysSong
Copy link
Member

A remark on names: Pattern can be renamed to something like MidiClip during the reorganization(see #5592).

Comment on lines 4837 to 4838
QString extFilter("XML Pattern (*.xpt *.xptz)");
FileDialog exportDialog(this, tr("Export pattern"), "", extFilter);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some minor inconsistencies between exportPattern() and importPattern():

  • XML Pattern vs. XML pattern file. I'm not sure if XML pattern describe well what the file is.
  • This file extension filter in this function is not translatable while the one in importPattern() is. Also, only this function uses an explicit temporary QString.

@PhysSong
Copy link
Member

@cyber-bridge Have you read my latest comment?

@cyber-bridge
Copy link
Contributor Author

cyber-bridge commented Feb 20, 2021 via email

@IanCaio
Copy link
Contributor

IanCaio commented Mar 20, 2021

@PhysSong You think this PR is ready for merge after those latest changes?

@PhysSong
Copy link
Member

@PhysSong You think this PR is ready for merge after those latest changes?

Yes.

@IanCaio IanCaio merged commit 4a99904 into LMMS:master Mar 21, 2021
devnexen pushed a commit to devnexen/lmms that referenced this pull request Apr 10, 2021
* Init

* Suggested changes by @IanCaio, thanks!

* Selecting one file to import is enough.

* Explicit use of TimePos in favour of int where expected, as suggested.

* Make pattern import/export future proof with using DataFile instead of custom code to read/write the pattern file.

* Remove unused/duplicate imports

* Make import/export dialogs file-ext filter consistent.

Co-authored-by: CYBERDEViL <cyberdevil@notabug.org>
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Init

* Suggested changes by @IanCaio, thanks!

* Selecting one file to import is enough.

* Explicit use of TimePos in favour of int where expected, as suggested.

* Make pattern import/export future proof with using DataFile instead of custom code to read/write the pattern file.

* Remove unused/duplicate imports

* Make import/export dialogs file-ext filter consistent.

Co-authored-by: CYBERDEViL <cyberdevil@notabug.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import/export native pattern data
5 participants