Phil's code review of COE ABQ 5 BOR development (March 2017, RW 7.1)
Branch: ABQCoe5Bor3 -- Edit: 3-29-2017.
Robynn, you did a really great job with this work. It's clear that you understood the complexity inherent in the relevant code. There's a lot. Most of my comments are just guidance. Very few actual problems were found.
You have this code:
QFontDatabase fontDb;
QList<int>::iterator i;
QList<int> fontSizes = fontDb.pointSizes(
_ui->_chartWidgetFont->currentFont().family());
for(i=fontSizes.begin(); i != fontSizes.end(); i++)
{
int sizeInt = *i;
char sizeChar[2] ;
itoa(*i, sizeChar, 10);
_ui->_chartWidgetFontSize->addItem(sizeChar);
} |
I observed fontSizes values up to 72 (in the debugger) -- two decimal digits, though it's not inconceivable that some context would provide three digit values. Even so, the buffer size for the "itoa" call is just two. This needs to include at least an extra byte for the null termination. (That is, "72" requires three bytes).
I would always provide enough space to handle any integer that could possibly be presented, and then some. (And it might as well be a multiple of eight, being that we are in a 64-bit architecture). I'd give it this, to be extra conservative: char sizeChar [32].
Still though, I prefer to use higher level tools. (We are, after all, headed for a QString representation of the number). Assuming we stick with STL iteration (see my less important note about that), I'd prefer to code this as follows, not using 'itoa' ...
QFontDatabase fontDb;
QList<int>::iterator i;
QList<int> fontSizes = fontDb.pointSizes(
_ui->_chartWidgetFont->currentFont().family());
for(i=fontSizes.begin(); i != fontSizes.end(); i++)
{
const int sizeInt = *i;
const QString sizeStr = QString ("%1") .arg (sizeInt);
_ui->_chartWidgetFontSize->addItem(sizeStr);
} |
I might have preferred that the original OcanRectAnchorPntItem implementation be expanded for this use. While that was sort of the original intention, I do recognized that many methods do call methods on its reference _chart pointer (the original application being for charts) -- so the decision to bufurcate this class, adding one specifically for Images -- is understandable.
I would like for the new module to have these things:
//----------------------------------------------------------------------------- |
... It's OK to manually edit what comes between "$Id:" and the final "&" by hand.
I personally find the timestamping of files (which used to be automatic in CVS, but isn't a feature of GIT) invaluable, e.g. when looking at diffs. I'm the only staff member doing this; so clearly, we haven't decided that this is necessary. The consultant who set us up with GIT (back in 2012) provided a script to perform this timestamping on "staged" files. This script is described here:
The cases in [Sim, ChartDevice.cpp] function chartDeviceCmd can't assume that there are at least four (4) arguments in that argv-list. Instead of an explicit check (another thing to test), in this function, we just fill in with an empty string if subsequent parameters are missing. After having checked that there are at least three arguments, we do this at the head of that method:
const char* argv1 = argv[1]; const char* argv2 = argv[2]; const char* argv3 = (argc >= 4) ? argv[3] : ""; const char* argv4 = (argc >= 5) ? argv[4] : ""; const char* argv5 = (argc >= 6) ? argv[5] : ""; const char* argv6 = (argc >= 7) ? argv[6] : ""; |
So, it is important to use "argc3" instead of "argc[3]" in these new cases:
if(strcmp(argv2, "chartWidgetFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setChartWidgetFont(font);
}
return TCL_OK;
}
if(strcmp(argv2, "dateFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setDateFont(font);
}
return TCL_OK;
}
// To support intermediate model for font.
if(strcmp(argv2, "slotFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setLegendFont(font);
}
return TCL_OK;
}
if(strcmp(argv2, "legendFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setLegendFont(font);
}
return TCL_OK;
}
if(strcmp(argv2, "notesFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setNotesFont(font);
}
return TCL_OK;
}
if(strcmp(argv2, "subtitleFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setSubtitleFont(font);
}
return TCL_OK;
}
if(strcmp(argv2, "titleFont") == 0)
{
QFont font;
if (font.fromString(argv[3]))
{
cd->setTitleFont(font);
}
return TCL_OK;
} |
An rwSetting (an rwIntSetting) data member was added to the OutputCanvasConfig::Image class. It does correctly "go with" the previously defined rwSetting members in the header file, but not in the initializer. Also, it would be good to conform to the commenting convention used (missing from the initializer list), and not introduce anomalies in deliberately lined-up source code columns (as in the declaration).
// Setting fields rwTextSetting* _name; // (1) OCAN_IMG_NAME rwBoolSetting* _isBackground; // (2) OCAN_IMG_IS_BG rwIntSetting* _opacityPercent; // (3) OCAN_IMG_OPACITY rwIntSetting* _posX; // (4) OCAN_IMG_POSX rwIntSetting* _posY; // (5) OCAN_IMG_POSY rwIntSetting* _srcWidth; // (6) OCAN_IMG_SRC_WIDTH rwIntSetting* _srcHeight; // (7) OCAN_IMG_SRC_HEIGHT rwTextSetting* _srcPath; // (8) OCAN_IMG_SRC_PATH rwIntSetting* _imageScale; // (9) OCAN_IMG_SCALE // Other data members QByteArray _binaryImageData; |
// constructor 1 of 2
OutputCanvasConfig::Image::Image (ImageGroup* parGroup)
: OutputCanvasNode(), // contains _settingList
_parentGroup (parGroup), // parent ImageGroup*
_name (NULL), // rwTextSetting* -- (1) OCAN_IMG_NAME
_isBackground (NULL), // rwBoolSetting* -- (2) OCAN_IMG_IS_BG
_opacityPercent (NULL), // rwIntSetting* -- (3) OCAN_IMG_OPACITY
_posX (NULL), // rwIntSetting* -- (4) OCAN_IMG_POSX
_posY (NULL), // rwIntSetting* -- (5) OCAN_IMG_POSY
_srcWidth (NULL), // rwIntSetting* -- (6) OCAN_IMG_SRC_WIDTH
_srcHeight (NULL), // rwIntSetting* -- (7) OCAN_IMG_SRC_HEIGHT
_srcPath (NULL), // rwTextSetting* -- (8) OCAN_IMG_SRC_PATH
_binaryImageData(), // QByteArray, image data.
_imageScale(NULL) |
There are a couple comments about the name of that new field (_imageScale), below.
[Robynn, I have to say, you very quickly understanding how to wipe out the printed background color, outside of the Output Canvas' graphics scene, just this morning, was impressive]. That code in OutputCanvasDlgImp::paintPrinter() sets the output canvas to white at the beginning, and then restores it to
This method defined in the header file was never implemented -- not needed, I guess:
I cited this code snippet above (for another observation) ... the new rwIntSetting data member added to the OutputCanvasConfig::Image class.
// Setting fields rwTextSetting* _name; // (1) OCAN_IMG_NAME rwBoolSetting* _isBackground; // (2) OCAN_IMG_IS_BG rwIntSetting* _opacityPercent; // (3) OCAN_IMG_OPACITY rwIntSetting* _posX; // (4) OCAN_IMG_POSX rwIntSetting* _posY; // (5) OCAN_IMG_POSY rwIntSetting* _srcWidth; // (6) OCAN_IMG_SRC_WIDTH rwIntSetting* _srcHeight; // (7) OCAN_IMG_SRC_HEIGHT rwTextSetting* _srcPath; // (8) OCAN_IMG_SRC_PATH rwIntSetting* _imageScale; // (9) OCAN_IMG_SCALE // Other data members QByteArray _binaryImageData; |
... percents are unusual numeric values -- more naturally represented as fractions. So when they ARE percentage values, that's important enough to indicate in the name of the field. That is, either:
BUT ALSO, all the data members in this class (OutputCanvasConfig::Image) are properties of an "Image" thing. There's no particular reason why this field needs to evoke the "Image" idea more than the others. I would have preferred that this -- AND the analogous "_teacupScale" field in the OutputCanvasConfig::Teacup class -- be named in one of these ways:
The case of the this scale-percent field in the OutputCanvasConfig::Teacup is a tad stranger, since it is used in the SimObj Icon use of this class, not the "Teacup" (as such) use.
We have these warnings:
Here is the definition of the CupType typdef enum:
typedef enum {
CupType_Teacup1 = 0, // Standard Object Teacup
CupType_ObjIcon, // Object Icon (only)
CupType_Count // (number of cup types)
} CupType; |
It seems strange, but "CupType" doesn't actually define a scope. (I guess). It can be scoped with the containing class name ("OutputCanvasConfig"), but since we are in that class, we don't have to. That is, these warnings can be addressed by just removing "CupType::" from those lines.
We typically conditionally show rwSettings with the use of these rwSetting methods -- that is, being dependent on a sibling having a particular string value, or a value within a set of designated string values.
Here is an example -- see the last line of each stanza. Note that the second one depends on the first:
_isLegend = new rwBoolSetting (rwSetting::TCUP_IS_LEGEND);
_isLegend->setTrueString (tr ("Yes"));
_isLegend->setFalseString (tr ("No"));
_isLegend->setDefaultValue (false);
_isLegend->setDependentOnSibling (
rwSetting::TCUP_IS_LEGEND, "NotFound-AlwaysHidden");
_obj1Name = new rwTextSetting (rwSetting::TCUP_REF_OBJ1);
_obj1Name->setValueType (rwSetting::SIMOBJ_NAME);
_obj1Name->setDependentOnSibling (
rwSetting::TCUP_IS_LEGEND, _isLegend->falseString()); |
What we now have in void OutputCanvasConfig::Teacup::buildSettings() does this differently for a new setting ...
_settingList << _cupType
<< _isLegend
<< _label
<< _obj1Name
<< _obj2Name
<< _posX
<< _posY;
if(cupType() == CupType::CupType_ObjIcon)
{
_settingList << _teacupScale;
} |
That works, and is probably OK. But there is a little confusion about that last thing also being implemented here (below). (It would be better to keep it in just the code above, if that makes sense).
void OutputCanvasConfig::Teacup::buildSettingsForScale()
{
if( _cupType->value() == cupTypeStr(CupType::CupType_ObjIcon))
{
_settingList << _teacupScale;
}
} |
I don't intend for this to be comprehensive, by any means. There's a lot of great code in there! But I would like to acknowledge some optional things in your code which I like to see.
(1) I really like comments like what you have here. (It's true that the meaning of the parameter _could_ change, but in that case, we need to carefully visit each use anyway. I'm willing to take this risk):
[Robynn, gosh, sorry. There is a lot of great code. I didn't end up mentioning more about that. And it really is clear that you understood many complex things here. It's really good].
It IS ok to use STL-style iterators (we are all OK with that), but, for more tractable debugging, I much prefer iteration with an integer. Often, deep within an iteration, in the debugger, looking at the stack, I want to know how far along I am in processing a list.
Here is an example. You have:
QString fontSizeString;
_ui->_chartWidgetFont->setCurrentFont(_device->getChartWidgetFont());
_ui->_chartWidgetFontSize->setCurrentText(
fontSizeString.setNum(_device->getChartWidgetFont().pointSize()));
_ui->_dateFont->setCurrentFont(_device->getDateFont());
_ui->_dateFontSize->setCurrentText(
fontSizeString.setNum(_device->getDateFont().pointSize()));
_ui->_legendFont->setCurrentFont(_device->getLegendFont());
_ui->_legendFontSize->setCurrentText(
fontSizeString.setNum(_device->getLegendFont().pointSize()));
_ui->_notesFont->setCurrentFont(_device->getNotesFont());
_ui->_notesFontSize->setCurrentText(
fontSizeString.setNum(_device->getNotesFont().pointSize()));
_ui->_subtitleFont->setCurrentFont(_device->getSubtitleFont());
_ui->_subtitleFontSize->setCurrentText(
fontSizeString.setNum(_device->getSubtitleFont().pointSize()));
_ui->_titleFont->setCurrentFont(_device->getTitleFont());
_ui->_titleFontSize->setCurrentText(
fontSizeString.setNum(_device->getTitleFont().pointSize())); |
I would code something like that, like this (below). For one thing, it looks like four different things happening to six things, rather than a huge glob of who knows what, unless you study it with great care. But also, much easier for debugging. The intermediate values in temporaries helps with inspection -- and "documents" the types of these values.
const QFont chartFont = _device->getChartWidgetFont(); const QFont dateFont = _device->getDateFont(); const QFont legendFont = _device->getLegendFont(); const QFont notesFont = _device->getNotesFont(); const QFont subTitFont = _device->getSubtitleFont(); const QFont titleFont = _device->getTitleFont(); _ui->_chartWidgetFont -> setCurrentFont (chartFont); _ui->_dateFont -> setCurrentFont (dateFont); _ui->_legendFont -> setCurrentFont (legendFont); _ui->_notesFont -> setCurrentFont (notesFont); _ui->_subtitleFont -> setCurrentFont (subTitFont); _ui->_titleFont -> setCurrentFont (titleFont); const int ptSizeChart = chartFont.pointSize(); const int ptSizeDate = dateFont.pointSize(); const int ptSizeLegend = legendFont.pointSize(); const int ptSizeNotes = notesFont.pointSize(); const int ptSizeSubTit = subTitFont.pointSize(); const int ptSizeTitle = titleFont.pointSize(); QString szStr; _ui->_chartWidgetFontSize -> setCurrentText (szStr.setNum (ptSizeChart)); _ui->_dateFontSize -> setCurrentText (szStr.setNum (ptSizeDate)); _ui->_legendFontSize -> setCurrentText (szStr.setNum (ptSizeLegend)); _ui->_notesFontSize -> setCurrentText (szStr.setNum (ptSizeNotes)); _ui->_subtitleFontSize -> setCurrentText (szStr.setNum (ptSizeSubTit)); _ui->_titleFontSize -> setCurrentText (szStr.setNum (ptSizeTitle)); |
Note that, like most Qt classes, QFont can be passed around (by value) very efficiently. These have "reference counted" / "copy on write" implementations. (But for method input parameters, I do still use const references).
I wouldn't indent BOTH the curly and the block content. That is you have this:
case UserType_OcanImageGfxItem:
{
OcanImageGfxItem* imageItem = dynamic_cast<OcanImageGfxItem*> (item);
if (imageItem)
imageItem->updateImage();
image = imageItem ? imageItem->image() : NULL;
break;
} |
Especially since you are using four (4) space indentations, this is almost THREE levels of indentation space (had you used just 3). It's a lot. I would prefer this. (Actually the other cases in the same "switch" statement are, in fact, presented this way, and with indents of 3 spaces) ...
case UserType_OcanImageGfxItem:
{
OcanImageGfxItem* imageItem = dynamic_cast<OcanImageGfxItem*> (item);
if (imageItem)
imageItem->updateImage();
image = imageItem ? imageItem->image() : NULL;
break;
} |
The Object Icon range seems ok (though I think we could go a bit larger). But I would adjust the Image range. For a subtle background (50% or less opacity), it will be reasonable to use a small image, and substantially increase its size on the canvas. (Data for large images is stored in the model file -- this can be very significant). Also, less then 10% seems silly (especially inefficient in terms of stored image space). I'd recommend these ranges:
--- (end) ---
