Skip to content

add [ChannelX],loaded ControlObject#1082

Merged
rryan merged 5 commits intomixxxdj:masterfrom
Be-ing:loaded_co
Dec 20, 2016
Merged

add [ChannelX],loaded ControlObject#1082
rryan merged 5 commits intomixxxdj:masterfrom
Be-ing:loaded_co

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Dec 19, 2016

Previously, controller mappings and skins had to use track_samples to detect if a deck was loaded. This is not obvious and was often a point of confusion for people new to Mixxx.

Previously, controller mappings and skins had to use track_samples to detect if a deck was loaded. This is not obvious and was often a point of confusion for people new to Mixxx.
Comment thread src/engine/enginebuffer.cpp Outdated
this, SLOT(slotEjectTrack(double)),
Qt::DirectConnection);

m_pTrackLoaded = new ControlPushButton(ConfigKey(m_group, "loaded"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you name it track_loaded?

Comment thread src/engine/enginebuffer.cpp Outdated
// Set play here, to signal the user that the play command is adopted
m_playButton->set((double)m_bPlayAfterLoading);
m_pTrackSamples->set(0); // Stop renderer
m_pTrackLoaded->set(1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not loaded at this point -- this signals CachingReader to load the track and see if it's valid.

We should set to zero here, then 1 in slotTrackLoaded (line 519, where we set track_samples).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Whoops, that's what I meant to do.

Comment thread src/engine/enginebuffer.cpp Outdated
this, SLOT(slotEjectTrack(double)),
Qt::DirectConnection);

m_pTrackLoaded = new ControlPushButton(ConfigKey(m_group, "track_loaded"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is not a PushButton just
new ControlObject() fits better

Since this is a read only type, you should use:
connectValueChangeRequest
and setAndConfirm
like here:

m_pStatusCO = new ControlObject(ConfigKey(BROADCAST_PREF_KEY, "status"));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please delete m_pTrackLoaded in the destructor.

(Or lift the whole bunch of COs in this file to std:uniqu_ptr())

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread src/engine/enginebuffer.cpp Outdated
TrackPointer pOldTrack = m_pCurrentTrack;

m_pause.lock();
m_pTrackLoaded->set(1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The user might think that all other track related COs are valid after TrackLoaded is 1.0.
So setting the CO should be the very last action.

Comment thread src/engine/enginebuffer.cpp Outdated
m_dSlipRate = 0;
// Reset the pitch value for the new track.
m_pause.unlock();
m_pTrackLoaded->setAndConfirm(1);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it's after pause.unlock() then the engine could view these COs in an inconsistent state -- best to stick it above the unlock.

Comment thread src/engine/enginebuffer.cpp Outdated
this, SLOT(slotEjectTrack(double)),
Qt::DirectConnection);

m_pTrackLoaded = new ControlObject(ConfigKey(m_group, "track_loaded"), 0, 1);
Copy link
Copy Markdown
Member

@rryan rryan Dec 20, 2016

Choose a reason for hiding this comment

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

Why dont-ignore-nops (0) and track (1)? (also better to use true/false instead of ints)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm confused. This is read-only, it ignores all writes.

Copy link
Copy Markdown
Member

@rryan rryan Dec 20, 2016

Choose a reason for hiding this comment

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

Right, I'm asking why you're passing values for the 2nd and 3rd args here (2nd arg is ignore-nops, and 3rd arg is to enable tracking). Maybe they're left over from it being a potmeter or something? You can leave off all but the first for a ControlObject.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they're left over from it being a ControlPushButton. I'll remove them.

@rryan
Copy link
Copy Markdown
Member

rryan commented Dec 20, 2016

Nice, thanks!

@rryan rryan merged commit 1c93830 into mixxxdj:master Dec 20, 2016
@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 20, 2016

Added to wiki

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