dfaure added a comment.
Looks good, just some coding style issues. INLINE COMMENTS > kopenwithdialog.cpp:195 > d->fillNode(QString(), d->root); > + for (int i=0; i<rowCount(); i++) { > + fetchAll(index(i,0)); spaces around '=' and '<' I would put rowCount() into a local var, the compiler can't optimize away that call. > kopenwithdialog.cpp:196 > + for (int i=0; i<rowCount(); i++) { > + fetchAll(index(i,0)); > + } space after comma > kopenwithdialog.cpp:274 > + } > + return; > +} remove useless statement > kopenwithdialog.cpp:382 > + QSortFilterProxyModel(parent) > + { > + } weird indentation, please indent these to column 0 > kopenwithdialog.cpp:398 > + > + //Show the non-leaf node also if the regexp matches one one of its > children > + int rows = sourceModel()->rowCount(index); Ah. Filipe and I implemented recursive filtering in QSortFilterProxyModel but that's only in Qt 5.10. Too bad ;) We'll have to keep this for now then (easier here since the data doesn't change, anyway) > kopenwithdialog.cpp:440 > > - d->appModel = qobject_cast<KApplicationModel *>(model); > + d->appModel = qobject_cast<KApplicationModel > *>(static_cast<QTreeViewProxyFilter *>(model)->sourceModel()); > if (d->appModel) { Urgh. Why don't we change this method to be setModels(QSortFilterProxyModel *, KApplicationModel *) to avoid all these casts? > kopenwithdialog.cpp:451 > QModelIndex index = selectionModel()->currentIndex(); > + index = static_cast<QTreeViewProxyFilter > *>(model())->mapToSource(index); > return d->appModel->isDirectory(index); ... and keep a QSFPM * member variable to avoid these casts here. > kopenwithdialog.cpp:479 > + if (indexes.count() == 1) { > + if(!d->appModel->isDirectory(indexes.at(0))) { > + QString exec = d->appModel->execFor(indexes.at(0)); space after if > kopenwithdialog.cpp:819 > + // otherwise changing text but hitting the same result clears curService > + bool selectionEmpty = !( d->view->currentIndex().row() >= 0); > + if (d->curService && selectionEmpty) { I think this would be more readable as const bool selectionEmpty = !d->view->currentIndex().isValid(); > kopenwithdialog.cpp:826 > + //Update the filter regexp with the new text in the lineedit > + static_cast<QTreeViewProxyFilter > *>(d->view->model())->setFilterRegExp(QRegExp(d->edit->text(), > + > Qt::CaseInsensitive, QRegExp::FixedString)); Please use QRegularExpression; QRegExp is deprecated. Although, if this is about FixedString, why not just use setFilterFixedString? Regexps are slow, better avoid them when not necessary. > kopenwithdialog.cpp:830 > + //Expand all the nodes when the search string is 3 characters long > + //If the search string doesn't match anything there will be not nodes to > expand > + if (d->edit->text().size() > 2) { s/not/no/ ? > kopenwithdialog.h:137 > private: > + bool eventFilter(QObject *object, QEvent *event); > + add "override" at the end REPOSITORY R241 KIO BRANCH openwithdialog-filter-app-tree REVISION DETAIL https://phabricator.kde.org/D8056 To: simgunz, dfaure, #frameworks, #vdg, ngraham Cc: subdiff, fabianr, abetts, ngraham, alexeymin, #frameworks