[Differential] [Updated] D2120: Implement a custom AddDialog

[elvisangelaccio (Elvis Angelaccio)]

elvisangelaccio added a comment.


  The dialog works fine, I only have minor comments!

INLINE COMMENTS

> CMakeLists.txt:17
>      mimetypetest.cpp
> -    LINK_LIBRARIES kerfuffle Qt5::Test
> +    LINK_LIBRARIES kerfuffle Qt5::Test KF5::KIOFileWidgets
>      NAME_PREFIX kerfuffle-)

Think links all the executables to KioFileWidgets. This could result in a slower build.
Please add a dedicated `ecm_add_test()` below for this new test.

> adddialog.cpp:61
> +
> +    QPushButton *optionsButton = new QPushButton(i18n("Advanced Options"));
> +    optionsButton->setSizePolicy(QSizePolicy::Fixed, QSizePolicy::Fixed);

Maybe this button could have the "option" icon? (the one used also in the Settings menu)

> adddialog.cpp:70
> +
> +    m_fileWidget->okButton()->setText(i18n("Add"));
> +    m_fileWidget->okButton()->show();

`i18nc("@action:button", "Add")`

> adddialog.cpp:103
> +    CompressionOptionsWidget *optionsWidget = new CompressionOptionsWidget(m_mimeType, m_compOptions, optionsDialog);
> +    optionsWidget->setMinimumWidth(300);
> +    optionsWidget->setEncryptionVisible(false);

Is this really necessary?

> adddialog.cpp:120
> +        optionsDialog->deleteLater();
> +        optionsWidget->deleteLater();
> +    });

`optionsWidget` is a child of `optionsDialog`, no need to delete it manually.

> adddialog.h:64
> +public slots:
> +    void slotOpenOptions(bool checked);
> +};

`checked` is not used, we can drop it (the `connect()` will work fine).

> compressionoptionswidget.h:47
> +                                      const CompressionOptions &opts = QHash<QString, QVariant>(),
> +                                      QWidget *parent = 0);
> +    int compressionLevel() const;

`Q_NULLPTR` instead of 0

> createdialog.cpp:96-97
>  
> +    optionsWidget = new CompressionOptionsWidget(currentMimeType(), QHash<QString, QVariant>(), this);
> +    m_ui->vlayout->insertWidget(1, optionsWidget);
> +

Can't you add this widget in `createdialog.ui`? You can call `setMimeType()` instead of passing the mimetype to the constructor.

> part.cpp:1244
> +    QPointer<AddDialog> dlg = new AddDialog(widget(),
> +                                            i18nc("@title:window", "Add Files"),
> +                                            QUrl::fromLocalFile(QDir::homePath()),

The name of the actions and the title of the dialog are not consistent:

- Action in the menu => `Add File...`
- Action in the toolbar => `Add File`
- Title of the dialog => `Add Files`

Not sure what should we do, though...

REPOSITORY
  rARK Ark

REVISION DETAIL
  https://phabricator.kde.org/D2120

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: rthomsen, elvisangelaccio
Cc: kde-utils-devel, tctara
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-utils-devel/attachments/20160708/1d41b61d/attachment-0001.html>