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 metadata when exporting multiple tracks (#4996) #5005

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
2 participants
@JohannesLorenz
Copy link
Contributor

commented Jun 4, 2019

No description provided.

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I'm adding the 1.2.0 label as a reminder. If you want to merge it without review, that's OK. If there's time for a review before, even better.

@JohannesLorenz JohannesLorenz added this to the 1.2.0 milestone Jun 4, 2019

Show resolved Hide resolved src/core/Mixer.cpp Outdated
Show resolved Hide resolved src/core/Mixer.cpp
Mixer.cpp: Fix spelling mistake
Co-Authored-By: Hyunjin Song <tteu.ingog@gmail.com>
@JohannesLorenz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Tested, also tested jack. It all seems to work. Let me know if you want to change something, or if it can be merge.

@PhysSong

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

  • I think you shouldn't include translation updates here.
  • Windows builds are failing because C++11 is disabled for LADSPA:
    # Enable C++11 for CXXFLAGS only and not for Windows
    IF(NOT LMMS_BUILD_WIN32)
    SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
    ENDIF()

    You can find the pretty old CI errors in https://travis-ci.org/LMMS/lmms/jobs/253835510#L1277-L1289, I'm not sure if we can re-enable it now. If std::shared_pointer doesn't work, we should move to QSharedPointer or just revert relevant commits.
@JohannesLorenz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

Do you propose to not add translation updates to this branch or not to stable-1.2?

About the Windows error: Has this been fixed meanwhile? Maybe the CI now works with C++11? If not, I'd say let's move my shared_ptr fix aside and merge the version from yesterday?

I did try QSharedPointer, but the Qt4 version did not have a proper reset function, I didn't like the workarounds.

@PhysSong

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

Do you propose to not add translation updates to this branch or not to stable-1.2?

Merging that commit into stable-1.2 is okay, but not by this pull request.

About the Windows error: Has this been fixed meanwhile?

This patch works(MinGW requires -std=gnu++0x for M_PI):

diff --git a/plugins/LadspaEffect/CMakeLists.txt b/plugins/LadspaEffect/CMakeLists.txt
index 029cd9168..1c500673c 100644
--- a/plugins/LadspaEffect/CMakeLists.txt
+++ b/plugins/LadspaEffect/CMakeLists.txt
@@ -2,8 +2,10 @@ INCLUDE(BuildPlugin)
 
 # Disable C++11
 REMOVE_DEFINITIONS(-std=c++0x)
-# Enable C++11 for CXXFLAGS only and not for Windows
-IF(NOT LMMS_BUILD_WIN32)
+# Enable C++11 for CXXFLAGS only
+IF("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
+	SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=gnu++0x")
+ELSE()
 	SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++0x")
 ENDIF()

Anyway, please feel free to defer changes related to shared pointers to master if you want to do so.

On a side note: there should be no issues with either std::shared_pointer or QSharedPointer on master.

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Would you prefer to apply that C++11 fix to Ladspa and then release with shared_ptr, or only apply shared_ptr on master? The first option may be a bit cleaner, but it's risky to do that on stable-1.2, right?

@PhysSong

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

If you think that change is unnecessary, you can remove the shared pointer part from here.

@JohannesLorenz JohannesLorenz force-pushed the JohannesLorenz:fix-4996 branch from 7a1fe68 to 8e5b7a6 Jun 7, 2019

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

OK, I did an undo of the German translation and and undo of the shared_ptr. I'll merge this now and later add the German translation to stable-1.2 and the shared_ptr to master.

@JohannesLorenz JohannesLorenz merged commit 6e5650c into LMMS:stable-1.2 Jun 7, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.