From 5e2f583a67c75c1c03e213467bb56207e5084279 Mon Sep 17 00:00:00 2001
From: Albert Astals Cid <albert.astals.cid@kdab.com>
Date: Thu, 21 Dec 2017 16:55:42 +0100
Subject: [PATCH] CUPS: Fix conflict handling

The previous code assumed that ppdMarkOption returning non zero (i.e. it has conflicts)
also meant that the option wasn't applied at the ppd level, but it actually is.

What we need to do is after calling ppdMarkOption parse the tree again looking
to see if any option is conflicting and mark it as such in the UI.

Change-Id: I836f1902d14dc8c176bb06776471cbf4ed11786f
Reviewed-by: Andy Shaw <andy.shaw@qt.io>
---
 src/plugins/printsupport/cups/qppdprintdevice.cpp  |   6 +-
 src/printsupport/dialogs/qprintdialog_unix.cpp     | 101 ++++++++++++++++++++-
 src/printsupport/dialogs/qprintpropertieswidget.ui |  65 +++++++++++--
 3 files changed, 160 insertions(+), 12 deletions(-)

diff --git a/src/plugins/printsupport/cups/qppdprintdevice.cpp b/src/plugins/printsupport/cups/qppdprintdevice.cpp
index 8aed8c5..9c4b699 100644
--- a/src/plugins/printsupport/cups/qppdprintdevice.cpp
+++ b/src/plugins/printsupport/cups/qppdprintdevice.cpp
@@ -443,8 +443,10 @@ bool QPpdPrintDevice::setProperty(QPrintDevice::PrintDevicePropertyKey key, cons
 {
     if (key == PDPK_PpdOption) {
         const QStringList values = value.toStringList();
-        if (values.count() == 2)
-            return ppdMarkOption(m_ppd, values[0].toLatin1(), values[1].toLatin1()) == 0;
+        if (values.count() == 2) {
+            ppdMarkOption(m_ppd, values[0].toLatin1(), values[1].toLatin1());
+            return true;
+        }
     }
 
     return false;
diff --git a/src/printsupport/dialogs/qprintdialog_unix.cpp b/src/printsupport/dialogs/qprintdialog_unix.cpp
index 3b1c492..fcb53f4 100644
--- a/src/printsupport/dialogs/qprintdialog_unix.cpp
+++ b/src/printsupport/dialogs/qprintdialog_unix.cpp
@@ -141,6 +141,7 @@ public:
 
 private slots:
     void reject() Q_DECL_OVERRIDE;
+    void accept() Q_DECL_OVERRIDE;
 
 private:
     friend class QUnixPrintWidgetPrivate;
@@ -298,6 +299,12 @@ public:
     QPrintDevice *currentPrintDevice() const;
     QTextCodec *cupsCodec() const;
 
+    void emitConflictsChanged();
+    bool hasConflicts() const;
+
+signals:
+    void hasConflictsChanged(bool conflicts);
+
 private:
     void parseGroups(QOptionTreeItem *parent);
     void parseOptions(QOptionTreeItem *parent);
@@ -305,6 +312,8 @@ private:
 
     void setCupsOptionsFromItems(QPrinter *printer, QOptionTreeItem *parent) const;
     void reject(QOptionTreeItem *item);
+    void emitDataChanged(QOptionTreeItem *item, const QModelIndex &itemIndex, bool *conflictsFound);
+    bool hasConflicts(QOptionTreeItem *item) const;
 
     QPrintDevice *m_currentPrintDevice;
     QTextCodec *m_cupsCodec;
@@ -377,6 +386,9 @@ QPrintPropertiesDialog::QPrintPropertiesDialog(QPrinter *printer, QPrintDevice *
         widget.treeView->setModel(nullptr);
         widget.tabs->setTabEnabled(advancedTabIndex, false);
     }
+
+    widget.conflictsLabel->setVisible(m_cupsOptionsModel->hasConflicts());
+    connect(m_cupsOptionsModel, &QPPDOptionsModel::hasConflictsChanged, widget.conflictsLabel, &QLabel::setVisible);
 #else
     Q_UNUSED(currentPrintDevice)
     widget.tabs->setTabEnabled(advancedTabIndex, false);
@@ -413,6 +425,21 @@ void QPrintPropertiesDialog::reject()
     QDialog::reject();
 }
 
+void QPrintPropertiesDialog::accept()
+{
+#if QT_CONFIG(cups)
+    if (m_cupsOptionsModel->hasConflicts()) {
+        widget.tabs->setCurrentWidget(widget.cupsPropertiesPage);
+        const QMessageBox::StandardButton answer = QMessageBox::warning(this, tr("Advanced option conflicts"),
+                                                                        tr("There are conflicts in some advanced options. Do you want to fix them?"),
+                                                                        QMessageBox::Yes | QMessageBox::No, QMessageBox::Yes);
+        if (answer != QMessageBox::No)
+            return;
+    }
+#endif
+    QDialog::accept();
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 ////////////////////////////////////////////////////////////////////////////////
 
@@ -1180,6 +1207,22 @@ QVariant QPPDOptionsModel::data(const QModelIndex &index, int role) const
     }
     break;
 
+    case Qt::DecorationRole: {
+        if (itm->type == QOptionTreeItem::Option && index.column() == 1) {
+            const ppd_option_t *option = static_cast<const ppd_option_t*>(itm->ptr);
+            if (option->conflicted) {
+                const QIcon warning = QApplication::style()->standardIcon(QStyle::SP_MessageBoxWarning, nullptr, nullptr);
+                if (!warning.isNull())
+                    return warning;
+
+                qWarning() << "Current application style returned a null icon for SP_MessageBoxWarning.";
+                return QColor(Qt::red);
+            }
+        }
+        return QVariant();
+    }
+    break;
+
     }
 
     return QVariant();
@@ -1316,6 +1359,55 @@ void QPPDOptionsModel::parseChoices(QOptionTreeItemOption *parent)
     }
 }
 
+bool QPPDOptionsModel::hasConflicts() const
+{
+    return hasConflicts(m_rootItem);
+}
+
+bool QPPDOptionsModel::hasConflicts(QOptionTreeItem *item) const
+{
+    if (item->type == QOptionTreeItem::Option) {
+        const ppd_option_t *option = static_cast<const ppd_option_t*>(item->ptr);
+        return option->conflicted;
+    }
+
+    for (QOptionTreeItem *child : qAsConst(item->childItems)) {
+        if (hasConflicts(child))
+            return true;
+    }
+
+    return false;
+}
+
+void QPPDOptionsModel::emitConflictsChanged()
+{
+    bool conflictsFound = false;
+    emitDataChanged(m_rootItem, QModelIndex(), &conflictsFound);
+
+    emit hasConflictsChanged(conflictsFound);
+}
+
+void QPPDOptionsModel::emitDataChanged(QOptionTreeItem *item, const QModelIndex &itemIndex, bool *conflictsFound)
+{
+    if (item->type == QOptionTreeItem::Option) {
+        // We just emit DecorationRole dataChanged for all the leaves
+        // and let the view requery the value
+        const QModelIndex secondColItem = index(itemIndex.row(), 1, itemIndex.parent());
+        emit dataChanged(secondColItem, secondColItem, QVector<int>() << Qt::DecorationRole);
+
+        if (conflictsFound && *conflictsFound == false) {
+            const ppd_option_t *option = static_cast<const ppd_option_t*>(item->ptr);
+            if (option->conflicted && conflictsFound)
+                *conflictsFound = true;
+        }
+    }
+
+    for (int i = 0; i < item->childItems.count(); ++i) {
+        QOptionTreeItem *child = item->childItems.at(i);
+        emitDataChanged(child, index(i, 0, itemIndex), conflictsFound);
+    }
+}
+
 QVariant QPPDOptionsModel::headerData(int section, Qt::Orientation, int role) const
 {
     if (role != Qt::DisplayRole)
@@ -1404,10 +1496,11 @@ void QPPDOptionsEditor::setModelData(QWidget *editor, QAbstractItemModel *model,
     QPPDOptionsModel *m = static_cast<QPPDOptionsModel*>(model);
 
     const auto values = QStringList{} << QString::fromLatin1(opt->keyword) << QString::fromLatin1(opt->choices[cb->currentIndex()].choice);
-    if (m->currentPrintDevice()->setProperty(PDPK_PpdOption, values)) {
-        itm->selected = cb->currentIndex();
-        itm->selDescription = static_cast<const ppd_option_t*>(itm->ptr)->choices[itm->selected].text;
-    }
+    m->currentPrintDevice()->setProperty(PDPK_PpdOption, values);
+    itm->selected = cb->currentIndex();
+    itm->selDescription = static_cast<const ppd_option_t*>(itm->ptr)->choices[itm->selected].text;
+
+    m->emitConflictsChanged();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/src/printsupport/dialogs/qprintpropertieswidget.ui b/src/printsupport/dialogs/qprintpropertieswidget.ui
index 0e56fb3..d8e5261 100644
--- a/src/printsupport/dialogs/qprintpropertieswidget.ui
+++ b/src/printsupport/dialogs/qprintpropertieswidget.ui
@@ -14,7 +14,16 @@
    <string>Form</string>
   </property>
   <layout class="QVBoxLayout" name="verticalLayout_4">
-   <property name="margin">
+   <property name="leftMargin">
+    <number>0</number>
+   </property>
+   <property name="topMargin">
+    <number>0</number>
+   </property>
+   <property name="rightMargin">
+    <number>0</number>
+   </property>
+   <property name="bottomMargin">
     <number>0</number>
    </property>
    <item>
@@ -32,18 +41,62 @@
        </item>
       </layout>
      </widget>
-     <widget class="QWidget" name="cupsPropertiesPage" >
-      <attribute name="title" >
+     <widget class="QWidget" name="cupsPropertiesPage">
+      <attribute name="title">
        <string>Advanced</string>
       </attribute>
-      <layout class="QHBoxLayout" name="horizontalLayout_2" >
+      <layout class="QVBoxLayout" name="verticalLayout_2">
        <item>
-        <widget class="QTreeView" name="treeView" >
-         <property name="alternatingRowColors" >
+        <widget class="QTreeView" name="treeView">
+         <property name="alternatingRowColors">
           <bool>true</bool>
          </property>
         </widget>
        </item>
+       <item>
+        <widget class="QLabel" name="conflictsLabel">
+         <property name="palette">
+          <palette>
+           <active>
+            <colorrole role="WindowText">
+             <brush brushstyle="SolidPattern">
+              <color alpha="255">
+               <red>255</red>
+               <green>0</green>
+               <blue>0</blue>
+              </color>
+             </brush>
+            </colorrole>
+           </active>
+           <inactive>
+            <colorrole role="WindowText">
+             <brush brushstyle="SolidPattern">
+              <color alpha="255">
+               <red>255</red>
+               <green>0</green>
+               <blue>0</blue>
+              </color>
+             </brush>
+            </colorrole>
+           </inactive>
+           <disabled>
+            <colorrole role="WindowText">
+             <brush brushstyle="SolidPattern">
+              <color alpha="255">
+               <red>165</red>
+               <green>167</green>
+               <blue>169</blue>
+              </color>
+             </brush>
+            </colorrole>
+           </disabled>
+          </palette>
+         </property>
+         <property name="text">
+          <string>There are conflicts in some options. Please fix them.</string>
+         </property>
+        </widget>
+       </item>
       </layout>
      </widget>
     </widget>
-- 
2.7.4

