Problem with a color class

[zorro68]

I am using Visual Studio 2005 and Qt 4.3.1.

When I call my class "colors" like this:

colors col;
int r=col.getRed(3);

the value of r is bad (always 0). ¿¿Someone knows why??


My colors.h

class colors
{
public:
	colors();
    ~colors();
 
	int getRed(int num);
	enum{max_colors=17};
 
private:
	struct ColorData
	{  
		int num;
		QString name;
		int R;
		int G;
		int B;
	}Propiedades[max_colors],*p;	
};

class colors
{
public:
	colors();
    ~colors();

	int getRed(int num);
	enum{max_colors=17};

private:
	struct ColorData
	{  
		int num;
		QString name;
		int R;
		int G;
		int B;
	}Propiedades[max_colors],*p;	
};

To copy to clipboard, switch view to plain text mode 

and my colors.cpp

#include "colors.h"
 
colors::colors()
{
	ColorData Propiedades[max_colors] =
	{
		{ 0, "black",  0,  0,  0}, 
		{ 1, "white",255,255,255}, 
		{ 2, "darkGray",128,128,128}, 
		{ 3, "gray",160,160,164}, 
		...
	};
	p=&Propiedades[0];
} 
 
int colors::getRed(int num)
{
	ColorData *q;
	q=p;
	for (int i=0;i<=max_colors;i++){
		if (num==q->num) return q->R;
		q++;
	}
	return 0;
}

#include "colors.h"

colors::colors()
{
	ColorData Propiedades[max_colors] =
	{
		{ 0, "black",  0,  0,  0}, 
		{ 1, "white",255,255,255}, 
		{ 2, "darkGray",128,128,128}, 
		{ 3, "gray",160,160,164}, 
		...
	};
	p=&Propiedades[0];
} 

int colors::getRed(int num)
{
	ColorData *q;
	q=p;
	for (int i=0;i<=max_colors;i++){
		if (num==q->num) return q->R;
		q++;
	}
	return 0;
}

To copy to clipboard, switch view to plain text mode 

[jpn]

May I ask why not use QColor?

[zorro68]

As you know (other post), I'm trying to programm a combobox with colors. But I need only some colors and when I programm QColor("name of color") :

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....
...
pixmap.fill(QColor(name));

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....
...
pixmap.fill(QColor(name));

To copy to clipboard, switch view to plain text mode 

some color are bad (for example darkyellow is black for me). So I programm colors with a structure:

struct colortype {QString name;int r; int g; int b;};
colortype colors[max_color+1];
colors[0].name="black";colors[0].r=0;colors[0].g=0;colors[0].b=0;
colors[1].name="white";colors[1].r=255;colors[1].g=255;colors[1].b=255;
...
pixmap.fill(QColor(colors[cont].r,colors[cont].g,colors[cont].b));

struct colortype {QString name;int r; int g; int b;};
colortype colors[max_color+1];
colors[0].name="black";colors[0].r=0;colors[0].g=0;colors[0].b=0;
colors[1].name="white";colors[1].r=255;colors[1].g=255;colors[1].b=255;
...
pixmap.fill(QColor(colors[cont].r,colors[cont].g,colors[cont].b));

To copy to clipboard, switch view to plain text mode 

but i need to use this in several classes. So I think to programm my own class for colors (with only the colors i need and in rgb mode).

[zorro68]

Can anybody help me???

[wysota]

Hmm... why don't you use RGB values instead of names then? Take a look at my color combobox (btw. there is no point in reinventing the wheel, you can use QwwColorComboBox in your project provided you won't violate the licence).

[zorro68]

I'm newbie in Qt and I don't know about your project (im looking it and its a good project). But I have two reason for programming my own class:

First, I use the same structure for defining chemical elements, and other properties, that's why a need this structure go fine.
Second, im learning with qt and i know that the most you programm, most you learn.

As you can see at first of this topic, I'm using RGB color but when I call the class don't work correctly (structure variables take bad values).

Thanks

[wysota]

The structure is weird. It is much tainted with C like code. You should consider something more object oriented. But still you are reinventing the wheel as QColor already does what you want. You could go for a simple QVector<QColor>.

[zorro68]

I don't use QColor class because works bad. When I use:

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....  <-only some colors, not all
 ...
pixmap.fill(QColor(name)); <- this inside a for loop, this is the cause of the QStringList

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....  <-only some colors, not all
 ...
pixmap.fill(QColor(name)); <- this inside a for loop, this is the cause of the QStringList

To copy to clipboard, switch view to plain text mode 

the green color is not (0,255,0), is a darkgreen, and darkyellow is black for me.

And if I have the number of one color in another class (0=black, 1=White, ..., not RGB) how can i know its RGB with QColor? (I only knows its ordinal number that is the same as it position in the combo, not its name and not its R, G or B)

[wysota]

So why don't you omit those colors which you don't like or not use names at all? You can create a QColor by passing R, G and B components to QColor constructor.

If you really wish to have a separate structure holding the components (which is completely unnecessary as that's exactly what QColor does), why don't you do it like this?

struct Color{
int red;
int green;
int blue;
QString name;
};
QVector<Color> colors;
foreach(Color col, colors){
  QColor c(col.red, col.green, col.blue);
  QPixmap px(16,16);
  px.fill(c);
  comboBox->addItem(QIcon(px), col.name);
}

struct Color{
int red;
int green;
int blue;
QString name;
};
QVector<Color> colors;
foreach(Color col, colors){
  QColor c(col.red, col.green, col.blue);
  QPixmap px(16,16);
  px.fill(c);
  comboBox->addItem(QIcon(px), col.name);
}

To copy to clipboard, switch view to plain text mode 

[zorro68]

I think you don't understand me (excuse me for my bad english), so i'm trying to explain better:

If i used color names like this:

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....  <-only some colors, not all
 ...
foreach (QString name, colorName){
	pixmap.fill(QColor(name));
	CMBcolor[i]->addItem(QIcon(pixmap),"", QColor(name));
}

QStringList colorNames;
colorNames <<"black"<<"white"<<"darkGray"....  <-only some colors, not all
 ...
foreach (QString name, colorName){
	pixmap.fill(QColor(name));
	CMBcolor[i]->addItem(QIcon(pixmap),"", QColor(name));
}

To copy to clipboard, switch view to plain text mode 

the green color don't display (0,255,0), is a darkgreen, and darkyellow is black for me.
I don't know if this is a bug or not but the combo displays bad colors for these two (green and darkyellow, the other i use are well but probably there will be other colors that fails)

So why don't you omit those colors which you don't like ...

I programm the QStringList to avoid those colors i don't like (I only want 16 colors - white, black, red, darkRed, green, darkGreen, blue, darkBlue, cyan, darkCyan, magenta, darkMagenta, yellow, darkYellow, gray, darkGray, lightGray)

... or not use names at all?

I can avoid names of colors (I use them to complete the structure). But this is not the explanation why don't work my structure.

struct ColorData
{ 
        int num;
        QString name;
        int R;
        int G;
        int B;
}Propiedades[max_colors],*p;
 
...
 
ColorData Propiedades[max_colors] =
{
        { 0, "black",  0,  0,  0},
        { 1, "white",255,255,255},
        { 2, "darkGray",128,128,128},
        { 3, "gray",160,160,164},
        ...
    };
    p=&Propiedades[0];

struct ColorData
{ 
        int num;
        QString name;
        int R;
        int G;
        int B;
}Propiedades[max_colors],*p;

...

ColorData Propiedades[max_colors] =
{
        { 0, "black",  0,  0,  0},
        { 1, "white",255,255,255},
        { 2, "darkGray",128,128,128},
        { 3, "gray",160,160,164},
        ...
    };
    p=&Propiedades[0];

To copy to clipboard, switch view to plain text mode 

You can create a QColor by passing R, G and B components to QColor constructor.

Yes, i can. But i have to create the new colors each time i have to use them. So this is the reason to create a class with my colors.

If you really wish to have a separate structure holding the components (which is completely unnecessary as that's exactly what QColor does), why don't you do it like this?

Because if i pick the combo i need to know the r g b components of the color i have picked, and with this code i have to redefined my colors again (in other class). And I don't know how to obtain the rgb components of the color when i click in the combo (the selected item of the combo).

So if my class works I wouldn't be any problem. In the class i have clasified the colors for a single number what is the same that the position that the colors is in the combo. For example, if the combo return 1, i know that is the color white and i know elsewhere of the program the r,g,b components of this color.

connect(CMBcolor,SIGNAL(currentIndexChanged(int)),this,SLOT(CMBcolor_change(int)));

connect(CMBcolor,SIGNAL(currentIndexChanged(int)),this,SLOT(CMBcolor_change(int)));

To copy to clipboard, switch view to plain text mode 

The current index is known in CMBcolor_change by the int variable. And with this integer i obtain the rgb components. With QColor i don't know how to do this because QColor use the name or the r,g,b but not the index.

As you can see i don't use the name of the color, as i say this is to complete the structure.

By all this i have decided to bulid the class.

Excuse me for my english and i hope you understand me.

[Rembobo]

Hello, I think

colors::colors()
{
ColorData Propiedades[max_colors] =// this is local (constructor) variable, not class instance Propiedades.
{
{ 0, "black", 0, 0, 0},
{ 1, "white",255,255,255},
{ 2, "darkGray",128,128,128},
{ 3, "gray",160,160,164},
...
};
p=&Propiedades[0]; // p now points to local (ctor) variable which will be diallocated
//right now.
}

int colors::getRed(int num)
{
ColorData *q;
q=p; // p points to deallocated memory.
for (int i=0;i<=max_colors;i++){// bad, must be "i < max_colors" or even "i < colors::max_colors".
if (num==q->num) return q->R;
q++;
}
return 0;
}

[zorro68]

Yes, I think that I am pointing to a deallocated variable, because p takes extrange values in the debugger. But how to do it well? How to convert this local (constructor) variable to a class instance? (Think that Im newbie in Qt and C++. I programm in Fortran, visual basic, c...)

Thanks

[wysota]

Yes, i can. But i have to create the new colors each time i have to use them.

No, if you store them as QColor objects in the first place.

Because if i pick the combo i need to know the r g b components of the color i have picked, and with this code i have to redefined my colors again (in other class). And I don't know how to obtain the rgb components of the color when i click in the combo (the selected item of the combo).

I really don't see a problem.

QMap<QString, QColor> colors; // or simply QList<QColor> if you don't want the names
colors["white"] = QColor(255,255,255);
colors["red"] = QColor(255,0,0);
//...
 
for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
  comboBox->addItem(...);
}

QMap<QString, QColor> colors; // or simply QList<QColor> if you don't want the names
colors["white"] = QColor(255,255,255);
colors["red"] = QColor(255,0,0);
//...

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
  comboBox->addItem(...);
}

To copy to clipboard, switch view to plain text mode 

and then when you want to identify the color:

int index = comboBox->currentIndex();
QColor col = colors[colors.keys().at(index)];

int index = comboBox->currentIndex();
QColor col = colors[colors.keys().at(index)];

To copy to clipboard, switch view to plain text mode 

Or simply use the ability of combobox to store custom data with each item:

QList<QColor> colors;
//...
foreach(QColor c, colors){
  comboBox->addItem(...);
  int index = comboBox->count()-1;
  comboBox->setItemData(index, c);
}

QList<QColor> colors;
//...
foreach(QColor c, colors){
  comboBox->addItem(...);
  int index = comboBox->count()-1;
  comboBox->setItemData(index, c);
}

To copy to clipboard, switch view to plain text mode 

and then use QComboBox::itemData to fetch the color directly from the combobox.

So if my class works I wouldn't be any problem.

Sure. You're just wasting time fixing something that you completely don't need, because the combobox already provides everything you might want. Oh, and about doubling data - you're doubling it with your structure as well. First you hold it in your structure and then QComboBox is creating its own items. You could of course avoid that by using a simple model (like I do with my color combobox class), but in your case I'd simply ignore the redundancy - there is no point in making things too complex.

[zorro68]

Ok wysota, sorry for my ignorance. I'm going to program like you say. I'm proving with the first sugerence and i have a problem:

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->second);
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->second);
}

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->second);
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->second);
}

To copy to clipboard, switch view to plain text mode 

this give me an error for it->second (second not a member of QColor). I have read that it->first is the QString and it->second the QColor, but don't work. Why?

[wysota]

I'm going to program like you say.

I don't say you should do as I said. I only said you were reinventing the wheel and wasting your precious time for things you don't really need. But the final choice is yours.

I'm proving with the first sugerence and i have a problem:

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->second);
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->second);
}

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->second);
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->second);
}

To copy to clipboard, switch view to plain text mode 

this give me an error for it->second (second not a member of QColor). I have read that it->first is the QString and it->second the QColor, but don't work. Why?

You should use it.key() and it.value() instead. Operator * of the iterator (which you are using behind the scenes by calling "->") returns the value of the map item (look at QMap::const_iterator docs) - in this case QColor.

[zorro68]

You have convinced me because i don't like to waste computer memory...

I have prove with it->value()

QMap<QString, QColor> colors; 
colors["black"] = QColor(0,0,0);	
colors["white"] = QColor(255,255,255);
colors["red"] = QColor(255,0,0);
colors["darkGray"]=QColor(128,128,128);
...
 
for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->value());
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->value());
}

QMap<QString, QColor> colors; 
colors["black"] = QColor(0,0,0);	
colors["white"] = QColor(255,255,255);
colors["red"] = QColor(255,0,0);
colors["darkGray"]=QColor(128,128,128);
...

for(QMap<QString,QColor>::const_iterator it = colors.begin(); it!=colors.end();++it){
	pixmap.fill(it->value());
	CMBcolor[i]->addItem(QIcon(pixmap),"",it->value());
}

To copy to clipboard, switch view to plain text mode 

But all colors are diferents blue and black. Im using Qt 4.3.1

[wysota]

What is CMBcolor? An array of combobox pointers? How did you create "pixmap"?

The following works well for me:

#include <QComboBox>
#include <QApplication>
#include <QList>
#include <QColor>
#include <QPixmap>
 
int main(int argc, char **argv){
  QApplication app(argc, argv);
  QComboBox box;
  QList<QColor> colors;
  colors << QColor(255, 255, 255) << QColor(255, 0, 0) << QColor(128, 128, 128) << QColor(0,0,0);
  QPixmap px(16,16);
  foreach(QColor color, colors){
    px.fill(color);
    box.addItem(px, QString::null, color);
  }
  box.show();
  return app.exec();
}

#include <QComboBox>
#include <QApplication>
#include <QList>
#include <QColor>
#include <QPixmap>

int main(int argc, char **argv){
  QApplication app(argc, argv);
  QComboBox box;
  QList<QColor> colors;
  colors << QColor(255, 255, 255) << QColor(255, 0, 0) << QColor(128, 128, 128) << QColor(0,0,0);
  QPixmap px(16,16);
  foreach(QColor color, colors){
    px.fill(color);
    box.addItem(px, QString::null, color);
  }
  box.show();
  return app.exec();
}

To copy to clipboard, switch view to plain text mode 

[zorro68]

What is CMBcolor? An array of combobox pointers?

Yes

How did you create "pixmap"?

int size = CMBcolor[i]->style()->pixelMetric(QStyle::PM_SmallIconSize);
QPixmap pixmap(size, size);

int size = CMBcolor[i]->style()->pixelMetric(QStyle::PM_SmallIconSize);
QPixmap pixmap(size, size);

To copy to clipboard, switch view to plain text mode 

As you know, this is the same as QPixmap pixmap(16,16) <- I have proved with this too

Now i'm going to prove this.

[zorro68]

Yes, this works fine if there is a simple function. But now I'm going to use the "colors" variable in another function and i have an error because this function cannot "see" this variable.
So I put in my .h file the definition of the "colors" variable like this: QList<QColor *> colors; (¿is this ok?)
and I'm going to initialize theirs values in .cpp file with this
colors << QColor(0,0,0) << QColor(255,255,255) << ...
but this not work because the << operator don't work with pointers (I think).
How can i fill the values of colors?

[wysota]

Yes, this works fine if there is a simple function. But now I'm going to use the "colors" variable in another function and i have an error because this function cannot "see" this variable.

So make it global to the parent scope (for example by making it a member variable) or global to the application or pass it as an argument to where you want to use it.

So I put in my .h file the definition of the "colors" variable like this: QList<QColor *> colors; (¿is this ok?)

No. Should be "extern QList<QColor> colors;" (and forget the pointer, there is no point using it and you're wasting additionally 4/8 bytes per item). Then create the variable in one of your .cpp files without the extern keyword.

and I'm going to initialize theirs values in .cpp file with this
colors << QColor(0,0,0) << QColor(255,255,255) << ...
but this not work because the << operator don't work with pointers (I think).
How can i fill the values of colors?

Don't use pointers