Add a widget for editing file names #5375

zeule · 2016-06-15T16:07:45Z

The combinations of QLineEdit or QComboBox with a browse button for file system paths editing most likely results in selecting files or dirs via the button and a file dialog. However, typing with auto completion might provide a better alternative. This PR implements it in the standard for Qt way, using QCompleter and QFileSystemModel.

The PR adds FileSystemPathLineEdit and FileSystemPathComboEdit classes (share most of the implementation) that provides widgets for editing file names with QLineEdit or QComboBox (in editable mode). The widgets also contain the "Browse" button, which launches a file dialog. Properties of the dialog (title, file filter, etc.) are specified as FileSystemPathEdit class properties.

The class also uses Utils::Fs::fromNativePath() and Utils::Fs::toNativePath() internally thus eliminating the need to use them with each widget explicitly.

Screenshots show example for Add torrent dialog:

glassez · 2016-06-15T17:11:18Z

src/gui/fspathedit.cpp

+#endif
+}
+
+#include "moc_fspathedit.cpp"


Why do you include this file?

Due to Q_PRIVATE_SLOT(d_func(), void browseActionTriggered()) in the FileSystemPathEdit class.

Q_PRIVATE_SLOT replaced with lambda.

Chocobo1 · 2016-06-16T04:41:12Z

Is it a good idea to move the new code to its own folder gui/fspathedit? There are already 99 files + dirs in gui...

glassez · 2016-06-16T05:24:01Z

Is it a good idea to move the new code to its own folder gui/fspathedit?

IMO, it's good.

zeule · 2016-06-16T22:06:24Z

Is it a good idea to move the new code to its own folder gui/fspathedit?

Maybe gui/widgets is a better name?

glassez · 2016-06-17T09:29:44Z

Maybe gui/widgets is a better name?

Then we should move all other widgets there.

zeule · 2016-06-17T09:32:30Z

... and do the same for gui/views, gui/models, and, gui/dialogs.

glassez · 2017-04-22T06:42:41Z

src/gui/addnewtorrentdialog.cpp

    size_string += ")";
    ui->size_lbl->setText(size_string);
 }

-void AddNewTorrentDialog::onSavePathChanged(int index)
+void AddNewTorrentDialog::onSavePathChanged(const QString &/*text*/)


You can connect slots with fewer parameters than signal has.

Excuse me, I don't understand what did you mean here.

I mean you can turn it to void AddNewTorrentDialog::onSavePathChanged() unless you need QString param.

Ah, thanks.

glassez · 2017-04-22T06:44:54Z

src/gui/addnewtorrentdialog.cpp

@@ -658,6 +623,15 @@ void AddNewTorrentDialog::accept()
        BitTorrent::Session::instance()->addTorrent(m_torrentInfo, params);

    m_torrentGuard->markAsAddedToSession();
+
+    // add selected save path to the save path history


I'm confused about it outside of class. Why not do this in the destructor?

glassez

I have not delved much into the logic (leave that for the author), but the code structure looks good. I left some comments. This is a very useful widget. I hope you will expand it in the future to support relative paths too.

glassez · 2017-04-22T06:46:49Z

src/gui/fspathedit.cpp

+#include <QToolButton>
+
+#include "fspathedit_p.h"
+#include "base/utils/fs.h"


Move one line up.

glassez · 2017-04-22T06:50:22Z

src/gui/fspathedit.cpp

+        break;
+    case FileSystemPathEdit::Mode::DirectoryOpen:
+    case FileSystemPathEdit::Mode::DirectorySave:
+        selectedPath = QFileDialog::getExistingDirectory(q, dialogCaptionOrDefault(), directory, QFileDialog::DontResolveSymlinks


I would split this line on some param boundary.

glassez · 2017-04-22T06:52:22Z

src/gui/fspathedit.cpp

+    connect(d->m_browseAction, SIGNAL(triggered(bool)), this, SLOT(browseActionTriggered()));
+}
+
+FileSystemPathEdit::~FileSystemPathEdit()


Maybe it would be better to omit this body and use = default in header?

glassez · 2017-04-22T06:53:12Z

src/gui/fspathedit.cpp

+    Q_D(FileSystemPathEdit);
+    d->m_fileNameFilter = val;
+
+#if 0 // QFileSystemModel applies name filters to directories too.


Move comment to the next line.

glassez · 2017-04-22T06:54:29Z

src/gui/fspathedit.cpp

+    Q_D(FileSystemPathEdit);
+    QString newPath = selectedPath();
+    if (newPath != d->m_lastSignaledPath) {
+        Q_EMIT selectedPathChanged(newPath);


We support emit keyword, isn't it?

glassez · 2017-04-22T07:01:06Z

src/gui/fspathedit.h

+
+    enum class Mode
+    {
+        FileOpen,                                 //!< Working mode for opening files, shows open dialog


Would you like to put these comments closer to the enum items?

glassez · 2017-04-22T07:04:29Z

src/gui/fspathedit.h

+protected slots:
+    void onPathEdited();
+
+signals:


Public members should appear before protected/private. Signals are public.
Also protected/private slots is less protected/private than other protected/private members.

glassez · 2017-04-22T07:07:42Z

src/gui/fspathedit.h

+    Q_DISABLE_COPY(FileSystemPathEdit)
+    class FileSystemPathEditPrivate;
+    Q_DECLARE_PRIVATE(FileSystemPathEdit)
+    Q_PRIVATE_SLOT(d_func(), void browseActionTriggered())


Qt has a naming convention for private slots (don't remember exactly how it looks).

Replaced with lambda.

glassez · 2017-04-22T07:09:29Z

src/gui/fspathedit.h

+    class FileSystemPathEditPrivate;
+    Q_DECLARE_PRIVATE(FileSystemPathEdit)
+    Q_PRIVATE_SLOT(d_func(), void browseActionTriggered())
+    QScopedPointer<FileSystemPathEditPrivate> const d_ptr;


Why do you use QScopedPointers in these places? Why not usual Qt ownership?

The private class does not inherit QObject.

glassez · 2017-04-22T07:11:03Z

src/gui/fspathedit_p.cpp

+    , m_completer {new QCompleter(this)}
+    , m_browseAction {nullptr}
+{
+#if !defined QBT_USES_QT5 && defined Q_OS_WIN


QBT_USES_QT5 is outdated.

glassez · 2017-04-22T07:27:52Z

src/gui/fspathedit_p.h

+    {
+    public:
+        virtual ~FileEditorWithCompletion() = default;
+        virtual void completeDirectoriesOnly(bool completeDirsOnly) = 0;


I wonder why C++ still does not have abstract keyword for this purpose?

zeule · 2017-04-24T15:09:27Z

When I update this PR now, do you think I should move its files into gui/widgets or not? Should I submit another PR instead to rearrange all files (gui/models, gui/views, gui/widgets) and break basically all other PRs? @qbittorrent/frequent-contributors?

glassez · 2017-04-24T15:42:03Z

When I update this PR now, do you think I should move its files into gui/widgets or not?

Not here, exactly.

Chocobo1 · 2017-04-24T15:46:29Z

When I update this PR now, do you think I should move its files into gui/widgets or not? Should I submit another PR instead to rearrange all files (gui/models, gui/views, gui/widgets) and break basically all other PRs?

Or you can wait till most existing PRs are merged, we are merging fast recently...

zeule · 2017-04-24T15:52:04Z

OK, thank you for your input! Then for this one the files will be kept at the same place.

zeule · 2017-05-05T12:22:20Z

PR updated:

Fixed filter string for IP filters in options dialog (@Chocobo1)
Widget classes migrated to Qt5: removed #ifdefs and new signal/slot syntax is used. Private slot is replaced with lambda.
Various formatting issues were corrected.
Button icon is fetched from QStyle now.

glassez · 2017-05-05T19:35:39Z

src/gui/fspathedit.h

+
+    virtual void clear() = 0;
+
+	signals:


Broken indentation. And next line too.

glassez · 2017-05-05T19:38:57Z

src/gui/fspathedit_p.cpp

+    m_completer->setCompletionMode(QCompleter::PopupCompletion);
+    setCompleter(m_completer);
+
+    connect(m_completerModel, SIGNAL(directoryLoaded(QString)), this, SLOT(showCompletionPopup()));


Please, use Qt5 style here too.

Chocobo1 · 2017-05-06T14:43:30Z

src/gui/fspathedit.cpp

+    m_browseAction->setIconText(browseButtonBriefText.tr());
+    m_browseAction->setText(browseButtonFullText.tr());
+    m_browseAction->setToolTip(browseButtonFullText.tr().remove(QLatin1Char('&')));
+    m_browseAction->setShortcut({Qt::CTRL + Qt::Key_B});


curly braces required?

Chocobo1 · 2017-05-06T14:45:11Z

src/gui/fspathedit.cpp

+
+void FileSystemPathEdit::FileSystemPathEditPrivate::browseActionTriggered()
+{
+    QString selectedPath;


I would move this to just above switch expression.

Chocobo1 · 2017-05-06T14:46:41Z

src/gui/fspathedit.cpp

+        pixmap = QStyle::SP_DirOpenIcon;
+        showDirsOnly = true;
+        break;
+    }


I'm not sure if a default case would make sense... but it doesn't hurt to add one?

Oh, yes! default with throw was mistakenly forgotten here.

Chocobo1 · 2017-05-06T14:49:00Z

src/gui/fspathedit.cpp

+    Q_D(FileSystemPathEdit);
+    d->m_fileNameFilter = val;
+
+#if 0


#if 0 for comments? /**/ does not suffice?

Yes, #if 0 is perfect for commenting out a block of code which itself contains comments inside.

Chocobo1 · 2017-05-06T14:54:02Z

src/gui/fspathedit_p.h

+#include <QFileSystemModel>
+#include <QKeyEvent>
+#include <QLineEdit>
+#include <QMenu>


forward declare as much as possible?

I don't think it is important here, because this is a private header, included from only single other place.

Chocobo1 · 2017-05-06T14:57:01Z

src/gui/optionsdlg.cpp

+
+    m_ui->textFilterPath->setDialogCaption(tr("Choose an IP filter file"));
+    m_ui->textFilterPath->setFileNameFilter(tr("All supported filters")
+        + QString(" (*.dat *.p2p *.p2b);;.dat (*.dat);;.p2p (*.p2p);;.p2b (*.p2b)"));


QString -> QLatin1String ?

zeule · 2017-05-06T21:29:36Z

Comments from reviewers are addressed.

glassez

I have commented something, but it's optional, you don't have to fix anything anymore.

glassez · 2017-05-08T07:04:38Z

src/gui/addnewtorrentdialog.cpp

@@ -34,6 +34,7 @@
 #include <QUrl>
 #include <QMenu>
 #include <QFileDialog>
+#include <QPushButton>


Is this include needed?

Yes, it is needed. Otherwise:

../../src/gui/addnewtorrentdialog.cpp:148:45: error: invalid use of incomplete type ‘class QPushButton’ ui->buttonBox->button(QDialogButtonBox::Ok)->setFocus(); ^~

glassez · 2017-05-08T07:07:58Z

src/gui/fspathedit.cpp

+    QString m_dialogCaption;
+};
+
+FileSystemPathEdit::FileSystemPathEditPrivate::FileSystemPathEditPrivate(FileSystemPathEdit *q,


IMO, it would be better if you insert line break before first param (instead of second one).

glassez · 2017-05-08T07:12:47Z

src/gui/optionsdlg.cpp

@@ -357,6 +353,25 @@ OptionsDialog::OptionsDialog(QWidget *parent)
    m_ui->advPageLayout->addWidget(advancedSettings);
    connect(advancedSettings, SIGNAL(settingsChanged()), this, SLOT(enableApplyButton()));

+    m_ui->textFileLogPath->setDialogCaption(tr("Choose a save directory"));


Can't you set these properties in designer?

Nope, I do not export the widget to Designer completely. Probably, with only a single custom widget in the app it does not make sense.

zeule · 2017-05-08T08:04:33Z

PR updated: taken two last suggestions from the reviewers. @sledgehammer999, do you want to say anything?

glassez

Thank you.

zeule · 2017-05-08T16:45:35Z

@sledgehammer999, should I count a week? Are you interested in this?

zeule · 2017-05-15T07:33:09Z

OK, a week has passed, merging.

Chocobo1 · 2017-05-17T05:41:03Z

Might be related to this PR, I got the following when opening options dialog:

QObject::connect: No such signal FileSystemPathEdit::textChanged(QString)
QObject::connect:  (sender name:   'textFilterPath')
QObject::connect:  (receiver name: 'OptionsDialog')

glassez · 2017-05-17T06:35:54Z

Damn, we should prohibit using old connect syntax in new code to prevent this bugs in future.

zeule · 2017-05-17T08:26:54Z

I'm sorry for that. See #6795, please.

Chocobo1 · 2017-05-17T16:40:51Z

src/gui/optionsdlg.ui

-    <width>779</width>
-    <height>591</height>
+    <width>1116</width>
+    <height>838</height>


This is causing the default dialog size to be large than ever, is it intentional?
Do you mind we switch back?

hrastnik · 2017-05-17T20:09:47Z

Did you consider the cases where the users manually writes invalid file paths?

zeule · 2017-05-18T06:17:21Z

@Chocobo1: sure
@hrastnik: no, I did not.

glassez reviewed Jun 15, 2016
View reviewed changes

zeule mentioned this pull request Dec 5, 2016

Add editable save path option to "Add New Torrent" dialog #5741

Closed

thalieht mentioned this pull request Dec 8, 2016

Editable save to in torrent dialog. #1153

Closed

thalieht mentioned this pull request Jan 25, 2017

[Request] Make path editing possible and root folder re-naming more productive #5669

Closed

glassez reviewed Apr 22, 2017

View reviewed changes

glassez requested changes Apr 22, 2017

View reviewed changes

glassez reviewed Apr 22, 2017

View reviewed changes

zeule requested review from glassez and Chocobo1 May 5, 2017 16:03

glassez requested changes May 5, 2017

View reviewed changes

Chocobo1 reviewed May 6, 2017

View reviewed changes

glassez previously approved these changes May 8, 2017

View reviewed changes

zeule dismissed glassez’s stale review via b484b0c May 8, 2017 08:02

zeule requested review from sledgehammer999, glassez and Chocobo1 May 8, 2017 08:07

zeule added 2 commits May 8, 2017 10:08

Add widget for editing filesystem paths

30081e0

Use file path edit widgets in options and add torrent dialog

7320ac1

glassez approved these changes May 8, 2017

View reviewed changes

Chocobo1 approved these changes May 8, 2017

View reviewed changes

glassez mentioned this pull request May 10, 2017

Steps for version 3.4 release #6767

Closed

zeule merged commit 954f05b into qbittorrent:master May 15, 2017

zeule deleted the filename-edit-widget branch May 15, 2017 07:33

Chocobo1 reviewed May 17, 2017

View reviewed changes

Chocobo1 mentioned this pull request May 18, 2017

[Quick Fix] Cleanup options dialog #6802

Merged

zeule mentioned this pull request May 23, 2017

Add visual feedback for wrong paths in FileSystemPathEdit #6820

Merged

thalieht mentioned this pull request Aug 4, 2017

[Wishlist] Editable “Save At” field in new torrent options dialog #7202

Closed

Add a widget for editing file names #5375

Add a widget for editing file names #5375

Conversation

zeule commented Jun 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Jun 16, 2016

glassez commented Jun 16, 2016

zeule commented Jun 16, 2016

glassez commented Jun 17, 2016

zeule commented Jun 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented Apr 24, 2017

glassez commented Apr 24, 2017

Chocobo1 commented Apr 24, 2017

zeule commented Apr 24, 2017

zeule commented May 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented May 6, 2017

glassez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeule commented May 8, 2017

glassez left a comment

Choose a reason for hiding this comment

zeule commented May 8, 2017

zeule commented May 15, 2017

Chocobo1 commented May 17, 2017

glassez commented May 17, 2017

zeule commented May 17, 2017

Choose a reason for hiding this comment

hrastnik commented May 17, 2017

zeule commented May 18, 2017

zeule commented May 5, 2017 •

edited