sync bpm edit fix Lp1808698 #1955
Conversation
|
Yes, I just tested it and it seems to work without issues as i expected it 👍 |
|
Great. Thank you for testing. Who has interests to do the code review? |
src/engine/sync/syncable.h
Outdated
| virtual void requestSyncMode(Syncable* pSyncable, SyncMode mode) = 0; | ||
|
|
||
| // Used by Syncables to tell EngineSync it wants to be enabled in any mode | ||
| // (master/follower). | ||
| virtual void requestEnableSync(Syncable* pSyncable, bool enabled) = 0; | ||
|
|
||
| // A Syncable must never call notifyBpmChanged in response to a setMasterBpm() | ||
| // call. | ||
| virtual void notifyBpmChanged(Syncable* pSyncable, double bpm, |
There was a problem hiding this comment.
Why not a separate signal notifyFileBpmChanged, like notifyInstantaneousBpmChanged? We should either use one signal with an enumeration parameter or separate signals, but not a mixture of both style.
Separate signals seem to make sense here, because we also have separate slots for the different use cases.
There was a problem hiding this comment.
OK, I have refactored the call a bit. I hope I have got your point.
src/engine/bpmcontrol.cpp
Outdated
| @@ -825,7 +825,7 @@ double BpmControl::updateLocalBpm() { | |||
| double BpmControl::updateBeatDistance() { | |||
| double beat_distance = getBeatDistance(getSampleOfTrack().current); | |||
| m_pThisBeatDistance->set(beat_distance); | |||
| if (getSyncMode() == SYNC_NONE) { | |||
| if (getSyncMode() <= SYNC_NONE) { | |||
There was a problem hiding this comment.
The same (or negated) comparison appears way too often and you can easily get it wrong. Please introduce a specialized function for this use case, e.g. bool isSynchronized(SyncMode syncMode) or whatever.
| @@ -52,7 +52,7 @@ Syncable* BaseSyncableListener::getSyncableForGroup(const QString& group) { | |||
|
|
|||
| bool BaseSyncableListener::syncDeckExists() const { | |||
| foreach (const Syncable* pSyncable, m_syncables) { | |||
| if (pSyncable->getSyncMode() != SYNC_NONE && pSyncable->getBaseBpm() > 0) { | |||
| if (pSyncable->getSyncMode() > SYNC_NONE && pSyncable->getBaseBpm() > 0) { | |||
There was a problem hiding this comment.
...and please also add a new member function bool isSynchronized() const for convenience.
|
OK, Done. |
| @@ -64,7 +64,7 @@ int BaseSyncableListener::playingSyncDeckCount() const { | |||
|
|
|||
| foreach (const Syncable* pSyncable, m_syncables) { | |||
| SyncMode sync_mode = pSyncable->getSyncMode(); | |||
| if (sync_mode == SYNC_NONE) { | |||
| if (sync_mode <= SYNC_NONE) { | |||
src/engine/sync/enginesync.cpp
Outdated
| return false; | ||
| } | ||
| continue; | ||
| } | ||
| if (theSyncable->isPlaying() && (theSyncable->getSyncMode() != SYNC_NONE)) { | ||
| if (theSyncable->isPlaying() && (isSynchonized)) { |
src/engine/sync/synccontrol.cpp
Outdated
| // master bpm, our bpm value is adopted. | ||
| m_pEngineSync->requestBpmUpdate(this, bpm); | ||
| } else { | ||
| // SYNC_MASTER |
There was a problem hiding this comment.
DEBUG_ASSERT instead of comment
src/engine/sync/synccontrol.cpp
Outdated
| // Note: bpmcontrol has updated local_bpm just before | ||
| double local_bpm = m_pLocalBpm ? m_pLocalBpm->get() : 0.0; | ||
|
|
||
| if (getSyncMode() <= SYNC_NONE) { |
| @@ -63,8 +63,7 @@ int BaseSyncableListener::playingSyncDeckCount() const { | |||
| int playing_sync_decks = 0; | |||
|
|
|||
| foreach (const Syncable* pSyncable, m_syncables) { | |||
| SyncMode sync_mode = pSyncable->getSyncMode(); | |||
| if (sync_mode == SYNC_NONE) { | |||
| if (!pSyncable->isSynchronized()) { | |||
There was a problem hiding this comment.
if (pSyncable->isSynchronized() && pSyncable->isPlaying()) {
|
done |
|
Thank you for review. |
This fixes the issue that the follower deck changes tempo if you edit the beat grid.
A test is in place confirming that the bug is fixed.