D8056: Improve usability of "Open With" dialog by adding option to filter the application tree

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171029/47e22b7a/attachment-0001.html>