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.

Sign up for GitHub

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

Jump to bottom

PlayerManager: Identify players by ChannelHandle #2969

Merged
merged 6 commits into from
Jul 31, 2020
Merged

PlayerManager: Identify players by ChannelHandle #2969

merged 6 commits into from
Jul 31, 2020

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Jul 27, 2020

No description provided.

Be-ing
Be-ing reviewed Jul 27, 2020
View reviewed changes
src/engine/channelhandle.h Outdated Show resolved Hide resolved
Be-ing
Be-ing reviewed Jul 27, 2020
View reviewed changes
src/mixer/basetrackplayer.cpp Outdated
bool defaultMaster,
bool defaultHeadphones,
bool primaryDeck)
: BaseTrackPlayer(pParent, group),
: BaseTrackPlayer(pParent, handle_group.name()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you make BaseTrackPlayer also store the ChannelHandleAndGroup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could, but this would require many more changes, and I don't want to go too deeply into this for now.

Be-ing
Be-ing reviewed Jul 27, 2020
View reviewed changes
src/mixer/playermanager.cpp
QString group = groupForDeck(m_decks.count());
VERIFY_OR_DEBUG_ASSERT(!m_players.contains(group)) {
ChannelHandleAndGroup handle_group =
m_pEngine->registerChannelGroup(groupForDeck(m_decks.count()));
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels hacky. It seems EngineMaster::registerChannelGroup is public only for the tests. I think using friend class would be better for that. For here, let's pass the ChannelHandleFactory to PlayerManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it is used in a few more places. As I said below, it is not really "register"ing anything, so maybe it would be enough to rename it?

Be-ing
Be-ing reviewed Jul 27, 2020
View reviewed changes
src/mixer/playermanager.cpp Outdated Show resolved Hide resolved
Be-ing
Be-ing reviewed Jul 27, 2020
View reviewed changes
src/mixer/playermanager.h Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jul 27, 2020

Thanks for working on this. I think we should build upon this going forward to remove the hacky uses of QStrings to identify things throughout Mixxx.

Holzhaus
Holzhaus requested changes Jul 27, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks!

src/engine/channelhandle.h
Comment on lines +57 to +64
inline bool operator>(const ChannelHandle& h1, const ChannelHandle& h2) {
return h1.handle() > h2.handle();
}

inline bool operator<(const ChannelHandle& h1, const ChannelHandle& h2) {
return h1.handle() < h2.handle();
}

Copy link
Member

Choose a reason for hiding this comment

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

Are these really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, for using them as Key in a Map

src/engine/enginemaster.h Outdated
@@ -66,7 +66,7 @@ class EngineMaster : public QObject, public AudioSource {
// be called by SoundManager.
const CSAMPLE* buffer(AudioOutput output) const;

ChannelHandleAndGroup registerChannelGroup(const QString& group) {
ChannelHandleAndGroup registerChannelGroup(const QString& group) const {
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be const if it modifies a member object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't modify anything?
I also thought about renaming this method, it basically just does a conversion, it is more of a getter rather than something that "registers" anything-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it does kinda modify as it might create a handle, though it is questionable how much that matters since the function is still idempotent as seen from outside the ChannelHandleFactory. Removed const for now.

src/mixer/previewdeck.h Outdated Show resolved Hide resolved
src/test/signalpathtest.h Outdated Show resolved Hide resolved
src/test/signalpathtest.h Outdated Show resolved Hide resolved
daschuer
daschuer requested changes Jul 28, 2020
View reviewed changes
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you for this useful PR.

src/engine/channelhandle.h Outdated Show resolved Hide resolved
src/mixer/playermanager.cpp Outdated Show resolved Hide resolved
src/mixer/playermanager.cpp Outdated Show resolved Hide resolved
@xeruf xeruf requested review from Holzhaus and Be-ing July 31, 2020 07:04
Holzhaus
Holzhaus requested changes Jul 31, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

I'm getting a DEBUG_ASSERT when starting Mixxx:

DEBUG ASSERT: "!m_players.contains(handleGroup.handle())" in function void PlayerManager::addSamplerInner() at /home/jan/Projects/mixxx/src/mixer/playermanager.cpp:440
Aborted (core dumped)

@xeruf
Copy link
Contributor Author

xeruf commented Jul 31, 2020

Oh, dumb mistake ^^ thanks for catching it :)

@xeruf
Copy link
Contributor Author

xeruf commented Jul 31, 2020

Fixed it, but I think we should have a test for that as well...

@Holzhaus
Copy link
Member

Fixed it, but I think we should have a test for that as well...

You didn't push though :D

Holzhaus
Holzhaus approved these changes Jul 31, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thank you! Waiting for CI.

@Holzhaus Holzhaus merged commit 1ea7cea into mixxxdj:master Jul 31, 2020
@xeruf xeruf deleted the playermanager-handle branch July 31, 2020 14:55
@xeruf xeruf mentioned this pull request Aug 29, 2020
Closed
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer approved these changes

@Holzhaus Holzhaus Holzhaus approved these changes

@Be-ing Be-ing Awaiting requested review from Be-ing

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@xeruf @Be-ing @Holzhaus @daschuer