ENH: Improve color terminology text; Move color model back to MRML

- Improve color terminology text to have the format ": , in , ". Create static function in color model so that the same string can be easily generated from the model and the delegate
- Move the color model back to MRMLWidgets as preparation for showing simple color table in terminology selector for color tables
- Fix warnings

Re Slicer#6975, Slicer#7593

Libs/MRML/Core/vtkMRMLColorNode.cxx

@@ -353,7 +353,7 @@ bool vtkMRMLColorNode::GetColorNameAutoGenerated(int index)
   PropertyType prop;
   if (!this->GetProperty(index, prop))
   {
-    return nullptr;
+    return false;
   }
   return prop.NameAutoGenerated;
 }

Libs/MRML/Core/vtkMRMLColorTableStorageNode.cxx

@@ -660,7 +660,7 @@ int vtkMRMLColorTableStorageNode::WriteCsvFile(std::string fullFileName, vtkMRML
 
   // Set values for all colors in all column arrays
   unsigned int rowIndex = 0;
-  for (int colorIdx = 0; colorIdx < numberOfColors; colorIdx++)
+  for (unsigned int colorIdx = 0; colorIdx < numberOfColors; colorIdx++)
   {
     // Skip unnamed color
     if (colorNode->GetNoName() && colorNode->GetColorName(colorIdx) &&
@@ -676,7 +676,7 @@ int vtkMRMLColorTableStorageNode::WriteCsvFile(std::string fullFileName, vtkMRML
     nameArray->SetValue(rowIndex, colorNode->GetColorName(colorIdx));
     colorArray->SetTuple(rowIndex, rgba255);
 
-    for (int idx=0; idx<TERMINOLOGY_COLUMN_NAMES.size(); ++idx)
+    for (unsigned int idx=0; idx<TERMINOLOGY_COLUMN_NAMES.size(); ++idx)
     {
       GetEntryFuncPtr getTerminologyEntry = terminologyGetEntryFuncVector[idx];
       vtkCodedEntry* terminologyEntry = (colorNode->*getTerminologyEntry)(colorIdx);
@@ -729,7 +729,7 @@ int vtkMRMLColorTableStorageNode::WriteCtblFile(std::string fullFileName, vtkMRM
   {
     unsigned int numberOfColors = colorNode->GetLookupTable()->GetNumberOfTableValues();
     of << "# " << numberOfColors << " values" << endl;
-    for (int i = 0; i < numberOfColors; i++)
+    for (unsigned int i = 0; i < numberOfColors; i++)
     {
       // Skip unnamed color
       if (colorNode->GetNoName() && colorNode->GetColorName(i) &&

Libs/MRML/Widgets/CMakeLists.txt

@@ -71,6 +71,9 @@ set(MRMLWidgets_SRCS
   qMRMLClipNodeDisplayWidget.h
   qMRMLCollapsibleButton.cxx
   qMRMLCollapsibleButton.h
+  qMRMLColorModel.cxx
+  qMRMLColorModel.h
+  qMRMLColorModel_p.h
   qMRMLColors.cxx
   qMRMLColors.h
   qMRMLColorTableComboBox.cxx
@@ -241,6 +244,7 @@ set(MRMLWidgets_MOC_SRCS
   qMRMLClipNodeWidget.h
   qMRMLClipNodeDisplayWidget.h
   qMRMLCollapsibleButton.h
+  qMRMLColorModel.h
   qMRMLColorTableComboBox.h
   qMRMLCoordinatesWidget.h
   qMRMLDisplayNodeViewComboBox.h

Libs/MRML/Widgets/Testing/CMakeLists.txt

@@ -34,6 +34,8 @@ set(TEST_SOURCES
   qMRMLCheckableNodeComboBoxTest1.cxx
   qMRMLClipNodeWidgetTest1.cxx
   qMRMLCollapsibleButtonTest.cxx
+  qMRMLColorModelTest.cxx
+  qMRMLColorModelTest1.cxx
   qMRMLDisplayNodeViewComboBoxTest.cxx
   qMRMLDisplayNodeWidgetTest1.cxx
   qMRMLLayoutManagerTest1.cxx
@@ -135,6 +137,7 @@ set(Tests_UtilityFiles
 set(KIT_TEST_GENERATE_MOC_SRCS
   qMRMLCheckableNodeComboBoxTest.cxx
   qMRMLCollapsibleButtonTest.cxx
+  qMRMLColorModelTest.cxx
   qMRMLDisplayNodeViewComboBoxTest.cxx
   qMRMLLabelComboBoxTest.cxx
   qMRMLLayoutManagerWithCustomFactoryTest.cxx
@@ -186,6 +189,8 @@ simple_test( qMRMLCheckableNodeComboBoxTest )
 simple_test( qMRMLCheckableNodeComboBoxTest1 )
 simple_test( qMRMLClipNodeWidgetTest1 )
 simple_test( qMRMLCollapsibleButtonTest)
+simple_test( qMRMLColorModelTest )
+simple_test( qMRMLColorModelTest1 )
 simple_test( qMRMLColorTableComboBoxTest1 )
 simple_test( qMRMLDisplayNodeViewComboBoxTest )
 simple_test( qMRMLDisplayNodeWidgetTest1 )

Modules/Loadable/Colors/Widgets/qMRMLColorModel.cxx → Libs/MRML/Widgets/qMRMLColorModel.cxx

@@ -29,8 +29,7 @@
 // MRML includes
 #include <vtkCodedEntry.h>
 #include <vtkMRMLColorTableNode.h>
-
-// VTK includes
+#include "vtkMRMLI18N.h"
 
 //------------------------------------------------------------------------------
 qMRMLColorModelPrivate::qMRMLColorModelPrivate(qMRMLColorModel& object)
@@ -78,9 +77,7 @@ void qMRMLColorModelPrivate::init()
   }
 
   q->setHorizontalHeaderLabels(headerLabels);
-  QObject::connect(q, SIGNAL(itemChanged(QStandardItem*)),
-                   q, SLOT(onItemChanged(QStandardItem*)),
-                   Qt::UniqueConnection);
+  QObject::connect(q, SIGNAL(itemChanged(QStandardItem*)), q, SLOT(onItemChanged(QStandardItem*)), Qt::UniqueConnection);
 }
 
 //------------------------------------------------------------------------------
@@ -422,25 +419,7 @@ void qMRMLColorModel::updateItemFromColor(QStandardItem* item, int color, int co
   }
   if (column == d->TerminologyColumn)
   {
-    std::vector<vtkCodedEntry*> terminologyEntries
-    {
-      d->MRMLColorNode->GetTerminologyCategory(color),
-      d->MRMLColorNode->GetTerminologyType(color),
-      d->MRMLColorNode->GetTerminologyTypeModifier(color),
-      d->MRMLColorNode->GetTerminologyAnatomicRegion(color),
-      d->MRMLColorNode->GetTerminologyAnatomicRegionModifier(color)
-    };
-    QStringList terminologyStrList;
-    for (auto entry : terminologyEntries)
-    {
-      if (entry == nullptr)
-      {
-        continue;
-      }
-      terminologyStrList.append(QString::fromUtf8(entry->GetCodeMeaning()));
-    }
-    item->setText(terminologyStrList.join(", "));
-    item->setToolTip(terminologyStrList.join("\n"));
+    item->setText(qMRMLColorModel::terminologyTextForColor(d->MRMLColorNode, color));
     item->setData(QVariant::fromValue(reinterpret_cast<long long>(d->MRMLColorNode.GetPointer())), qMRMLItemDelegate::PointerRole);
   }
   if (column == d->CheckableColumn)
@@ -529,3 +508,45 @@ QVariant qMRMLColorModel::headerData(int section, Qt::Orientation orientation, i
   return retval;
 
 }
+
+//------------------------------------------------------------------------------
+QString qMRMLColorModel::terminologyTextForColor(vtkMRMLColorNode* colorNode, int colorIndex)
+{
+  QString text;
+  if (colorNode == nullptr || colorIndex >= colorNode->GetNumberOfColors())
+  {
+    return text;
+  }
+
+  if ( colorNode->GetTerminologyCategory(colorIndex) == nullptr || colorNode->GetTerminologyCategory(colorIndex)->GetCodeMeaning() == nullptr
+    || colorNode->GetTerminologyType(colorIndex) == nullptr || colorNode->GetTerminologyType(colorIndex)->GetCodeMeaning() == nullptr)
+  {
+    text.append(vtkMRMLTr("qMRMLColorModel", "(none)").c_str());
+    return text;
+  }
+
+  // Add category and type
+  text.append(QString("%1: %2").arg(
+    colorNode->GetTerminologyCategory(colorIndex)->GetCodeMeaning()).arg(colorNode->GetTerminologyType(colorIndex)->GetCodeMeaning()));
+  // Add type modifier if any
+  if ( colorNode->GetTerminologyTypeModifier(colorIndex) != nullptr
+    && colorNode->GetTerminologyTypeModifier(colorIndex)->GetCodeMeaning() != nullptr)
+  {
+    text.append(QString(", %1 ").arg(colorNode->GetTerminologyTypeModifier(colorIndex)->GetCodeMeaning()));
+  }
+  // Add anatomic region if any
+  if ( colorNode->GetTerminologyAnatomicRegion(colorIndex) != nullptr
+    && colorNode->GetTerminologyAnatomicRegion(colorIndex)->GetCodeMeaning() != nullptr)
+  {
+    text.append(vtkMRMLTr("qMRMLColorModel", "in").c_str());
+    text.append(QString(" %1").arg(colorNode->GetTerminologyAnatomicRegion(colorIndex)->GetCodeMeaning()));
+  }
+  // Add anatomic region modifier if any
+  if ( colorNode->GetTerminologyAnatomicRegionModifier(colorIndex) != nullptr
+    && colorNode->GetTerminologyAnatomicRegionModifier(colorIndex)->GetCodeMeaning() != nullptr)
+  {
+    text.append(QString(", %1").arg(colorNode->GetTerminologyAnatomicRegionModifier(colorIndex)->GetCodeMeaning()));
+  }
+
+  return text;
+}

Modules/Loadable/Colors/Widgets/qMRMLColorModel.h → Libs/MRML/Widgets/qMRMLColorModel.h

@@ -28,8 +28,8 @@
 #include <ctkPimpl.h>
 #include <ctkVTKObject.h>
 
-// Colors includes
-#include "qSlicerColorsModuleWidgetsExport.h"
+// MRMLWidgets includes
+#include "qMRMLWidgetsExport.h"
 
 class vtkMRMLNode;
 class vtkMRMLColorNode;
@@ -39,7 +39,7 @@ class QAction;
 class qMRMLColorModelPrivate;
 
 //------------------------------------------------------------------------------
-class Q_SLICER_MODULE_COLORS_WIDGETS_EXPORT qMRMLColorModel : public QStandardItemModel
+class QMRML_WIDGETS_EXPORT qMRMLColorModel : public QStandardItemModel
 {
   Q_OBJECT
   QVTK_OBJECT
@@ -124,6 +124,9 @@ class Q_SLICER_MODULE_COLORS_WIDGETS_EXPORT qMRMLColorModel : public QStandardIt
   /// so that can return the color index rather than the row
   QVariant headerData(int section, Qt::Orientation orientation, int role = Qt::DisplayRole) const override;
 
+  /// Assemble human readable text in format ": , in , " from color in color node.
+  static QString terminologyTextForColor(vtkMRMLColorNode* colorNode, int colorIndex);
+
 protected slots:
   void onMRMLColorNodeModified(vtkObject* node);
   void onItemChanged(QStandardItem * item);

Modules/Loadable/Colors/Widgets/qMRMLColorModel_p.h → Libs/MRML/Widgets/qMRMLColorModel_p.h

@@ -48,7 +48,7 @@ class vtkMRMLColorNode;
 //------------------------------------------------------------------------------
 // qMRMLColorModelPrivate
 //------------------------------------------------------------------------------
-class Q_SLICER_MODULE_COLORS_WIDGETS_EXPORT qMRMLColorModelPrivate
+class QMRML_WIDGETS_EXPORT qMRMLColorModelPrivate
 {
   Q_DECLARE_PUBLIC(qMRMLColorModel);
 protected:

Modules/Loadable/Colors/Widgets/CMakeLists.txt

@@ -14,9 +14,6 @@ set(${KIT}_SRCS
   qMRMLColorLegendDisplayNodeWidget.h
   qMRMLColorListView.cxx
   qMRMLColorListView.h
-  qMRMLColorModel.cxx
-  qMRMLColorModel.h
-  qMRMLColorModel_p.h
   qMRMLColorPickerWidget.cxx
   qMRMLColorPickerWidget.h
   qMRMLColorTableView.cxx
@@ -34,7 +31,6 @@ set(${KIT}_SRCS
 set(${KIT}_MOC_SRCS
   qMRMLColorLegendDisplayNodeWidget.h
   qMRMLColorListView.h
-  qMRMLColorModel.h
   qMRMLColorPickerWidget.h
   qMRMLColorTableView.h
   qSlicerColorTableTerminologyDelegate.h

Modules/Loadable/Colors/Widgets/Testing/CMakeLists.txt

@@ -2,8 +2,6 @@ set(KIT ${PROJECT_NAME})
 
 set(TEST_SOURCES
   qMRMLColorListViewTest1.cxx
-  qMRMLColorModelTest.cxx
-  qMRMLColorModelTest1.cxx
   qMRMLColorPickerWidgetTest1.cxx
   qMRMLColorPickerWidgetTest2.cxx
   qMRMLColorPickerWidgetTest3.cxx
@@ -22,23 +20,14 @@ create_test_sourcelist(Tests ${KIT}CppTests.cxx
   ${TEST_SOURCES}
   )
 
-set(KIT_TEST_GENERATE_MOC_SRCS
-  qMRMLColorModelTest.cxx
-  )
-  set(_moc_options OPTIONS -DMRML_WIDGETS_HAVE_QT5)
-  QT5_WRAP_CPP(Tests_MOC_CXX ${_moc_options})
-  QT5_GENERATE_MOCS(${KIT_TEST_GENERATE_MOC_SRCS})
-
-include_directories( ${CMAKE_CURRENT_BINARY_DIR})
-include_directories( ${CMAKE_CURRENT_SOURCE_DIR})
+include_directories(${CMAKE_CURRENT_BINARY_DIR})
+include_directories(${CMAKE_CURRENT_SOURCE_DIR})
 
 ctk_add_executable_utf8(${KIT}CxxTests ${Tests} ${Tests_MOC_CXX})
 target_link_libraries(${KIT}CxxTests ${KIT})
 
 #-----------------------------------------------------------------------------
 simple_test( qMRMLColorListViewTest1 )
-simple_test( qMRMLColorModelTest )
-simple_test( qMRMLColorModelTest1 )
 simple_test( qMRMLColorPickerWidgetTest1 )
 simple_test( qMRMLColorPickerWidgetTest2 )
 simple_test( qMRMLColorPickerWidgetTest3 )

Modules/Loadable/Colors/Widgets/qSlicerColorTableTerminologyDelegate.cxx

@@ -37,6 +37,7 @@
 
 // MRMLWidgets includes
 #include "qMRMLItemDelegate.h"
+#include "qMRMLColorModel.h"
 
 // MRML includes
 #include "vtkMRMLColorTableNode.h"
@@ -174,26 +175,7 @@ void qSlicerColorTableTerminologyDelegate::setModelData(QWidget* editor, QAbstra
 
   colorNode->SetTerminologyFromString(colorIndex, logic->SerializeTerminologyEntry(entry));
 
-  // Set text
-  std::vector<vtkCodedEntry*> terminologyEntries
-  {
-    colorNode->GetTerminologyCategory(colorIndex),
-    colorNode->GetTerminologyType(colorIndex),
-    colorNode->GetTerminologyTypeModifier(colorIndex),
-    colorNode->GetTerminologyAnatomicRegion(colorIndex),
-    colorNode->GetTerminologyAnatomicRegionModifier(colorIndex)
-  };
-  QStringList terminologyStrList;
-  for (auto entry : terminologyEntries)
-  {
-    if (entry == nullptr)
-    {
-      continue;
-    }
-    terminologyStrList.append(QString::fromUtf8(entry->GetCodeMeaning()));
-  }
-
-  model->setData(index, terminologyStrList.join(", "), Qt::DisplayRole);
+  model->setData(index, qMRMLColorModel::terminologyTextForColor(colorNode, colorIndex), Qt::DisplayRole);
 }
 
 //-----------------------------------------------------------------------------