Add a properties dialog that shows various info for the opened archive.
Details
Details
- Reviewers
elvisangelaccio - Maniphest Tasks
- T919: Add a properties dialog
- Commits
- R36:5552a4c95aa1: Add properties dialog
Diff Detail
Diff Detail
- Repository
- R36 Ark
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Nice work. I will wait an updated version of the patch, before adding specific comments.
For now just notice that the archive name will be truncated if too long.
| kerfuffle/propertiesdialog.cpp | ||
|---|---|---|
| 71 | setFixedSize(m_ui->size()); looks what you were looking for :) | |
Comment Actions
Looks good, I have mostly nitpicks :)
| kerfuffle/propertiesdialog.cpp | ||
|---|---|---|
| 55 | I don't understand this. Why not use one of the QFileInfo functions? Also, please use i18nc("@title:window", ...) here. | |
| 61 | QStringLiteral -> i18n(), same for the strings below. | |
| 66 | Use KIO::convertSize() here | |
| 69 | Same here. | |
| 70 | Maybe here could make sense a percentage symbol? | |
| 79 | New connect style? | |
| part/part.cpp | ||
| 363 | i18nc("@action:inmenu", ...) | |
| 415 | This way wa cannot open the dialog with read-only archives. I guess you wanted to check the validity of m_model->archive(), so better do it directly here. | |
Comment Actions
Incorporate Elvis' suggestions.
One question:
Archive::isPasswordProtected() (which already existed before this patch) cannot be const due to calling listIfNotListed(). In documentation for Q_PROPERTY it is recommended that all READ functions be const. What's the reason for isPasswordProtected() calling listIfNotListed()? And is it safe that this functions is not const? Also, if this function calls listIfNotListed() should some of the other READ functions call listIfNotListed() as well?
Comment Actions
Yeah the listIfNotListed() function is weird. I think is the right time to move it in the right place. After all we just need to list the archive once, in the Archive constructor. Then we need to list it again only after an update operation (add or delete for now).
If we do this, we can make the isPasswordProtected() function const, as it should be.