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

Pattern import and export #5197

Closed
wants to merge 1 commit into from
Closed

Pattern import and export #5197

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 17, 2019

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.

Edit: This PR is ready for testing / review.

Also feel free to ignore/close this if it is not wanted.

Closes #3393

@PhysSong PhysSong added the needs testing This pull request needs more testing label Sep 29, 2019
@ibuki2003
Copy link
Contributor

ibuki2003 commented Oct 1, 2019

I'm pleased with this enhancement.

Worked on Arch Linux environment.

file-extension .xpf, I think this extension needs to change, suggestions?

As you know extension .xpf is already used as 'Preset File'.
The extension doesn't have to contain 'f' for 'File', so I suggest .xpt.

I like to add compression
pattern file should have both formats (without compression) in order to make editing with text editor easier

Finally, I report a problem
when loading failed, succesfully loaded message also shown.

Sorry for my bad English.
Thank you

@LmmsBot
Copy link

LmmsBot commented Oct 1, 2019

🤖 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://5249-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.572-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/5249?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://5252-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.572-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/5252?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://5253-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.572-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/5253?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/ncyeb0cyln6qr89a/artifacts/build/lmms-1.2.1-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/28919507"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/ifk6dki2r0hkl3hj/artifacts/build/lmms-1.2.1-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/28919507"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://5250-15778896-gh.circle-artifacts.com/0/lmms-1.2.1.572-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/5250?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "963ea37687c3fc4e6d255ad20b512b9ac4b99d3f"}

@ghost
Copy link
Author

ghost commented Oct 1, 2019

Thank you for testing this feature.

The extension doesn't have to contain 'f' for 'File', so I suggest .xpt.

What does the t in .xpt stand for?

pattern file should have both formats (without compression) in order to make editing with text editor easier

👍

when loading failed, succesfully loaded message also shown.

This should be fixed now :-)

@ibuki2003
Copy link
Contributor

Sorry for not explaining enough.
pt stands for pattern

@ghost
Copy link
Author

ghost commented Oct 3, 2019

Changed the file extentions to .xpt and .xptz and added compression.

If compression is used depends on if you add .xptz or .xpt to the filename, if no suffix is given then it depends on the Compress project files by default setting.

@tecknixia
Copy link
Contributor

tecknixia commented Oct 15, 2019

Successfully exported and imported patterns over 1000 measures long using .xpt and .xptz, each using both settings... "compress project files by default" selected and unselected.

Checked file sizes for both types in both settings.

Seems to work well, as far as I can tell. Linux Mint 64.

@husamalhomsi
Copy link
Member

@cyberdevilnl I guess the advantage of xpt/xptz over midi is having note pan data, right?

@ghost
Copy link
Author

ghost commented Nov 10, 2019

@Sawuare for me it's more for exporting a pattern from project A and importing it to project B.

@Spekular
Copy link
Member

@cyberdevilnl functioning MIDI import/export would allow the same thing. I believe that's what Sawuare is pointing out, that the only additional data provided by pattern export/import is note pan.

@ghost
Copy link
Author

ghost commented Nov 11, 2019

@Sawuare @Spekular ah yes it would, it is possible with midi too but it's messy since each note would require it's own channel in the midi file.

@husamalhomsi
Copy link
Member

Anyway, I like this feature, but I think it should be presented to the user through the piano roll, not the song editor, because that is where the pattern editing happens.
If pattern MIDI export/import is ever implemented, it should also reside there.

@ghost
Copy link
Author

ghost commented Nov 13, 2019

All right, where should the import/export options/buttons be placed in the Piano-Roll? I can add it to the tools QToolButton if this gets merged: #5216 if that's fine.

If so then I suggest to move the functionality code (void exportPattern(), void importPattern() and int saveXML(QString filepath, bool compress = true)) to Pattern instead of PatternView so we can access it through the Piano-Roll.

@husamalhomsi
Copy link
Member

How about a "File" QToolButton? I think "Tools" should be left for note operations like glue, quantize, reverse, flip, etc.

About moving the functionality code, I say that sounds good.

@ghost
Copy link
Author

ghost commented Nov 16, 2019

All right I'm on it. I have a few questions.

  1. Where should the "File" QToolButton be added? (before, after or in notesActionsToolBar)
  2. Should import only be enabled for patterns without notes? If not then should it merge or overwrite?

@husamalhomsi
Copy link
Member

  1. Before.
  2. No. Overwrite.

@ghost
Copy link
Author

ghost commented Nov 17, 2019

Ok I made the changes, I chose to move the functionality code to PianoRollWindow instead of what I previously suggested to move it to Pattern.

Current behaviour:

  • If no valid pattern is set, then the "File" QToolButton is disabled.
  • On import of a pattern it warns the user that notes will be overwritten if there are any present.

Edit: Will resolve the merge-conflicts asap.

* Cherry-picked changes.
@husamalhomsi
Copy link
Member

On import of a pattern it warns the user that notes will be overwritten if there are any present.

The action should be undoable, so no need for a warning.

@PhysSong
Copy link
Member

@cyber-bridge Can you create a new pull request from your new account?

@cyber-bridge
Copy link
Contributor

cyber-bridge commented Jan 19, 2021 via email

@PhysSong
Copy link
Member

Continued in #5891.

@PhysSong PhysSong closed this Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress needs testing This pull request needs more testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import/export native pattern data
7 participants