[PATCH] Fixed layout of yearly statistics model

[Dirk Hohndel]

On Thu, 2014-03-06 at 18:59 +0530, Heena Mahour wrote:
> 
> From aacfca96716bbb82f6bb9b14e32e590c0c11ec7c Mon Sep 17 00:00:00 2001
> From: heena mahour <heena393 at gmail.com>
> Date: Thu, 6 Mar 2014 18:52:36 +0530
> Subject: [PATCH 4/4] Signed-off-by: heena mahour <heena393 at gmail.com>
> 
> 
> Yearly Statistics model
> #This is my first patch 
> Changes in the layout of the tree view ,set the pixel size, stretch,
> bold of the font ,fixed the layout

So your commit message is pretty messed up. You have your SOB as the
commit title, your commit message body isn't really a sentence and worse
it doesn't quite match the patch...

> diff --git a/qt-ui/models.cpp b/qt-ui/models.cpp
> index a149488..ab0f3c4 100644
> --- a/qt-ui/models.cpp
> +++ b/qt-ui/models.cpp
> @@ -1224,19 +1224,19 @@ QVariant DiveTripModel::headerData(int
> section, Qt::Orientation orientation, int
> - case DATE: ret = tr("date"); break;
> + case DATE: ret = tr("Date"); break;
> - case SUIT: ret = tr("suit"); break;
> + case SUIT: ret = tr("Suit"); break;
> - case LOCATION: ret = tr("location"); break;
> + case LOCATION: ret = tr("Location"); break;

So this changes the case of three of the terms and the logic behind that
is not entirely clear to me - and it's not explained in the commit
message. Why should these three words be upper case?

> @@ -1418,8 +1418,23 @@ void DiveComputerModel::keepWorkingList()
>  
>  class YearStatisticsItem : public TreeItem {
>  public:
> - enum {YEAR, DIVES, TOTAL_TIME, AVERAGE_TIME, SHORTEST_TIME,
> LONGEST_TIME, AVG_DEPTH, MIN_DEPTH,
> - MAX_DEPTH, AVG_SAC, MIN_SAC, MAX_SAC, AVG_TEMP, MIN_TEMP, MAX_TEMP,
> COLUMNS};
> + enum {
> +  YEAR, 
> +  DIVES,
> +  TOTAL_TIME, 
> +  AVERAGE_TIME, 
> +  SHORTEST_TIME,
> +  LONGEST_TIME, 
> +  AVG_DEPTH,
> +  MIN_DEPTH,
> +  MAX_DEPTH,
> +  AVG_SAC,
> +  MIN_SAC, 
> +  MAX_SAC,
> +  AVG_TEMP, 
> +  MIN_TEMP, 
> +  MAX_TEMP, 
> +  COLUMNS};

That's a random whitespace change in the middle of your patch.
Please try not to mix pure whitespace changes with functional changes.
 
> @@ -1438,6 +1453,8 @@ QVariant YearStatisticsItem::data(int column,
> int role) const
>  
>   if (role == Qt::FontRole) {
>   QFont font = defaultModelFont();
> + font.setStretch(130);
> + font.setPixelSize(15);
>   font.setBold(stats_interval.is_year);
>   return font;
>   } else if (role != Qt::DisplayRole) {

This does the stretch / size change that you mention (why change the
stretch? you should explain that in the commit message) but doesn't
appear to change the layout or boldness, both of which are mentioned in
the commit message.

> @@ -1588,11 +1605,9 @@ void TablePrintModel::callReset()
>  
>  QVariant TablePrintModel::data(const QModelIndex &index, int role)
> const
>  {
> - if (!index.isValid())
> - return QVariant();
> - if (role == Qt::BackgroundRole)
> + if ((role == Qt::BackgroundRole)&&(index.isValid()))
>   return QColor(list.at(index.row())->colorBackground);
> - if (role == Qt::DisplayRole)
> + if ((role == Qt::DisplayRole)&&(index.isValid()))
>   switch (index.column()) {
>   case 0: return list.at(index.row())->number;
>   case 1: return list.at(index.row())->date;

Why would you do this? You keep testing for index.isValid() instead of
testing for it in the beginning as the existing code did. Why? What's
the advantage? Which error does this fix?

Please redo / resubmit.

Thanks

/D