Allow creation of new folders from the Settings Dialog. #2897

allexzander · 2021-02-02T12:59:42Z

Signed-off-by: allexzander blackslayer4@gmail.com

Also fixed incorrect path for SubFolderInfo that was resulting in an inability to open encrypted subfolder and create a new folder inside it via the Settings Dialog's context menu.

er-vin · 2021-02-02T13:18:00Z

src/gui/foldercreationdialog.cpp

+
+    if (!QDir(_destination).mkdir(ui->newFolderNameEdit->text())) {
+        QMessageBox messageBox;
+        messageBox.critical(0, tr("Error"), tr("Could not create a folder! Check your write permissions."));


nullptr and not 0

That said you could have used this, since we want this message box to stack up on on top of the current dialog. Also you're calling a static method, you don't need an instance of QMessageBox for that.

@er-vin

nullptr and not 0

Indeed. Copy-pasted :)

er-vin · 2021-02-02T13:18:42Z

src/gui/foldercreationdialog.cpp

+    if (!QDir(_destination).mkdir(ui->newFolderNameEdit->text())) {
+        QMessageBox messageBox;
+        messageBox.critical(0, tr("Error"), tr("Could not create a folder! Check your write permissions."));
+        messageBox.setFixedSize(500,200);


This call is unneeded since you used the static method anyway it's not doing anything (well forcing just a fixed size on a message box which was never displayed).

er-vin · 2021-02-02T13:19:48Z

src/gui/accountsettings.cpp

@@ -333,6 +334,43 @@ void AccountSettings::slotEditCurrentIgnoredFiles()
    openIgnoredFilesDialog(f->path());
 }

+void AccountSettings::slotOpenMkFolderDialog()


nitpick: I'd spell "Make" fully not "Mk".

I think I'd even favor "Create" to "Make" (your call).

@er-vin I'll go with "Make" as "OpenCreate" really sounds for me like -Open and Create, instead of Open Create Folder Dialog" :)

er-vin · 2021-02-02T13:22:15Z

src/gui/accountsettings.cpp

+        return;
+    }
+
+    QString fileName;


I'd use an Immediately Invoked Lambda (IIL) here to have the filename as const. I'd even do the chop() call below in said lambda

@er-vin Although, I don't consider "having something as const" as a good reason for using lambda, I'm going to add this nitpick.

er-vin · 2021-02-02T13:23:32Z

src/gui/accountsettings.cpp

+
+    QString fileName;
+
+    if (classification == FolderStatusModel::RootFolder) {


Alternatively I guess we could have added support for FolderPathRole also on the RootFolder? This way we wouldn't need that if in the first place?

@er-vin Hmmmmmm, I don't know. This will introduce significantly more work and more QA + more chances for an error. Do we really need this sacrifice for the sake of not having if/else? I mean, if there were no other way, or if we've happened to become in need of using the same condition somewhere else, then I'd certainly vote for such refactoring, but, now, I see more evil than good if I do this.

Fair enough, let's not do this. It came mostly out of my sheer surprise that this role wasn't supported on said roots... feels like it was overlooked back then.

er-vin · 2021-02-02T13:28:57Z

src/gui/foldercreationdialog.cpp

+    delete ui;
+}
+
+void FolderCreationDialog::setDestination(const QString& destination)


Space before and not after &

er-vin · 2021-02-02T13:32:06Z

src/gui/foldercreationdialog.cpp

+
+void FolderCreationDialog::showEvent(QShowEvent *event)
+{
+    const auto newFolderFullPath = _destination + "/" + SuggestedFolderNamePrefix;


Wouldn't it make more sense to have that inside setDestination instead?

I mean we could always construct cases where the dialog is displayed first and setDestination called a bit later asynchronously.

@er-vin My intent was to make sure the dialog is always initialized when it is shown. But, I haven't made this intent clear. I will change the constructor and remove the setter now.

Note I didn't mind the setter. But OK let's go with a ctor parameter instead. You won't need show event anymore though, this can all be done in the ctor then.

@er-vin Alrighty then. Removed.

er-vin · 2021-02-02T13:34:08Z

src/gui/foldercreationdialog.cpp

+    QDialog::accept();
+}
+
+void FolderCreationDialog::on_newFolderNameEdit_textEdited(const QString &)


Maybe a matter of personal taste... but I think also a question of benefiting from compile time checks. I'd not rely on such connect via convention but have my slot named on my own convention and use and explicit connect in the ctor.

This way if it should break because something changed in the ui file (for instance) the compiler would catch it.

er-vin · 2021-02-02T13:34:42Z

src/gui/foldercreationdialog.cpp

@@ -0,0 +1,80 @@
+#include "foldercreationdialog.h"


copyright header missing

er-vin · 2021-02-02T13:34:51Z

src/gui/foldercreationdialog.h

@@ -0,0 +1,34 @@
+#ifndef FOLDERCREATIONDIALOG_H


copyright header missing

er-vin · 2021-02-02T14:44:36Z

src/gui/accountsettings.cpp

 {
-    const auto selected = _ui->_folderList->selectionModel()->currentIndex();
+    const QString fileName = [this] () {


Sorry, probably my fault for being unclear. :-/

What I meant if that the IIL would be nicely used for the part starting now from QString result. Using it all the way to selected, classification and friend goes overboard and hinder readability in my opinion. This is pattern which works only for short things where typically the ternary operator wouldn't be enough but not for full blown initialization of many intermediate variables.

er-vin · 2021-02-02T14:45:56Z

src/gui/accountsettings.cpp

-        FolderCreationDialog folderCreationDialog;
-        folderCreationDialog.setDestination(fileName);
-        folderCreationDialog.exec();
+        FolderCreationDialog(fileName).exec();


Note it is still on the stack, without parent and using exec. It should really go the other way around and be on the heap with a parent and using show + delete on close attribute.

@er-vin Changed

er-vin · 2021-02-02T14:47:33Z

src/gui/foldercreationdialog.cpp

@@ -10,30 +24,29 @@ namespace  {
  const QString SuggestedFolderNamePrefix = QObject::tr("New folder");
 };

-FolderCreationDialog::FolderCreationDialog(QWidget *parent) :
+FolderCreationDialog::FolderCreationDialog(const QString &destination, QWidget *parent) :


Somehow didn't notice it earlier but we seem to use mostly that convention in our code base (although it's not my favorite):

Foo::Foo(QWidget *parent) : Base(parent) , _member1(...) , _member2(...) {

er-vin · 2021-02-02T14:49:55Z

src/gui/foldercreationdialog.cpp

+
+void FolderCreationDialog::showEvent(QShowEvent *event)
+{
+    const auto newFolderFullPath = _destination + "/" + SuggestedFolderNamePrefix;


Note I didn't mind the setter. But OK let's go with a ctor parameter instead. You won't need show event anymore though, this can all be done in the ctor then.

er-vin · 2021-02-03T08:12:51Z

src/gui/accountsettings.cpp

+        return;
+    }
+
+    const QString fileName = [this, &selected, &classification] () {


Don't necessarily go change it, it is more a "by the way for next time" comment. So you know you can omit the parenthesis for lambdas not taking any parameter. It's a tiny bit less syntactic noise which is always welcome.

er-vin · 2021-02-03T08:16:58Z

src/gui/accountsettings.cpp

+            result = _model->data(selected, FolderStatusDelegate::FolderPathRole).toString();
+        }
+
+        if (!result.isEmpty()) {


Actually that if is redundant. Indeed if result.endsWith('/') is true then !result.isEmpty() is necessarily true. Also if you're concerned about performances all the implementations of endsWith have a fast path for the empty case so it's checked in some way.

er-vin · 2021-02-03T08:17:53Z

src/gui/accountsettings.cpp

+
+    if (!fileName.isEmpty()) {
+        const auto folderCreationDialog = new FolderCreationDialog(fileName, this);
+        folderCreationDialog->open();


nitpick: I'd open last.

Not that it'd matter in practice it's more of a readability thing IMHO.

er-vin · 2021-02-03T08:18:48Z

src/gui/foldercreationdialog.cpp

+#include <QMessageBox>
+
+namespace  {
+  const QString SuggestedFolderNamePrefix = QObject::tr("New folder");


Still needs to be moved as a local variable in the ctor.

er-vin · 2021-02-03T08:22:44Z

src/gui/foldercreationdialog.cpp

+    QDialog::accept();
+}
+
+void FolderCreationDialog::slotNewFolderNameEditTextEdited(const QString &)


Why have that parameter at all since we don't use it?

er-vin · 2021-02-03T12:37:40Z

src/gui/foldercreationdialog.cpp

+
+    connect(ui->newFolderNameEdit, &QLineEdit::textChanged, this, &FolderCreationDialog::slotNewFolderNameEditTextEdited);
+
+    const QString SuggestedFolderNamePrefix = QObject::tr("New folder");


please start the name with a lowercase s like any other variable.

er-vin · 2021-02-04T07:25:17Z

src/gui/accountsettings.cpp

+        if (classification == FolderStatusModel::RootFolder) {
+            const auto alias = _model->data(selected, FolderStatusDelegate::FolderAliasRole).toString();
+            const auto folderMan = FolderMan::instance();
+            if (const QPointer<Folder> folder = folderMan->folder(alias)) {


Either this is back or I missed the fact that the comment wasn't addressed but you don't need a QPointer here, a raw pointer is enough. There's no chance that object will be destroyed between the folder and path calls.

@er-vin I guess you missed it. Alright. This was copy-pasted from another method :(

@er-vin Was blindly copy-pasted from void AccountSettings::slotCustomContextMenuRequested(const QPoint &pos)

FYI I guess it made sense there at some point in time if one of slots connected to the actions were receiving that pointer, but not anymore since none of them use it (the folder is refetched in the slots anyway).

@er-vin Ok. Removed it. And, accidentally, cleared the history. But you can locate this change I am sure. This was the only one.

er-vin · 2021-02-04T07:27:11Z

You can start cleaning up the history BTW. I'm confident it'll be good to go on the next iteration. ;-)

allexzander · 2021-02-04T07:43:06Z

You can start cleaning up the history BTW. I'm confident it'll be good to go on the next iteration. ;-)

@er-vin I guess I can do it before the merge. Let's not be in hurry.

Signed-off-by: allexzander <blackslayer4@gmail.com>

nextcloud-desktop-bot · 2021-02-04T08:17:15Z

AppImage file: Nextcloud-PR-2897-81a090b362f10df8d208924c97148b0bcff135ad-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

allexzander requested review from er-vin, FlexW and camilasan February 2, 2021 12:59

er-vin reviewed Feb 2, 2021

View reviewed changes

allexzander requested a review from er-vin February 2, 2021 14:35

er-vin reviewed Feb 2, 2021

View reviewed changes

allexzander force-pushed the allow-creation-of-new-folders-from-the-settings-dialog branch from ccc9272 to 0dd9be6 Compare February 2, 2021 17:48

allexzander requested a review from er-vin February 2, 2021 17:49

er-vin reviewed Feb 3, 2021

View reviewed changes

allexzander force-pushed the allow-creation-of-new-folders-from-the-settings-dialog branch from ca478bf to 1c0c1be Compare February 3, 2021 11:51

allexzander requested a review from er-vin February 3, 2021 11:53

er-vin reviewed Feb 3, 2021

View reviewed changes

allexzander force-pushed the allow-creation-of-new-folders-from-the-settings-dialog branch 3 times, most recently from 261c9bb to eef9f68 Compare February 3, 2021 18:28

allexzander requested a review from er-vin February 3, 2021 19:53

er-vin reviewed Feb 4, 2021

View reviewed changes

er-vin approved these changes Feb 4, 2021

View reviewed changes

Allow creation of new folders from the Settings Dialog.

81a090b

Signed-off-by: allexzander <blackslayer4@gmail.com>

allexzander force-pushed the allow-creation-of-new-folders-from-the-settings-dialog branch from eae63dd to 81a090b Compare February 4, 2021 08:06

allexzander merged commit 2c8fa40 into master Feb 4, 2021

allexzander deleted the allow-creation-of-new-folders-from-the-settings-dialog branch February 4, 2021 08:28

allexzander mentioned this pull request Feb 4, 2021

Allow to create a new encrypted folder from the settings #468

Closed


		QString fileName;

		if (classification == FolderStatusModel::RootFolder) {


		connect(ui->newFolderNameEdit, &QLineEdit::textChanged, this, &FolderCreationDialog::slotNewFolderNameEditTextEdited);

		const QString SuggestedFolderNamePrefix = QObject::tr("New folder");

Allow creation of new folders from the Settings Dialog. #2897

Allow creation of new folders from the Settings Dialog. #2897

Conversation

allexzander commented Feb 2, 2021

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

allexzander Feb 2, 2021 • edited Loading

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

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

er-vin commented Feb 4, 2021

allexzander commented Feb 4, 2021

nextcloud-desktop-bot commented Feb 4, 2021

allexzander Feb 2, 2021 •

edited

Loading