Feature: Pattern import/export #5891

cyber-bridge · 2021-01-21T16:15:40Z

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 · 2021-01-21T16:21:49Z

🤖 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

Windows 32-bit: lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.3.0-alpha-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.3.0-alpha-msvc2017-win64.exe (build link)

Linux

Linux (AppImage): lmms-1.3.0-alpha.1.75%2Bgff2ec42-linux-x86_64.AppImage (build link)

macOS

macOS : lmms-1.3.0-alpha.1.75%2Bgff2ec42be-mac10.14.dmg (build link)

🤖

{"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"}

IanCaio

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

include/PianoRoll.h

src/gui/editors/PianoRoll.cpp

include/PianoRoll.h

src/gui/editors/PianoRoll.cpp

cyber-bridge · 2021-01-27T17:14:20Z

@IanCaio thanks for the review, I've made the suggested changes.

I also got curious, why does the export requires a `!selectedFiles().first().isEmpty()` check and the import doesn't? Is it because the file mode is `ExistingFiles`, so it will only accept the dialog with an existing file name?

Yes it is. However this question did make me look at that code again and noticed that `FileDialog::ExistingFiles` was used instead of `FileDialog::ExistingFile`; so also changed that.

IanCaio

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

cyber-bridge · 2021-01-27T20:15:14Z

@IanCaio

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.

Agreed :-)

PhysSong · 2021-01-28T06:21:51Z

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

superpaik · 2021-01-28T11:57:04Z

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 · 2021-01-28T17:55:33Z

@PhysSong that's a cool class, adopted the code to use it. @superpaik I get the issue; from what I've tested all other LMMS file dialogs have pre-defined paths (example: {LMMS_HOME}/projects or {LMMS_HOME}/samples). Maybe create a default `pattern` directory and use that as default? Also would it be nice to be able to drag and drop from the sidebar filebrowser into a `InstrumentTrack`?

IanCaio · 2021-01-28T19:50:23Z

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?

…f custom code to read/write the pattern file.

cyber-bridge · 2021-01-28T20:28:08Z

Awesome :-) Yeah leaving it for what it is sounds OK to me. Once it's merged I will create another PR for drag-drop from the sidebar. On the default directory idea: I thought there was a issue on exporting automation tracks, now I can't find it :') But if that's the case it will probably better to use `{LMMS_HOME}/patterns/notes` so potential future (other) patterns can have their own sub-directories. But yeah.. that's future stuff then.

PhysSong · 2021-01-29T06:37:20Z

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

PhysSong · 2021-02-07T07:10:45Z

src/gui/editors/PianoRoll.cpp

+	QString extFilter("XML Pattern (*.xpt *.xptz)");
+	FileDialog exportDialog(this, tr("Export pattern"), "", extFilter);


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 · 2021-02-20T06:14:02Z

@cyber-bridge Have you read my latest comment?

cyber-bridge · 2021-02-20T18:26:58Z

Yes thanks for reminding me! Would "notes pattern" be more descriptive?

IanCaio · 2021-03-20T15:27:17Z

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

PhysSong · 2021-03-21T02:42:08Z

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

Yes.

@IanCaio

* 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>

@IanCaio

* 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>

Init

96160c6

PhysSong mentioned this pull request Jan 22, 2021

Pattern import and export #5197

Closed

IanCaio requested changes Jan 27, 2021

View reviewed changes

CYBERDEViL added 2 commits January 27, 2021 20:00

Suggested changes by @IanCaio, thanks!

c6a30d9

Selecting one file to import is enough.

7d48020

IanCaio approved these changes Jan 27, 2021

View reviewed changes

src/gui/editors/PianoRoll.cpp Outdated Show resolved Hide resolved

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

f2de28a

CYBERDEViL added 2 commits January 28, 2021 20:59

Make pattern import/export future proof with using DataFile instead o…

756044a

…f custom code to read/write the pattern file.

Remove unused/duplicate imports

2794d9e

PhysSong reviewed Feb 7, 2021

View reviewed changes

Make import/export dialogs file-ext filter consistent.

16852a8

IanCaio merged commit 4a99904 into LMMS:master Mar 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Pattern import/export #5891

Feature: Pattern import/export #5891

cyber-bridge commented Jan 21, 2021

LmmsBot commented Jan 21, 2021 •

edited

Loading

Windows

Linux

macOS

IanCaio left a comment

cyber-bridge commented Jan 27, 2021 via email

IanCaio left a comment

cyber-bridge commented Jan 27, 2021 via email

PhysSong commented Jan 28, 2021

superpaik commented Jan 28, 2021

cyber-bridge commented Jan 28, 2021 via email

IanCaio commented Jan 28, 2021

cyber-bridge commented Jan 28, 2021 via email

PhysSong commented Jan 29, 2021

PhysSong Feb 7, 2021

PhysSong commented Feb 20, 2021

cyber-bridge commented Feb 20, 2021 via email

IanCaio commented Mar 20, 2021

PhysSong commented Mar 21, 2021

		QString extFilter("XML Pattern (.xpt .xptz)");
		FileDialog exportDialog(this, tr("Export pattern"), "", extFilter);

Feature: Pattern import/export #5891

Feature: Pattern import/export #5891

Conversation

cyber-bridge commented Jan 21, 2021

LmmsBot commented Jan 21, 2021 • edited Loading

Windows

Linux

macOS

IanCaio left a comment

Choose a reason for hiding this comment

cyber-bridge commented Jan 27, 2021 via email

IanCaio left a comment

Choose a reason for hiding this comment

cyber-bridge commented Jan 27, 2021 via email

PhysSong commented Jan 28, 2021

superpaik commented Jan 28, 2021

cyber-bridge commented Jan 28, 2021 via email

IanCaio commented Jan 28, 2021

cyber-bridge commented Jan 28, 2021 via email

PhysSong commented Jan 29, 2021

PhysSong Feb 7, 2021

Choose a reason for hiding this comment

PhysSong commented Feb 20, 2021

cyber-bridge commented Feb 20, 2021 via email

IanCaio commented Mar 20, 2021

PhysSong commented Mar 21, 2021

LmmsBot commented Jan 21, 2021 •

edited

Loading