Review Request 126831: WIP: Fix QFileDialog::openUrl() for remote files
David Faure
faure at kde.org
Sat Jan 23 11:12:04 UTC 2016
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126831/#review91468
-----------------------------------------------------------
src/filewidgets/kfilewidget.cpp (line 1462)
<https://git.reviewboard.kde.org/r/126831/#comment62519>
I think we should, yes, or even in the caller of setDirectory, if it's calling it with a non-directory URL. From the name it's pretty clear that we don't expect that to be called with a file.
- David Faure
On Jan. 20, 2016, 9 p.m., Alex Richardson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126831/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2016, 9 p.m.)
>
>
> Review request for KDE Frameworks.
>
>
> Bugs: 345253
> https://bugs.kde.org/show_bug.cgi?id=345253
>
>
> Repository: kio
>
>
> Description
> -------
>
> WIP: Fix QFileDialog::openUrl() for remote files
>
> QFileDialog doesn't make an attempt to determine whether remote URLs are files or directories so it [just passes the file URL to the platform file dialog](http://code.woboq.org/qt5/qtbase/src/widgets/dialogs/qfiledialog.cpp.html#_ZL17_qt_get_directoryRK4QUrl) which currently causes an error popup and a nonexistent directory to be selected
>
> I will clean up the code and remove qDebug() if this is the correct approach to fixing https://bugs.kde.org/show_bug.cgi?id=345253
> I prefer fixing this here instead of in KDEPlatformFileDialog as this means that other calls KFileWidget::setUrl() are also fixed.
>
> Should be a better solution than https://git.reviewboard.kde.org/r/126821/
>
>
> Problem is there are a lot of KIO::stat() calls going on which can be quite slow for remote files. Any suggestions how to fix them?
>
>
> Diffs
> -----
>
> src/filewidgets/kfilewidget.cpp 868928b3f6511c7890e4cf4ba09edc68048649e7
>
> Diff: https://git.reviewboard.kde.org/r/126831/diff/
>
>
> Testing
> -------
>
> kwrite sftp://user@host/home/user/foo.txt, press open button, directory is shown and foo.txt is selected.
>
>
> However, there are quite a lot of stats going on for a single QFileDialog::getOpenUrl(), probably we should optimize this.
> A total of 8 KIO::stat() calls are made when opening a file, I would expect only 2: original URL to find out if it is a directory or a file
> and then the selected file to check if it can be opened.
>
> Debug output:
> ```
> startDir QUrl("")
> for QUrl("") -> QUrl("file:///auto/homes/alr48") recentDirClass "" fileName ""
> startDir after getStartUrl() QUrl("file:///auto/homes/alr48") file name ""
> KIO::stat() QUrl("file:///auto/homes/alr48")
> stat of QUrl("file:///auto/homes/alr48") -> statRes true isDir true
> KIO::stat() QUrl("sftp://user@host/home/user/foo.txt")
> stat of QUrl("sftp://user@host/home/user/foo.txt") -> statRes true isDir false
> statJob -> directory QUrl("sftp://user@host/home/user") filename "foo.txt"
> File passed to KFileWidget::setUrl(), expected directory: QUrl("sftp://user@host/home/user/foo.txt")
> KIO::stat() QUrl("sftp://user@host/home/user/")
> stat of QUrl("sftp://user@host/home/user/") -> statRes true isDir true
> KIO::stat() QUrl("sftp://user@host/home/user/foo.txt")
> stat of QUrl("sftp://user@host/home/user/foo.txt") -> statRes true isDir false
> statJob -> directory QUrl("sftp://user@host/home/user") filename "foo.txt"
> File passed to KFileWidget::setUrl(), expected directory: QUrl("sftp://user@host/home/user/foo.txt")
> KIO::stat() QUrl("sftp://user@host/home/user/")
> stat of QUrl("sftp://user@host/home/user/") -> statRes true isDir true
> KIO::stat() QUrl("sftp://user@host/home/user/foo.txt")
> stat of QUrl("sftp://user@host/home/user/foo.txt") -> statRes true isDir false
> statJob -> directory QUrl("sftp://user@host/home/user") filename "foo.txt"
> File passed to KFileWidget::setUrl(), expected directory: QUrl("sftp://user@host/home/user/foo.txt")
> KIO::stat() QUrl("sftp://user@host/home/user/")
> stat of QUrl("sftp://user@host/home/user/") -> statRes true isDir true
> KIO::stat() QUrl("sftp://user@host/home/user/bar.txt")
> ```
>
>
> First StatJob stats QUrl("$HOME") and comes from the constructor with QUrl(), we can remove that by skipping it if url is empty.
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::KFileWidget kfilewidget.cpp 643 0x7fce663f1e05
> 3 KDEPlatformFileDialog::KDEPlatformFileDialog kdeplatformfiledialoghelper.cpp 96 0x7fce666b0678
> 4 KDEPlatformFileDialogHelper::KDEPlatformFileDialogHelper kdeplatformfiledialoghelper.cpp 201 0x7fce666b0dbd
> 5 KdePlatformTheme::createPlatformDialogHelper kdeplatformtheme.cpp 273 0x7fce666a63bc
> 6 QDialogPrivate::platformHelper qdialog.cpp 94 0x7fce759eaecc
> 7 QFileDialogPrivate::platformFileDialogHelper qfiledialog_p.h 112 0x7fce75a03f98
> 8 QFileDialogPrivate::init qfiledialog.cpp 2800 0x7fce759fa737
> 9 QFileDialog::QFileDialog qfiledialog.cpp 373 0x7fce759f169d
> 10 QFileDialog::getOpenFileUrls qfiledialog.cpp 2271 0x7fce759f7da3
> 11 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> 12 KWrite::qt_static_metacall moc_kwrite.cpp 131 0x41ea10
> 13 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 14 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 15 QAction::triggered moc_qaction.cpp 368 0x7fce7574ae7a
> 16 QAction::activate qaction.cpp 1163 0x7fce7574a28a
> 17 QAction::trigger qaction.h 177 0x7fce7574b821
> 18 QToolButton::nextCheckState qtoolbutton.cpp 954 0x7fce759a6bd3
> 19 QAbstractButtonPrivate::click qabstractbutton.cpp 515 0x7fce758aab1c
> 20 QAbstractButton::mouseReleaseEvent qabstractbutton.cpp 1131 0x7fce758ac08c
> ... <More>
> ```
>
>
> Second one for QUrl("sftp://user@host/home/user/foo.txt") has this backtrace:
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 4 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 5 QFileDialogPrivate::setDirectory_sys qfiledialog_p.h 368 0x7fce75a043a1
> 6 QFileDialog::setDirectoryUrl qfiledialog.cpp 976 0x7fce759f3cbb
> 7 QFileDialogPrivate::init qfiledialog.cpp 2806 0x7fce759fa7cd
> 8 QFileDialog::QFileDialog qfiledialog.cpp 373 0x7fce759f169d
> 9 QFileDialog::getOpenFileUrls qfiledialog.cpp 2271 0x7fce759f7da3
> 10 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> 11 KWrite::qt_static_metacall moc_kwrite.cpp 131 0x41ea10
> 12 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 13 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 14 QAction::triggered moc_qaction.cpp 368 0x7fce7574ae7a
> 15 QAction::activate qaction.cpp 1163 0x7fce7574a28a
> 16 QAction::trigger qaction.h 177 0x7fce7574b821
> 17 QToolButton::nextCheckState qtoolbutton.cpp 954 0x7fce759a6bd3
> 18 QAbstractButtonPrivate::click qabstractbutton.cpp 515 0x7fce758aab1c
> 19 QAbstractButton::mouseReleaseEvent qabstractbutton.cpp 1131 0x7fce758ac08c
> 20 QToolButton::mouseReleaseEvent qtoolbutton.cpp 609 0x7fce759a5839
> ... <More>
> ```
>
> next one on QUrl("sftp://user@host/home/user/") (caused by an url change here, possibly block signals on d->ops during setUrl()?):
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KFileWidgetPrivate::_k_enterUrl kfilewidget.cpp 1545 0x7fce663f759a
> 4 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 176 0x7fce663fe3e2
> 5 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 6 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 7 KUrlNavigator::urlChanged moc_kurlnavigator.cpp 299 0x7fce66439dcf
> 8 KUrlNavigator::setLocationUrl kurlnavigator.cpp 1080 0x7fce66438eb4
> 9 KFileWidgetPrivate::_k_urlEntered kfilewidget.cpp 1515 0x7fce663f73a7
> 10 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 175 0x7fce663fe3bf
> 11 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 12 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 13 KDirOperator::urlEntered moc_kdiroperator.cpp 586 0x7fce663de2b3
> 14 KDirOperator::setUrl kdiroperator.cpp 1039 0x7fce663d2599
> 15 KFileWidget::setUrl kfilewidget.cpp 1491 0x7fce663f7207
> 16 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 17 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 18 QFileDialogPrivate::setDirectory_sys qfiledialog_p.h 368 0x7fce75a043a1
> 19 QFileDialog::setDirectoryUrl qfiledialog.cpp 976 0x7fce759f3cbb
> 20 QFileDialogPrivate::init qfiledialog.cpp 2806 0x7fce759fa7cd
> ... <More>
> ```
>
> and again QUrl("sftp://user@host/home/user/foo.txt") (why is QFileDialogPrivate::restoreFromSettings() setting an URL if one was explicitly requested):
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 4 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 5 QFileDialogPrivate::setDirectory_sys qfiledialog_p.h 368 0x7fce75a043a1
> 6 QFileDialog::setDirectoryUrl qfiledialog.cpp 976 0x7fce759f3cbb
> 7 QFileDialogPrivate::restoreFromSettings qfiledialog.cpp 2716 0x7fce759f9c71
> 8 QFileDialogPrivate::init qfiledialog.cpp 2812 0x7fce759fa817
> 9 QFileDialog::QFileDialog qfiledialog.cpp 373 0x7fce759f169d
> 10 QFileDialog::getOpenFileUrls qfiledialog.cpp 2271 0x7fce759f7da3
> 11 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> 12 KWrite::qt_static_metacall moc_kwrite.cpp 131 0x41ea10
> 13 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 14 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 15 QAction::triggered moc_qaction.cpp 368 0x7fce7574ae7a
> 16 QAction::activate qaction.cpp 1163 0x7fce7574a28a
> 17 QAction::trigger qaction.h 177 0x7fce7574b821
> 18 QToolButton::nextCheckState qtoolbutton.cpp 954 0x7fce759a6bd3
> 19 QAbstractButtonPrivate::click qabstractbutton.cpp 515 0x7fce758aab1c
> 20 QAbstractButton::mouseReleaseEvent qabstractbutton.cpp 1131 0x7fce758ac08c
> ... <More>
> ```
>
> which seems to cause another call to KFileWidget::setUrl() (block signals?). Stats QUrl("sftp://user@host/home/user/") as before:
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KFileWidgetPrivate::_k_enterUrl kfilewidget.cpp 1545 0x7fce663f759a
> 4 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 176 0x7fce663fe3e2
> 5 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 6 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 7 KUrlNavigator::urlChanged moc_kurlnavigator.cpp 299 0x7fce66439dcf
> 8 KUrlNavigator::setLocationUrl kurlnavigator.cpp 1080 0x7fce66438eb4
> 9 KFileWidgetPrivate::_k_urlEntered kfilewidget.cpp 1515 0x7fce663f73a7
> 10 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 175 0x7fce663fe3bf
> 11 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 12 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 13 KDirOperator::urlEntered moc_kdiroperator.cpp 586 0x7fce663de2b3
> 14 KDirOperator::setUrl kdiroperator.cpp 1039 0x7fce663d2599
> 15 KFileWidget::setUrl kfilewidget.cpp 1491 0x7fce663f7207
> 16 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 17 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 18 QFileDialogPrivate::setDirectory_sys qfiledialog_p.h 368 0x7fce75a043a1
> 19 QFileDialog::setDirectoryUrl qfiledialog.cpp 976 0x7fce759f3cbb
> 20 QFileDialogPrivate::restoreFromSettings qfiledialog.cpp 2716 0x7fce759f9c71
> 21 QFileDialogPrivate::init qfiledialog.cpp 2812 0x7fce759fa817
> 22 QFileDialog::QFileDialog qfiledialog.cpp 373 0x7fce759f169d
> 23 QFileDialog::getOpenFileUrls qfiledialog.cpp 2271 0x7fce759f7da3
> 24 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> 25 KWrite::qt_static_metacall moc_kwrite.cpp 131 0x41ea10
> ... <More>
> ```
>
> And now KDEPlatformFileDialogHelper::show() sets the URL to QUrl("sftp://user@host/home/user/foo.txt") again
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 4 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 5 KDEPlatformFileDialogHelper::initializeDialog kdeplatformfiledialoghelper.cpp 238 0x7fce666b147f
> 6 KDEPlatformFileDialogHelper::show kdeplatformfiledialoghelper.cpp 310 0x7fce666b1a36
> 7 QDialogPrivate::setNativeDialogVisible qdialog.cpp 129 0x7fce759eb10c
> 8 QFileDialog::setVisible qfiledialog.cpp 842 0x7fce759f3571
> 9 QWidget::show qwidget.cpp 7728 0x7fce757a8921
> 10 QDialog::exec qdialog.cpp 533 0x7fce759eb8c7
> 11 QFileDialog::getOpenFileUrls qfiledialog.cpp 2275 0x7fce759f7e08
> 12 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> 13 KWrite::qt_static_metacall moc_kwrite.cpp 131 0x41ea10
> 14 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 15 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 16 QAction::triggered moc_qaction.cpp 368 0x7fce7574ae7a
> 17 QAction::activate qaction.cpp 1163 0x7fce7574a28a
> 18 QAction::trigger qaction.h 177 0x7fce7574b821
> 19 QToolButton::nextCheckState qtoolbutton.cpp 954 0x7fce759a6bd3
> 20 QAbstractButtonPrivate::click qabstractbutton.cpp 515 0x7fce758aab1c
> ... <More>
> ```
>
> causing another call (possibly fixable by blocking signals). Stats QUrl("sftp://user@host/home/user/") again:
>
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::setUrl kfilewidget.cpp 1476 0x7fce663f6e6c
> 3 KFileWidgetPrivate::_k_enterUrl kfilewidget.cpp 1545 0x7fce663f759a
> 4 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 176 0x7fce663fe3e2
> 5 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 6 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 7 KUrlNavigator::urlChanged moc_kurlnavigator.cpp 299 0x7fce66439dcf
> 8 KUrlNavigator::setLocationUrl kurlnavigator.cpp 1080 0x7fce66438eb4
> 9 KFileWidgetPrivate::_k_urlEntered kfilewidget.cpp 1515 0x7fce663f73a7
> 10 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 175 0x7fce663fe3bf
> 11 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 12 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 13 KDirOperator::urlEntered moc_kdiroperator.cpp 586 0x7fce663de2b3
> 14 KDirOperator::setUrl kdiroperator.cpp 1039 0x7fce663d2599
> 15 KFileWidget::setUrl kfilewidget.cpp 1491 0x7fce663f7207
> 16 KDEPlatformFileDialog::setDirectory kdeplatformfiledialoghelper.cpp 189 0x7fce666b0ce0
> 17 KDEPlatformFileDialogHelper::setDirectory kdeplatformfiledialoghelper.cpp 344 0x7fce666b1c06
> 18 KDEPlatformFileDialogHelper::initializeDialog kdeplatformfiledialoghelper.cpp 238 0x7fce666b147f
> 19 KDEPlatformFileDialogHelper::show kdeplatformfiledialoghelper.cpp 310 0x7fce666b1a36
> 20 QDialogPrivate::setNativeDialogVisible qdialog.cpp 129 0x7fce759eb10c
> 21 QFileDialog::setVisible qfiledialog.cpp 842 0x7fce759f3571
> 22 QWidget::show qwidget.cpp 7728 0x7fce757a8921
> 23 QDialog::exec qdialog.cpp 533 0x7fce759eb8c7
> 24 QFileDialog::getOpenFileUrls qfiledialog.cpp 2275 0x7fce759f7e08
> 25 KWrite::slotOpen kwrite.cpp 250 0x41a0c1
> ... <More>
> ```
>
>
> Finally, when selecting a new file (but this one is required as we need to check whether the new file can be opened)
> Stats QUrl("sftp://user@host/home/user/bar.txt")
> ```
> 1 KIO::stat statjob.cpp 180 0x7fce732928c8
> 2 KFileWidget::slotOk kfilewidget.cpp 993 0x7fce663f4005
> 3 KFileWidget::qt_static_metacall moc_kfilewidget.cpp 171 0x7fce663fe357
> 4 QMetaObject::activate qobject.cpp 3730 0x7fce7490fc54
> 5 QMetaObject::activate qobject.cpp 3595 0x7fce7490f442
> 6 QAbstractButton::clicked moc_qabstractbutton.cpp 306 0x7fce75bec92a
> 7 QAbstractButtonPrivate::emitClicked qabstractbutton.cpp 533 0x7fce758aac1f
> 8 QAbstractButtonPrivate::click qabstractbutton.cpp 526 0x7fce758aabae
> 9 QAbstractButton::mouseReleaseEvent qabstractbutton.cpp 1131 0x7fce758ac08c
> 10 QWidget::event qwidget.cpp 8738 0x7fce757ab018
> 11 QAbstractButton::event qabstractbutton.cpp 1088 0x7fce758abeca
> 12 QPushButton::event qpushbutton.cpp 673 0x7fce7596c071
> 13 QApplicationPrivate::notify_helper qapplication.cpp 3712 0x7fce7575b278
> 14 QApplication::notify qapplication.cpp 3270 0x7fce75758fdf
> 15 QCoreApplication::notifyInternal2 qcoreapplication.cpp 1013 0x7fce748cf642
> 16 QCoreApplication::sendSpontaneousEvent qcoreapplication.h 230 0x7fce7575e226
> 17 QApplicationPrivate::sendMouseEvent qapplication.cpp 2765 0x7fce75757997
> 18 QWidgetWindow::handleMouseEvent qwidgetwindow.cpp 554 0x7fce757d7a99
> 19 QWidgetWindow::event qwidgetwindow.cpp 210 0x7fce757d671f
> 20 QApplicationPrivate::notify_helper qapplication.cpp 3712 0x7fce7575b278
> ... <More>
> ```
>
>
> Thanks,
>
> Alex Richardson
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160123/bb18e820/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list