Merge lp:~brendan-donegan/checkbox/bug1030871_component_status_table into lp:checkbox

[Brendan Donegan]

This merge request combines the seperate Component and Status tree view widgets into one widget with a Component column and a Status column.

[Brendan Donegan (brendan-donegan)]

It's probably worth giving this a good vigorous test, including making a submission. My own tests check out but a second opinion would be valuable, as there is a lot of code that depends on the tree views and it's difficult to be sure it was all re-factored properly.

[Sylvain Pineau (sylvain-pineau)]

I would suppress the Component and status headers and the line separator just below as well. Without two real columns I don't see the need to have them now and it will offer a bigger area to display the test selection.

Talking about the test selection, it's a suggestion not a must have but graying all tests titles once we start is not really user friendly. I would instead only gray out the checkboxes only.

Disabling the select all/deselected all buttons could be also a good idea once tests are started.

[Brendan Donegan (brendan-donegan)]

1.) I don't agree. A key is always useful for the user and I doubt the extra half a centimeter will be worthwhile

2.) I doubt this is technically possible. Besides it's outside the scope of the enhancement so unless it was straightforward (which is isn't) I don't think we should expend too much effort making it happen.

3.) This can be done, but perhaps should be in a separate merge (since it's outside the scope of this bug). Maybe you can raise a separate issue?

[Sylvain Pineau (sylvain-pineau)]

1.) It was my feeling and a wish to have nothing but a minimal ui. But your opinion about usability is valid too. So let keep the ui as it is today.

2.) Ok, I will keep that as a nice to have request for the next cycle :-)

3.) Sure.

I will update my review once I get a valid submission.

[Brendan Donegan (brendan-donegan)]

Number 2 you can probably add to the Qt UI improvements spreadsheet. That might be the best place to track it

[Daniel Manrique (roadmr)]

OK here's the third opinion:

1) I vote for leaving the headers, it's perfectly clear to us but it will confuse newcomers if we remove them. I'd remove the separator though; the column headers belong with the table and the separator, well, separates them creating a somewhat jarring visual break that feels out of place.

2) Graying out only the checkboxes would be better but if it's technically not so easy, I think what we have is good enough.

3) Graying out the buttons should not be *that* hard to do, but I agree that it would be best handled as a separate improvement.

[lp:~brendan-donegan/checkbox/bug1030871_component_status_table]

1546. By Brendan Donegan on 2012-08-02

Remove line seperating column headers from tree view.

[Brendan Donegan (brendan-donegan)]

1.) Done!

review: Needs Resubmitting

[Marc Tardif (cr3)]

1) Thanks, this abides well by the design principle expressed so eloquently by Steve Krug: Keep the noise down to a dull roar.

[lp:~brendan-donegan/checkbox/bug1030871_component_status_table]

1547. By Brendan Donegan on 2012-08-02

Put some padding to the right of the tree view and add super zebra stripe powers.

[Sylvain Pineau (sylvain-pineau)]

The padding on the right side of the selection window is larger than the left one. It's easier to see when the tests are loading because the scrollbar is not present.

I suggest to allow the widget to take the full width and also to make the scrollbar visible even if there is nothing to scroll.

PS. Zebra rocks !

review: Needs Fixing

[lp:~brendan-donegan/checkbox/bug1030871_component_status_table]

1548. By Brendan Donegan on 2012-08-02

Fix padding of tree view and keep the scroll bar always on.

[Brendan Donegan (brendan-donegan)]

Done. Now I can haz merge plz? :)

review: Needs Resubmitting

[lp:~brendan-donegan/checkbox/bug1030871_component_status_table]

1549. By Brendan Donegan on 2012-08-03

Merge from trunk

[Sylvain Pineau (sylvain-pineau)]

Thanks for the fixes, it looks beautiful now.
Approved !

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2012-08-02 20:09:52 +0000
+++ debian/changelog	2012-08-03 08:19:21 +0000
@@ -10,6 +10,8 @@
     /etc/default/apport (LP: #1029897)
   * Initially disable the 'Run' tab in the Qt UI, re-enabling it when the
     'Start Testing' has been clicked (LP: #1029815)
+  * Put Component and Status into one tree view on the selection screen,
+    rather than two seperate ones (LP: #1030871)
 
   [Daniel Manrique]
   * New version 0.14.3 for Quantal Quetzal development.

=== modified file 'qt/frontend/qtfront.cpp'
--- qt/frontend/qtfront.cpp	2012-07-30 09:09:23 +0000
+++ qt/frontend/qtfront.cpp	2012-08-03 08:19:21 +0000
@@ -71,7 +71,6 @@
     connect(ui->checkBox, SIGNAL(toggled(bool)), SIGNAL(welcomeCheckboxToggled(bool)));
     connect(ui->treeView, SIGNAL(collapsed(QModelIndex)), this, SLOT(onJobItemChanged(QModelIndex)));
     connect(ui->treeView, SIGNAL(expanded(QModelIndex)), this, SLOT(onJobItemChanged(QModelIndex)));
-    connect(ui->treeView->verticalScrollBar(), SIGNAL(valueChanged(int)), ui->statusView->verticalScrollBar(), SLOT(setValue(int)));
     connect(ui->treeView, SIGNAL(clicked(QModelIndex)), this,  SLOT(onTestSelectionChanged()));
     connect(this, SIGNAL(testSelectionChanged()), this,  SLOT(onTestSelectionChanged()));
     ui->stepsFrame->setFixedHeight(0);
@@ -382,43 +381,36 @@
     }
 }
 
-void QtFront::updateTestStatus(QStandardItem *item, QString status) {
+void QtFront::updateTestStatus(QStandardItem *item, QStandardItem *statusItem, QString status) {
     int numRows = item->rowCount();
     int done = 0;
     int inprogress = 0;
-    for(int i=0; i< numRows; i++) {
-        QStandardItem * childItem = item->child(i,0);
+    for(int i = 0; i < numRows; i++) {
+        QStandardItem * childItem = item->child(i,1);
         if (childItem->hasChildren()) {
-            updateTestStatus(childItem, status);
+            updateTestStatus(childItem, statusItem, status);
         }
         if (childItem->data(Qt::UserRole).toString() == m_currentTestName && !status.isEmpty()) {
             childItem->setText(m_statusStrings[status]);
         }
         if (childItem->text() == m_statusStrings[STATUS_PASS]) {
-            childItem->setData(QVariant(Qt::Checked), Qt::CheckStateRole);
             done++;
         } else if (childItem->text() == m_statusStrings[STATUS_INPROGRESS]) {
-            childItem->setData(QVariant(Qt::Unchecked), Qt::CheckStateRole);
             inprogress++;
-        } else if (childItem->text() == m_statusStrings[STATUS_UNINITIATED]) {
-            childItem->setData(QVariant(Qt::Unchecked), Qt::CheckStateRole);
         }
     }
     if (done == item->rowCount() && done != 0) {
-        item->setText(m_statusStrings[STATUS_PASS]);
-        item->setData(QVariant(Qt::Checked), Qt::CheckStateRole);
+        statusItem->setText(m_statusStrings[STATUS_PASS]);
     } else if (inprogress != 0 || done != 0) {
-        item->setText(m_statusStrings[STATUS_INPROGRESS]);
-        item->setData(QVariant(Qt::PartiallyChecked), Qt::CheckStateRole);
+        statusItem->setText(m_statusStrings[STATUS_INPROGRESS]);
     } else {
-        item->setText(m_statusStrings[STATUS_UNINITIATED]);
-        item->setData(QVariant(Qt::Unchecked), Qt::CheckStateRole);
+        statusItem->setText(m_statusStrings[STATUS_UNINITIATED]);
     }
 }
 
 void QtFront::updateTestStatus(QString status) {
-    for (int i=0; i < m_statusModel->rowCount(); i++) {
-        updateTestStatus(m_statusModel->item(i,0), status);
+    for (int i=0; i < m_model->rowCount(); i++) {
+        updateTestStatus(m_model->item(i,0), m_model->item(i,1), status);
     }
 }
 
@@ -432,11 +424,10 @@
 void QtFront::buildTree(QVariantMap options, 
                         QVariantMap defaults, 
                         QString baseIndex, 
-                        QStandardItem *parentItem, 
-                        QStandardItem *parentStatusItem) {
+                        QStandardItem *parentItem) {
     int internalIndex = 1;
     while (true) {
-        QString index = baseIndex+"."+QString::number(internalIndex);
+        QString index = baseIndex + "." + QString::number(internalIndex);
         QVariant value = options.value(index);
         QVariant defaultValue = defaults.value(index);
         QDBusArgument arg = value.value<QDBusArgument>();
@@ -463,19 +454,19 @@
             }
 
             testStatusItem->setData(test, Qt::UserRole);
-            testStatusItem->setFlags(Qt::ItemIsUserCheckable | Qt::ItemIsEnabled|Qt::ItemIsTristate);
-            testStatusItem->setData(QVariant(Qt::Unchecked), Qt::CheckStateRole);
 
             if (parentItem) {
                 // for children nodes
-                parentItem->appendRow(item);
-                parentStatusItem->appendRow(testStatusItem);
-                buildTree(options, defaults, index, item, testStatusItem);
+                QList<QStandardItem *> itemList;
+                itemList << item << testStatusItem;
+                parentItem->appendRow(itemList);
+                buildTree(options, defaults, index, item);
             } else {
                 // for root nodes
-                buildTree(options, defaults, index, item, testStatusItem);
-                m_model->appendRow(item);
-                m_statusModel->appendRow(testStatusItem);
+                buildTree(options, defaults, index, item);
+                QList<QStandardItem *> itemList;
+                itemList << item << testStatusItem;
+                m_model->appendRow(itemList);
             }
         }
         internalIndex++;
@@ -495,9 +486,11 @@
     // build the model only once
     if (!this->m_model) {
         this->m_model = new TreeModel();
+        this->m_model->setColumnCount(2);
         buildTree(options, defaults, "1");
         ui->treeView->setModel(m_model);
-        ui->statusView->setModel(m_statusModel);
+        ui->treeView->header()->setResizeMode(0, QHeaderView::Stretch);
+        ui->treeView->setColumnWidth(1, 150);
     }
     this->m_model->deselect_warning = QString(deselect_warning);
 
@@ -514,20 +507,17 @@
     if (item->hasChildren()) {
         int numRows = item->rowCount();
         for(int i=0; i< numRows; i++) {
-            QStandardItem *childItem = item->child(i, 0);
+            QStandardItem *childItem = item->child(i, 1);
             onJobItemChanged(childItem,job, baseIndex);
         }
     }
-    if (item->data(Qt::UserRole) == job) {
-        ui->statusView->setExpanded(item->index(), ui->treeView->isExpanded(baseIndex));
-    }
 }
 
 void QtFront::onJobItemChanged(QModelIndex index) {
     QString job = m_model->data(index).toString();
-    int numRows = m_statusModel->rowCount();
+    int numRows = m_model->rowCount();
     for(int i=0; i< numRows; i++) {
-        QStandardItem *item = m_statusModel->item(i, 0);
+        QStandardItem *item = m_model->item(i, 1);
         onJobItemChanged(item, job, index);
     }
 }

=== modified file 'qt/frontend/qtfront.h'
--- qt/frontend/qtfront.h	2012-06-21 21:02:29 +0000
+++ qt/frontend/qtfront.h	2012-08-03 08:19:21 +0000
@@ -76,7 +76,7 @@
 
     void onJobItemChanged(QModelIndex index);
     void onJobItemChanged(QStandardItem *item, QString job, QModelIndex baseIndex);
-    void updateTestStatus(QStandardItem *item, QString status);
+    void updateTestStatus(QStandardItem *item, QStandardItem* statusItem, QString status);
     void updateTestStatus(QString status = QString());
     void onClosedFrontend();
 
@@ -98,7 +98,7 @@
 
 private:
     bool registerService();
-    void buildTree(QVariantMap options, QVariantMap defaults, QString baseIndex = "1", QStandardItem *parentItem = 0, QStandardItem *parentStatusItem = 0);
+    void buildTree(QVariantMap options, QVariantMap defaults, QString baseIndex = "1", QStandardItem *parentItem = 0);
     void buildTestsToRun(QStandardItem *item, QString baseIndex, QVariantMap &items);
     Ui_main *ui;
     QWidget *m_mainWindow;

=== modified file 'qt/frontend/qtfront.ui'
--- qt/frontend/qtfront.ui	2012-08-03 01:17:41 +0000
+++ qt/frontend/qtfront.ui	2012-08-03 08:19:21 +0000
@@ -40,15 +40,15 @@
    </property>
    <item>
     <widget class="QTabWidget" name="tabWidget">
+     <property name="styleSheet">
+      <string notr="true">QTabBar::tab {min-height: 0ex;}</string>
+     </property>
      <property name="tabPosition">
       <enum>QTabWidget::North</enum>
      </property>
      <property name="tabShape">
       <enum>QTabWidget::Rounded</enum>
      </property>
-     <property name="styleSheet">
-      <string notr="true">QTabBar::tab {min-height: 0ex;}</string>
-     </property>     
      <property name="currentIndex">
       <number>0</number>
      </property>
@@ -277,28 +277,28 @@
              <active>
               <colorrole role="Button">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Base">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Window">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
@@ -306,28 +306,28 @@
              <inactive>
               <colorrole role="Button">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Base">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Window">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
@@ -335,28 +335,28 @@
              <disabled>
               <colorrole role="Button">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Base">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
               <colorrole role="Window">
                <brush brushstyle="SolidPattern">
-                <color alpha="255">
-                 <red>73</red>
-                 <green>73</green>
-                 <blue>80</blue>
+                <color alpha="0">
+                 <red>0</red>
+                 <green>0</green>
+                 <blue>0</blue>
                 </color>
                </brush>
               </colorrole>
@@ -509,32 +509,6 @@
               <string>Choose tests to run on your system:</string>
              </property>
             </widget>
-            <widget class="Line" name="line">
-             <property name="geometry">
-              <rect>
-               <x>10</x>
-               <y>60</y>
-               <width>601</width>
-               <height>20</height>
-              </rect>
-             </property>
-             <property name="orientation">
-              <enum>Qt::Horizontal</enum>
-             </property>
-            </widget>
-            <widget class="Line" name="line_2">
-             <property name="geometry">
-              <rect>
-               <x>390</x>
-               <y>40</y>
-               <width>20</width>
-               <height>271</height>
-              </rect>
-             </property>
-             <property name="orientation">
-              <enum>Qt::Vertical</enum>
-             </property>
-            </widget>
             <widget class="QLabel" name="label_2">
              <property name="geometry">
               <rect>
@@ -551,64 +525,33 @@
             <widget class="QLabel" name="label_3">
              <property name="geometry">
               <rect>
-               <x>410</x>
+               <x>450</x>
                <y>50</y>
                <width>67</width>
                <height>17</height>
               </rect>
              </property>
+             <property name="frameShape">
+              <enum>QFrame::NoFrame</enum>
+             </property>
+             <property name="frameShadow">
+              <enum>QFrame::Plain</enum>
+             </property>
              <property name="text">
               <string>Status</string>
              </property>
             </widget>
-            <widget class="QTreeView" name="statusView">
-             <property name="enabled">
-              <bool>false</bool>
-             </property>
+            <widget class="QTreeView" name="treeView">
              <property name="geometry">
               <rect>
-               <x>410</x>
-               <y>80</y>
-               <width>201</width>
-               <height>231</height>
+               <x>10</x>
+               <y>70</y>
+               <width>611</width>
+               <height>241</height>
               </rect>
              </property>
-             <property name="palette">
-              <palette>
-               <active>
-                <colorrole role="Base">
-                 <brush brushstyle="SolidPattern">
-                  <color alpha="255">
-                   <red>255</red>
-                   <green>255</green>
-                   <blue>255</blue>
-                  </color>
-                 </brush>
-                </colorrole>
-               </active>
-               <inactive>
-                <colorrole role="Base">
-                 <brush brushstyle="SolidPattern">
-                  <color alpha="255">
-                   <red>255</red>
-                   <green>255</green>
-                   <blue>255</blue>
-                  </color>
-                 </brush>
-                </colorrole>
-               </inactive>
-               <disabled>
-                <colorrole role="Base">
-                 <brush brushstyle="SolidPattern">
-                  <color alpha="255">
-                   <red>255</red>
-                   <green>255</green>
-                   <blue>255</blue>
-                  </color>
-                 </brush>
-                </colorrole>
-               </disabled>
-              </palette>
+             <property name="styleSheet">
+              <string notr="true"/>
              </property>
              <property name="frameShape">
               <enum>QFrame::Box</enum>
@@ -617,42 +560,26 @@
               <enum>QFrame::Plain</enum>
              </property>
              <property name="verticalScrollBarPolicy">
-              <enum>Qt::ScrollBarAlwaysOff</enum>
-             </property>
-             <property name="horizontalScrollBarPolicy">
-              <enum>Qt::ScrollBarAlwaysOff</enum>
-             </property>
-             <property name="headerHidden">
-              <bool>true</bool>
-             </property>
-             <attribute name="headerVisible">
-              <bool>false</bool>
-             </attribute>
-            </widget>
-            <widget class="QTreeView" name="treeView">
-             <property name="geometry">
-              <rect>
-               <x>10</x>
-               <y>80</y>
-               <width>381</width>
-               <height>231</height>
-              </rect>
-             </property>
-             <property name="frameShape">
-              <enum>QFrame::Box</enum>
-             </property>
-             <property name="frameShadow">
-              <enum>QFrame::Plain</enum>
+              <enum>Qt::ScrollBarAlwaysOn</enum>
              </property>
              <property name="showDropIndicator" stdset="0">
               <bool>false</bool>
              </property>
+             <property name="alternatingRowColors">
+              <bool>true</bool>
+             </property>
              <property name="allColumnsShowFocus">
               <bool>false</bool>
              </property>
              <attribute name="headerVisible">
               <bool>false</bool>
              </attribute>
+             <attribute name="headerCascadingSectionResizes">
+              <bool>false</bool>
+             </attribute>
+             <attribute name="headerStretchLastSection">
+              <bool>false</bool>
+             </attribute>
             </widget>
             <widget class="QPushButton" name="selectAllButton">
              <property name="enabled">
@@ -714,7 +641,7 @@
              <property name="orientation">
               <enum>Qt::Horizontal</enum>
              </property>
-            </widget>            
+            </widget>
            </widget>
            <widget class="QWidget" name="testing">
             <property name="enabled">
@@ -888,8 +815,8 @@
                                <rect>
                                 <x>0</x>
                                 <y>0</y>
-                                <width>62</width>
-                                <height>16</height>
+                                <width>100</width>
+                                <height>30</height>
                                </rect>
                               </property>
                               <layout class="QVBoxLayout" name="verticalLayout_4">

=== modified file 'qt/frontend/treemodel.cpp'
--- qt/frontend/treemodel.cpp	2012-06-21 21:02:29 +0000
+++ qt/frontend/treemodel.cpp	2012-08-03 08:19:21 +0000
@@ -28,7 +28,6 @@
     item->setEnabled(enable);
     for(int i=0; i < item->rowCount(); i++) {
         QStandardItem *childItem = item->child(i);
- //       childItem->setEnabled(enable);
         enableAllChildren(enable, childItem);
     }
 }