Problem with a color class

[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