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

Cue colors #992

Merged
merged 57 commits into from
Aug 27, 2016
Merged

Cue colors #992

merged 57 commits into from
Aug 27, 2016

Conversation

ferranpujolcamins
Copy link
Contributor

@ferranpujolcamins ferranpujolcamins commented Aug 14, 2016

This is the first step of https://blueprints.launchpad.net/mixxx/+spec/cuepoints-2.0-new
I'll be issuing small PR regularly, instead of a big messy one.

Here I prepare the database and the DAO layer to support cue colors. I also make waveformrendermark to draw the mark according to the cue color. It also draws cue label.

@@ -7,6 +7,8 @@
#include "library/dao/cue.h"
#include "util/assert.h"

const QString defaultColor = "#FF0000";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be enclosed in an anonymous namespace to restrict the visibility of this symbol. Otherwise it is visible globally and might cause conflicts during linking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would use QColor instead of QString for this constant.

@daschuer
Copy link
Member

By the way: Thank you very much for adopting this big topic.

@ferranpujolcamins
Copy link
Contributor Author

Everything's fixed :)

private:
void generateMarkImage(WaveformMark& mark);
void generateMarkImage(WaveformMark* mark);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a minor naming issue: Should be pMark in both this declaration and the definition in the .cpp file.

@uklotzde
Copy link
Contributor

Thank you very much, Ferran!

I did not have the time to test your extensions, maybe sometime during the following days ;)

@daschuer
Copy link
Member

Travis fails:

Failed query: "ALTER TABLE cues ADD COLUMN color INTEGER DEFAULT 0xFFFF0000 NOT NULL" QSqlError(1, "Unable to execute statement", "unrecognized token: "0xFFFF0000"")

@uklotzde
Copy link
Contributor

As I mentioned, hexadecimal integers are supported since SQLite 4.8.6
according to the documentation, but on Travis we are using
4.8.5-some-ubuntu-patchlevel for Linux and ??? for OS X.

I thought this was already fixed?

On 21.08.2016 22:23, Daniel Schürmann wrote:

Travis fails:

Failed query: "ALTER TABLE cues ADD COLUMN color INTEGER DEFAULT
0xFFFF0000 NOT NULL" QSqlError(1, "Unable to execute statement",
"unrecognized token: "0xFFFF0000"")


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#992 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABZ-ahUEBJd2nf_Gs4B38uiI8a0GB3Q7ks5qiLPXgaJpZM4Jj7vm.

@ferranpujolcamins
Copy link
Contributor Author

Sorry, I forgot about that. It is fixed now. Thank your for the review and explanations, I really appreciate it!

Next thing to come is a new combobox to select colors :)

@ferranpujolcamins
Copy link
Contributor Author

A comment would be nice, what this "random" number is.

Done

@ferranpujolcamins
Copy link
Contributor Author

@daschuer @uklotzde please merge this if there's nothing more to be done. I don't want more stale PR that need to be reviewed again in six months.

@uklotzde
Copy link
Contributor

I don't have write permissions ;)

On 27.08.2016 12:39, Ferran Pujol Camins wrote:

@daschuer https://github.com/daschuer @uklotzde
https://github.com/uklotzde please merge this if there's nothing more to
be done. I don't want more stale PR that need to be reviewed again in six
months.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#992 (comment), or
mute the thread
https://github.com/notifications/unsubscribe-auth/ABZ-agHGE9tAmwlG-7vQku5srDG8NseOks5qkBPhgaJpZM4Jj7vm.

@daschuer
Copy link
Member

Thank you very much for this nice first step! :-)

@daschuer daschuer merged commit 11b451e into mixxxdj:master Aug 27, 2016
@ferranpujolcamins
Copy link
Contributor Author

Thank you for your support :)

@esbrandt esbrandt mentioned this pull request Apr 14, 2018
37 tasks
esbrandt added a commit to esbrandt/manual that referenced this pull request Apr 14, 2018
@ferranpujolcamins ferranpujolcamins deleted the CueColors branch May 14, 2019 16:23
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