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

Fix broken data file upgrade #6279

Merged
merged 3 commits into from
Jan 15, 2022
Merged

Conversation

M374LX
Copy link
Contributor

@M374LX M374LX commented Jan 14, 2022

Addresses the data file upgrade breakage introduced in #6226.

@LmmsBot
Copy link

LmmsBot commented Jan 14, 2022

🤖 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

🤖
{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/pqj97x6x6ma70x9e/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42214771"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/90dgk91511xokge0/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/42214771"}]}, "commit_sha": "8b2a309148ef714bad2cf62467238577105b2f39"}

@JohannesLorenz JohannesLorenz self-requested a review January 15, 2022 08:01
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Code review finished. Issues:

  • In DataFile.cpp, ELEMENTS_WITH_RESOURCES still uses sampleclip.

Testing is required after that.

@M374LX M374LX requested a review from JohannesLorenz January 15, 2022 16:05
Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Sorry, last time I have forgotten to check the headers. But you seem to have them complete now, too. LGTM. Now, testing review required.

@JohannesLorenz
Copy link
Contributor

Test review minimum:

The following should work with all kinds of clips (Automation Clip, BB Clip, MidiClip (these are the clips with MIDI notes), SampleClip):

  • drag/drop a clip
  • save a file with that clip type
  • load a file (which you created with this PR) with that clip type
  • load a file (which you did not create with this PR) with that clip type

@M374LX
Copy link
Contributor Author

M374LX commented Jan 15, 2022

Another problem found: when dragging and dropping a sample track from the Song Editor to the Beat+Bassline Editor, it does not work.

Update: Apparently, this happened because I accidentally opened an invalid sample file, which is a bug but out of the scope of this PR (more details: #6283).

Other than this, everything seems to work fine in my tests.

@JohannesLorenz
Copy link
Contributor

Oh, OK. Then I'd say let's merge this? It can't get much worse 🥲

@M374LX
Copy link
Contributor Author

M374LX commented Jan 15, 2022

For me, it is ready to be merged.

@JohannesLorenz JohannesLorenz merged commit 7fe2152 into LMMS:master Jan 15, 2022
@M374LX M374LX deleted the datafile-upgrade-fix branch January 15, 2022 21:20
allejok96 pushed a commit to allejok96/lmms that referenced this pull request Jan 20, 2022
Fixes issues introduced in previous commit (55d361f),
which affected loading, saving and drag-dropping of clips.
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.

3 participants