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

Support dragging track from deck to deck #173

Merged
merged 8 commits into from Feb 11, 2014
Merged

Support dragging track from deck to deck #173

merged 8 commits into from Feb 11, 2014

Conversation

loadstar81
Copy link
Contributor

This is my branch corresponding to https://bugs.launchpad.net/mixxx/+bug/1173481

Please look it over if you have time and leave me some feedback. I had some trouble figuring out how to unit test the new functionality, but would gladly implement tests if I was pointed in the right direction.

Thanks!

@esbrandt
Copy link
Contributor

esbrandt commented Feb 6, 2014

Thanks for picking this up.
Tested on OSX 10.8.5, and it works like a charm.

Drag from Deck to Deck = OK
Drag from Deck to Sampler, and vice versa = OK
Drag from Deck/Sampler to Preview Deck, and vice versa = OK
Respects Track load behavior set in Preferences = OK

One possible addition would be to display the WTrackText, not just the dnd icon when dragging. Nonetheless, it is cool as it stands :-)

@@ -782,19 +782,22 @@ QWidget* LegacySkinParser::parseVisual(QDomElement node) {

QWidget* LegacySkinParser::parseText(QDomElement node) {
QString channelStr = lookupNodeGroup(node);
const char* pSafeChannelStr = safeChannelString(channelStr);
Copy link
Member

Choose a reason for hiding this comment

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

This is off topic and the rest of the code is also effected (not your fault):

IMHO it should be "save" = de:gespeichert not "safe" (security) = de:gesichert
The string is saved into heap, because channelStr will be deleted from stack after returning from parseText().
Maybe a native speaker can verify it.

Because of this, pSafeChannelStr should become pSavedChannelStr.

Copy link
Member

Choose a reason for hiding this comment

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

Safe in this case means "safe to use without crashing" not "save it for later" so I think the current wording is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then safe us from the work ;-)

@daschuer
Copy link
Member

daschuer commented Feb 6, 2014

Hi @loadstar81
thank you for the nice feature. For me this is already in a mergeable state.
If you mind to go an extra mile, it would be nice if you issue my inline comments and an if you avoid code duplication by sub-classing the common DND parts.

@@ -806,20 +809,22 @@ QWidget* LegacySkinParser::parseText(QDomElement node) {

QWidget* LegacySkinParser::parseTrackProperty(QDomElement node) {
QString channelStr = lookupNodeGroup(node);

const char* pSafeChannelStr = safeChannelString(channelStr);
Copy link
Member

Choose a reason for hiding this comment

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

Can you use a QString?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, it looks like we usually use const char* for group names. nevermind.

@loadstar81
Copy link
Contributor Author

Thanks for the review guys! I'll make some changes and rescope.

@loadstar81
Copy link
Contributor Author

Updated the tooltip and changed drag creation to the mouseMove event. I plan to subclass the common dNd parts, but I'll leave that for another pull request!

@rryan
Copy link
Member

rryan commented Feb 11, 2014

Looks good to me! Thanks @loadstar81 :)

@rryan
Copy link
Member

rryan commented Feb 11, 2014

Could you sign the contributor agreement? I think your last commits were from before we started doing this: https://docs.google.com/a/mixxx.org/spreadsheet/viewform?usp=drive_web&formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ#gid=0

This just keeps a record of our permission to distribute your changes with Mixxx.

rryan added a commit that referenced this pull request Feb 11, 2014
Support dragging track from deck to deck
@rryan rryan merged commit f607fb6 into mixxxdj:master Feb 11, 2014
@loadstar81
Copy link
Contributor Author

signed! \m/

esbrandt added a commit to mixxxdj/manual that referenced this pull request Feb 12, 2014
m0dB pushed a commit to m0dB/mixxx that referenced this pull request Jan 21, 2024
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.

None yet

5 participants