ProjectExplorer: Re-do the EnvironmentWidget

Editing in the item view was cumbersome and did not generally allow one
variable to refer to another, as there was no defined order on the
assignments.
Therefore, we make the item view read-only and promote the batch editing
widget to be the standard input method. User changes are not sorted
anymore, except in the summary text of the details widget.

Fixes: QTCREATORBUG-28480
Change-Id: I225cf86fff7b001a57d663e1fd267a4645e695c4
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: hjk <hjk@qt.io>

src/libs/utils/namevaluemodel.cpp

@@ -179,7 +179,7 @@ QVariant NameValueModel::data(const QModelIndex &index, int role) const
 Qt::ItemFlags NameValueModel::flags(const QModelIndex &index) const
 {
     Q_UNUSED(index)
-    return Qt::ItemIsSelectable | Qt::ItemIsEditable | Qt::ItemIsEnabled;
+    return Qt::ItemIsSelectable | Qt::ItemIsEnabled;
 }
 
 QVariant NameValueModel::headerData(int section, Qt::Orientation orientation, int role) const
@@ -264,10 +264,7 @@ bool NameValueModel::setData(const QModelIndex &index, const QVariant &value, in
 
 QModelIndex NameValueModel::addVariable()
 {
-    //: Name when inserting a new variable
-    return addVariable(NameValueItem(Tr::tr("<VARIABLE>"),
-                                     //: Value when inserting a new variable
-                                     Tr::tr("<VALUE>")));
+    return addVariable(NameValueItem("NEWVAR", "VALUE"));
 }
 
 QModelIndex NameValueModel::addVariable(const NameValueItem &item)

src/libs/utils/namevaluesdialog.cpp

@@ -12,6 +12,8 @@
 #include <QPlainTextEdit>
 #include <QPushButton>
 #include <QSet>
+#include <QTextBlock>
+#include <QTimer>
 #include <QVBoxLayout>
 
 namespace Utils {
@@ -34,20 +36,21 @@ static EnvironmentItems cleanUp(const EnvironmentItems &items)
     return uniqueItems;
 }
 
-class NameValueItemsWidget : public QWidget
+class TextEditHelper : public QPlainTextEdit
 {
+    Q_OBJECT
 public:
-    explicit NameValueItemsWidget(QWidget *parent = nullptr);
+    using QPlainTextEdit::QPlainTextEdit;
 
-    void setEnvironmentItems(const EnvironmentItems &items);
-    EnvironmentItems environmentItems() const;
-
-    void setPlaceholderText(const QString &text);
+signals:
+    void lostFocus();
 
 private:
-    QPlainTextEdit *m_editor;
+    void focusOutEvent(QFocusEvent *) override { emit lostFocus(); }
 };
 
+} // namespace Internal
+
 NameValueItemsWidget::NameValueItemsWidget(QWidget *parent)
     : QWidget(parent)
 {
@@ -60,26 +63,41 @@ NameValueItemsWidget::NameValueItemsWidget(QWidget *parent)
         "To clear a variable, put its name on a line with nothing else on it.\n"
         "To disable a variable, prefix the line with \"#\".");
 
-    m_editor = new QPlainTextEdit(this);
+    m_editor = new Internal::TextEditHelper(this);
     auto layout = new QVBoxLayout(this);
     layout->setContentsMargins(0, 0, 0, 0);
     layout->addWidget(m_editor);
     layout->addWidget(new QLabel(helpText, this));
+
+    const auto checkForItemChange = [this] {
+        const NameValueItems newItems = environmentItems();
+        if (newItems != m_originalItems) {
+            m_originalItems = newItems;
+            emit userChangedItems(newItems);
+        }
+    };
+    const auto timer = new QTimer(this);
+    timer->setSingleShot(true);
+    timer->setInterval(1000);
+    connect(m_editor, &QPlainTextEdit::textChanged, timer, qOverload<>(&QTimer::start));
+    connect(timer, &QTimer::timeout, this, checkForItemChange);
+    connect(m_editor, &Internal::TextEditHelper::lostFocus, this, [timer, checkForItemChange] {
+        timer->stop();
+        checkForItemChange();
+    });
 }
 
 void NameValueItemsWidget::setEnvironmentItems(const EnvironmentItems &items)
 {
-    EnvironmentItems sortedItems = items;
-    EnvironmentItem::sort(&sortedItems);
-    const QStringList list = EnvironmentItem::toStringList(sortedItems);
-    m_editor->document()->setPlainText(list.join(QLatin1Char('\n')));
+    m_originalItems = items;
+    m_editor->document()->setPlainText(EnvironmentItem::toStringList(items)
+                                           .join(QLatin1Char('\n')));
 }
 
 EnvironmentItems NameValueItemsWidget::environmentItems() const
 {
     const QStringList list = m_editor->document()->toPlainText().split(QLatin1String("\n"));
-    EnvironmentItems items = EnvironmentItem::fromStringList(list);
-    return cleanUp(items);
+    return Internal::cleanUp(EnvironmentItem::fromStringList(list));
 }
 
 void NameValueItemsWidget::setPlaceholderText(const QString &text)
@@ -87,13 +105,61 @@ void NameValueItemsWidget::setPlaceholderText(const QString &text)
     m_editor->setPlaceholderText(text);
 }
 
-} // namespace Internal
+bool NameValueItemsWidget::editVariable(const QString &name, Selection selection)
+{
+    QTextDocument * const doc = m_editor->document();
+    for (QTextBlock b = doc->lastBlock(); b.isValid(); b = b.previous()) {
+        const QString &line = b.text();
+        qsizetype offset = 0;
+        const auto skipWhiteSpace = [&] {
+            for (; offset < line.length(); ++offset) {
+                if (!line.at(offset).isSpace())
+                    return;
+            }
+        };
+        skipWhiteSpace();
+        if (line.mid(offset, name.size()) != name)
+            continue;
+        offset += name.size();
+
+        const auto updateCursor = [&](int anchor, int pos) {
+            QTextCursor newCursor(doc);
+            newCursor.setPosition(anchor);
+            newCursor.setPosition(pos, QTextCursor::KeepAnchor);
+            m_editor->setTextCursor(newCursor);
+        };
+
+        if (selection == Selection::Name) {
+            m_editor->setFocus();
+            updateCursor(b.position() + offset, b.position());
+            return true;
+        }
+
+        skipWhiteSpace();
+        if (offset < line.length()) {
+            QChar nextChar = line.at(offset);
+            if (nextChar.isLetterOrNumber())
+                continue;
+            if (nextChar == '=') {
+                if (++offset < line.length() && line.at(offset) == '+')
+                    ++offset;
+            } else if (nextChar == '+') {
+                if (++offset < line.length() && line.at(offset) == '=')
+                    ++offset;
+            }
+        }
+        m_editor->setFocus();
+        updateCursor(b.position() + b.length() - 1, b.position() + offset);
+        return true;
+    }
+    return false;
+}
 
 NameValuesDialog::NameValuesDialog(const QString &windowTitle, QWidget *parent)
     : QDialog(parent)
 {
     resize(640, 480);
-    m_editor = new Internal::NameValueItemsWidget(this);
+    m_editor = new NameValueItemsWidget(this);
     auto box = new QDialogButtonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel,
                                     Qt::Horizontal,
                                     this);
@@ -143,3 +209,5 @@ std::optional<NameValueItems> NameValuesDialog::getNameValueItems(QWidget *paren
 }
 
 } // namespace Utils
+
+#include <namevaluesdialog.moc>

src/libs/utils/namevaluesdialog.h

@@ -3,19 +3,39 @@
 
 #pragma once
 
-#include "utils_global.h"
-
 #include "namevalueitem.h"
+#include "utils_global.h"
 
 #include <QDialog>
 
 #include <functional>
-#include <memory>
 #include <optional>
 
 namespace Utils {
 
-namespace Internal { class NameValueItemsWidget; }
+namespace Internal { class TextEditHelper; }
+
+class QTCREATOR_UTILS_EXPORT NameValueItemsWidget : public QWidget
+{
+    Q_OBJECT
+public:
+    explicit NameValueItemsWidget(QWidget *parent = nullptr);
+
+    void setEnvironmentItems(const EnvironmentItems &items);
+    EnvironmentItems environmentItems() const;
+
+    void setPlaceholderText(const QString &text);
+
+    enum class Selection { Name, Value };
+    bool editVariable(const QString &name, Selection selection);
+
+signals:
+    void userChangedItems(const EnvironmentItems &items);
+
+private:
+    Internal::TextEditHelper *m_editor;
+    NameValueItems m_originalItems;
+};
 
 class QTCREATOR_UTILS_EXPORT NameValuesDialog : public QDialog
 {
@@ -36,7 +56,7 @@ protected:
                               QWidget *parent = {});
 
 private:
-    Internal::NameValueItemsWidget *m_editor;
+    NameValueItemsWidget *m_editor;
 };
 
 } // namespace Utils

src/plugins/projectexplorer/environmentwidget.cpp

@@ -125,38 +125,19 @@ private:
     PathTreeWidget m_view;
 };
 
-class EnvironmentDelegate : public QStyledItemDelegate
-{
-public:
-    EnvironmentDelegate(Utils::EnvironmentModel *model,
-                        QTreeView *view)
-        : QStyledItemDelegate(view), m_model(model), m_view(view)
-    {}
-
-    QWidget *createEditor(QWidget *parent, const QStyleOptionViewItem &option, const QModelIndex &index) const override
-    {
-        QWidget *w = QStyledItemDelegate::createEditor(parent, option, index);
-        if (index.column() != 0)
-            return w;
-
-        if (auto edit = qobject_cast<QLineEdit *>(w))
-            edit->setValidator(new Utils::NameValueValidator(
-                edit, m_model, m_view, index, Tr::tr("Variable already exists.")));
-        return w;
-    }
-private:
-    Utils::EnvironmentModel *m_model;
-    QTreeView *m_view;
-};
-
-
 ////
 // EnvironmentWidget::EnvironmentWidget
 ////
 
-class EnvironmentWidgetPrivate
+class EnvironmentWidget::Private
 {
 public:
+    Private(EnvironmentWidget *q) : q(q) {}
+
+    void handleEditRequest(NameValueItemsWidget::Selection selection);
+
+    EnvironmentWidget * const q;
+    Utils::NameValueItemsWidget m_editor;
     Utils::EnvironmentModel *m_model;
     EnvironmentWidget::Type m_type = EnvironmentWidget::TypeLocal;
     QString m_baseEnvironmentText;
@@ -168,14 +149,13 @@ public:
     QPushButton *m_resetButton;
     QPushButton *m_unsetButton;
     QPushButton *m_toggleButton;
-    QPushButton *m_batchEditButton;
     QPushButton *m_appendPathButton = nullptr;
     QPushButton *m_prependPathButton = nullptr;
     QPushButton *m_terminalButton;
 };
 
 EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additionalDetailsWidget)
-    : QWidget(parent), d(std::make_unique<EnvironmentWidgetPrivate>())
+    : QWidget(parent), d(std::make_unique<Private>(this))
 {
     d->m_model = new Utils::EnvironmentModel();
     d->m_type = type;
@@ -183,14 +163,27 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
             this, &EnvironmentWidget::userChangesChanged);
     connect(d->m_model, &QAbstractItemModel::modelReset,
             this, &EnvironmentWidget::invalidateCurrentIndex);
-
     connect(d->m_model, &Utils::EnvironmentModel::focusIndex,
             this, &EnvironmentWidget::focusIndex);
 
+    // The text edit represents the raw, unexpanded user changes.
+    // There are two possible data flows:
+    //   a) text edit -> model -> view for when the user types in the text edit
+    //   b) top level widget
+    //        -> text edit
+    //        -> model -> view
+    //      for when convenience functionality is used via the buttons.
+    //   So the model is always updated from the text edit or text edit and model
+    //   are updated in sync from this widget.
+    connect(&d->m_editor, &NameValueItemsWidget::userChangedItems,
+            d->m_model, &EnvironmentModel::setUserChanges);
+
     auto vbox = new QVBoxLayout(this);
     vbox->setContentsMargins(0, 0, 0, 0);
 
     d->m_detailsContainer = new Utils::DetailsWidget(this);
+    connect(d->m_detailsContainer, &DetailsWidget::expanded,
+            this, &EnvironmentWidget::updateSummaryText);
 
     auto details = new QWidget(d->m_detailsContainer);
     d->m_detailsContainer->setWidget(details);
@@ -209,7 +202,6 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
             tree, [tree](const QModelIndex &idx) { tree->edit(idx); });
     d->m_environmentView = tree;
     d->m_environmentView->setModel(d->m_model);
-    d->m_environmentView->setItemDelegate(new EnvironmentDelegate(d->m_model, d->m_environmentView));
     d->m_environmentView->setMinimumHeight(400);
     d->m_environmentView->setRootIsDecorated(false);
     const auto stretcher = new HeaderViewStretcher(d->m_environmentView->header(), 1);
@@ -218,7 +210,7 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
     connect(d->m_model, &EnvironmentModel::userChangesChanged,
             stretcher, &HeaderViewStretcher::softStretch);
     d->m_environmentView->setSelectionMode(QAbstractItemView::SingleSelection);
-    d->m_environmentView->setSelectionBehavior(QAbstractItemView::SelectItems);
+    d->m_environmentView->setSelectionBehavior(QAbstractItemView::SelectRows);
     d->m_environmentView->setFrameShape(QFrame::NoFrame);
     QFrame *findWrapper = Core::ItemViewFind::createSearchableWrapper(d->m_environmentView, Core::ItemViewFind::LightColored);
     findWrapper->setFrameStyle(QFrame::StyledPanel);
@@ -266,10 +258,6 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
                 this, &EnvironmentWidget::prependPathButtonClicked);
     }
 
-    d->m_batchEditButton = new QPushButton(this);
-    d->m_batchEditButton->setText(Tr::tr("&Batch Edit..."));
-    buttonLayout->addWidget(d->m_batchEditButton);
-
     d->m_terminalButton = new QPushButton(this);
     d->m_terminalButton->setText(Tr::tr("Open &Terminal"));
     d->m_terminalButton->setToolTip(Tr::tr("Open a terminal with this environment set up."));
@@ -278,6 +266,7 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
     buttonLayout->addStretch();
 
     horizontalLayout->addLayout(buttonLayout);
+    horizontalLayout->addWidget(&d->m_editor);
     vbox2->addLayout(horizontalLayout);
 
     vbox->addWidget(d->m_detailsContainer);
@@ -293,8 +282,6 @@ EnvironmentWidget::EnvironmentWidget(QWidget *parent, Type type, QWidget *additi
             this, &EnvironmentWidget::removeEnvironmentButtonClicked);
     connect(d->m_unsetButton, &QAbstractButton::clicked,
             this, &EnvironmentWidget::unsetEnvironmentButtonClicked);
-    connect(d->m_batchEditButton, &QAbstractButton::clicked,
-            this, &EnvironmentWidget::batchEditEnvironmentButtonClicked);
     connect(d->m_environmentView->selectionModel(), &QItemSelectionModel::currentChanged,
             this, &EnvironmentWidget::environmentCurrentIndexChanged);
     connect(d->m_terminalButton, &QAbstractButton::clicked,
@@ -354,7 +341,7 @@ Utils::EnvironmentItems EnvironmentWidget::userChanges() const
 void EnvironmentWidget::setUserChanges(const Utils::EnvironmentItems &list)
 {
     d->m_model->setUserChanges(list);
-    updateSummaryText();
+    d->m_editor.setEnvironmentItems(list);
 }
 
 void EnvironmentWidget::setOpenTerminalFunc(const EnvironmentWidget::OpenTerminalFunc &func)
@@ -370,6 +357,12 @@ void EnvironmentWidget::expand()
 
 void EnvironmentWidget::updateSummaryText()
 {
+    // The summary is redundant with the text edit, so we hide it on expansion.
+    if (d->m_detailsContainer->state() == DetailsWidget::Expanded) {
+        d->m_detailsContainer->setSummaryText({});
+        return;
+    }
+
     Utils::EnvironmentItems list = d->m_model->userChanges();
     Utils::EnvironmentItem::sort(&list);
 
@@ -428,30 +421,52 @@ void EnvironmentWidget::updateButtons()
 
 void EnvironmentWidget::editEnvironmentButtonClicked()
 {
-    const QModelIndex current = d->m_environmentView->currentIndex();
-    if (current.column() == 1
-            && d->m_type == TypeLocal
-            && d->m_model->currentEntryIsPathList(current)) {
-        PathListDialog dlg(d->m_model->indexToVariable(current),
-                           d->m_model->data(current).toString(), this);
-        if (dlg.exec() == QDialog::Accepted)
-            d->m_model->setData(current, dlg.paths());
-    } else {
-        d->m_environmentView->edit(current);
+    d->handleEditRequest(NameValueItemsWidget::Selection::Value);
+}
+
+void EnvironmentWidget::Private::handleEditRequest(NameValueItemsWidget::Selection selection)
+{
+    QModelIndex current = m_environmentView->currentIndex();
+    if (current.column() == 0)
+        current = current.siblingAtColumn(1);
+    const QString varName = m_model->indexToVariable(current);
+
+    // Known path lists are edited via a dedicated widget.
+    if (m_type == TypeLocal && m_model->currentEntryIsPathList(current)) {
+        PathListDialog dlg(varName, m_model->data(current).toString(), q);
+        if (dlg.exec() == QDialog::Accepted) {
+            m_model->setData(current, dlg.paths());
+            m_editor.setEnvironmentItems(m_model->userChanges());
+        }
+        return;
     }
+
+    if (m_editor.editVariable(varName, selection))
+        return;
+
+    // The variable does not appear on an LHS in the text edit. This means it is not
+    // set or unset in any of the user changes. Therefore, we have to create a new
+    // change item for the user to edit.
+    EnvironmentItems items = m_model->userChanges();
+    items.append({varName, "NEWVALUE"});
+    q->setUserChanges(items);
+    const bool editable = m_editor.editVariable(varName, NameValueItemsWidget::Selection::Value);
+    QTC_CHECK(editable);
 }
 
 void EnvironmentWidget::addEnvironmentButtonClicked()
 {
     QModelIndex index = d->m_model->addVariable();
+    d->m_editor.setEnvironmentItems(d->m_model->userChanges());
     d->m_environmentView->setCurrentIndex(index);
-    d->m_environmentView->edit(index);
+    d->handleEditRequest(NameValueItemsWidget::Selection::Name);
 }
 
 void EnvironmentWidget::removeEnvironmentButtonClicked()
 {
     const QString &name = d->m_model->indexToVariable(d->m_environmentView->currentIndex());
     d->m_model->resetVariable(name);
+    d->m_editor.setEnvironmentItems(d->m_model->userChanges());
 }
 
 // unset in Merged Environment Mode means, unset if it comes from the base environment
@@ -463,6 +478,7 @@ void EnvironmentWidget::unsetEnvironmentButtonClicked()
         d->m_model->resetVariable(name);
     else
         d->m_model->unsetVariable(name);
+    d->m_editor.setEnvironmentItems(d->m_model->userChanges());
 }
 
 void EnvironmentWidget::amendPathList(Utils::NameValueItem::Operation op)
@@ -473,7 +489,7 @@ void EnvironmentWidget::amendPathList(Utils::NameValueItem::Operation op)
         return;
     Utils::NameValueItems changes = d->m_model->userChanges();
     changes.append({varName, dir.toUserOutput(), op});
-    d->m_model->setUserChanges(changes);
+    setUserChanges(changes);
 }
 
 void EnvironmentWidget::appendPathButtonClicked()
@@ -486,16 +502,6 @@ void EnvironmentWidget::prependPathButtonClicked()
     amendPathList(Utils::NameValueItem::Prepend);
 }
 
-void EnvironmentWidget::batchEditEnvironmentButtonClicked()
-{
-    const Utils::EnvironmentItems changes = d->m_model->userChanges();
-
-    const auto newChanges = Utils::EnvironmentDialog::getEnvironmentItems(this, changes);
-
-    if (newChanges)
-        d->m_model->setUserChanges(*newChanges);
-}
-
 void EnvironmentWidget::environmentCurrentIndexChanged(const QModelIndex &current)
 {
     if (current.isValid()) {

src/plugins/projectexplorer/environmentwidget.h

@@ -17,8 +17,6 @@ QT_FORWARD_DECLARE_CLASS(QModelIndex)
 
 namespace ProjectExplorer {
 
-class EnvironmentWidgetPrivate;
-
 class PROJECTEXPLORER_EXPORT EnvironmentWidget : public QWidget
 {
     Q_OBJECT
@@ -51,7 +49,6 @@ private:
     void unsetEnvironmentButtonClicked();
     void appendPathButtonClicked();
     void prependPathButtonClicked();
-    void batchEditEnvironmentButtonClicked();
     void environmentCurrentIndexChanged(const QModelIndex &current);
     void invalidateCurrentIndex();
     void updateSummaryText();
@@ -62,7 +59,8 @@ private:
     using PathListModifier = std::function<QString(const QString &oldList, const QString &newDir)>;
     void amendPathList(Utils::NameValueItem::Operation op);
 
-    const std::unique_ptr<EnvironmentWidgetPrivate> d;
+    class Private;
+    const std::unique_ptr<Private> d;
 };
 
 } // namespace ProjectExplorer

src/plugins/projectexplorer/kitaspects.cpp

@@ -1480,8 +1480,7 @@ private:
         if (HostOsInfo::isWindowsHost())
             changes.removeAll(forceMSVCEnglishItem());
 
-        return sorted(std::move(changes), [](const EnvironmentItem &lhs, const EnvironmentItem &rhs)
-        { return QString::localeAwareCompare(lhs.name, rhs.name) < 0; });
+        return changes;
     }
 
     void initMSVCOutputSwitch(QVBoxLayout *layout)