Commit bbf021c6 authored by Igor Kushnir's avatar Igor Kushnir
Browse files

Actually show empty-pattern project filter error

I have discovered this bug thanks to the following GCC warning:
warning: loop variable ‘filter’ of type ‘const KDevelop::Filter&’ binds to a temporary constructed from type ‘const KDevelop::SerializedFilter’ [-Wrange-loop-construct]
  162 |     for (const Filter& filter : filters) {
      |                        ^~~~~~
note: use non-reference type ‘const KDevelop::Filter’ to make the copy explicit or ‘const KDevelop::SerializedFilter&’ to prevent copying

Filter::Filter(const SerializedFilter& filter) prepends "*/" to an empty
pattern. So, aside from the inefficiency of constructing a temporary
Filter object, the bug prevented displaying the empty-pattern error.

The project filter validation was implemented in
59ed3308. The very next commit on the
next day - a15b683d - changed the type
of FilterModel::filters() from Filters to SerializedFilters and
introduced the bug. So the empty-pattern error must have been seen only
by its implementer during development testing.

Simply changing the type of the loop variable from Filter& to auto&
shows the empty-pattern error as soon as the user adds a new filter.
Prevent this annoyance by not rechecking filters when a row is inserted
(and when a row is moved, for good measure, to skip useless work).
parent 326bdd63
Pipeline #128553 passed with stage
in 33 minutes and 22 seconds
......@@ -61,11 +61,19 @@ ProjectFilterConfigPage::ProjectFilterConfigPage(ProjectFilterProvider* provider
connect(m_ui->filters->selectionModel(), &QItemSelectionModel::currentChanged,
this, &ProjectFilterConfigPage::selectionChanged);
connect(this, &ProjectFilterConfigPage::changed, this, &ProjectFilterConfigPage::selectionChanged);
connect(m_model, &FilterModel::dataChanged, this, &ProjectFilterConfigPage::emitChanged);
connect(m_model, &FilterModel::rowsInserted, this, &ProjectFilterConfigPage::emitChanged);
connect(m_model, &FilterModel::rowsRemoved, this, &ProjectFilterConfigPage::emitChanged);
connect(m_model, &FilterModel::modelReset, this, &ProjectFilterConfigPage::emitChanged);
connect(m_model, &FilterModel::rowsMoved, this, &ProjectFilterConfigPage::emitChanged);
connect(m_model, &FilterModel::modelReset, this, &ProjectFilterConfigPage::checkFiltersAndEmitChanged);
connect(m_model, &FilterModel::dataChanged, this, &ProjectFilterConfigPage::checkFiltersAndEmitChanged);
connect(m_model, &FilterModel::rowsRemoved, this, &ProjectFilterConfigPage::checkFiltersAndEmitChanged);
// Don't recheck filters when a row is:
// 1. Inserted - because existing errors remain. And if there were no errors, a new
// empty-pattern error would be shown - annoyingly and uselessly - on each insertion, before
// the user had a chance to enter a new pattern.
// 2. Moved - because neither existing errors disappear nor new ones appear after this operation.
// When there are multiple errors, the row order determines which error message is shown.
// But the message choice in this corner case is not important enough to recheck all filters.
connect(m_model, &FilterModel::rowsInserted, this, &ProjectFilterConfigPage::changed);
connect(m_model, &FilterModel::rowsMoved, this, &ProjectFilterConfigPage::changed);
connect(m_ui->add, &QPushButton::clicked, this, &ProjectFilterConfigPage::add);
connect(m_ui->remove, &QPushButton::clicked, this, &ProjectFilterConfigPage::remove);
......@@ -159,12 +167,11 @@ void ProjectFilterConfigPage::checkFilters()
// check for errors, only show one error at once
QString errorText;
const auto filters = m_model->filters();
for (const Filter& filter : filters) {
const QString &pattern = filter.pattern.pattern();
if (pattern.isEmpty()) {
for (const auto& filter : filters) {
if (filter.pattern.isEmpty()) {
errorText = i18n("A filter with an empty pattern will match all items. Use <code>\"*\"</code> to make this explicit.");
} else if (pattern.endsWith(QLatin1Char('/')) && filter.targets == Filter::Files) {
} else if (filter.pattern.endsWith(QLatin1Char('/')) && filter.targets == Filter::Files) {
errorText = i18n("A filter ending on <code>\"/\"</code> can never match a file.");
......@@ -179,7 +186,7 @@ void ProjectFilterConfigPage::checkFilters()
void ProjectFilterConfigPage::emitChanged()
void ProjectFilterConfigPage::checkFiltersAndEmitChanged()
......@@ -43,7 +43,7 @@ private Q_SLOTS:
void moveUp();
void moveDown();
void selectionChanged();
void emitChanged();
void checkFiltersAndEmitChanged();
public Q_SLOTS:
void apply() override;
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment