Differential D15980 Diff 43057 src/views/dolphinview.cpp

Changeset View

Standalone View

View Options

src/views/dolphinview.cpp

Context not available.
1574 QUrl newUrl = oldUrl.adjusted(QUrl::RemoveFilename); 1574​ QUrl newUrl = oldUrl.adjusted(QUrl::RemoveFilename);
1575 newUrl.setPath(newUrl.path() + KIO::encodeFileName(newName)); 1575​ newUrl.setPath(newUrl.path() + KIO::encodeFileName(newName));
1576 1576​
1577​#ifndef Q_OS_WIN
1578​ // Confirm hiding files/folders by renaming inline
1579​ if (!hiddenFilesShown() && newName.startsWith(QLatin1Char('.')) && !oldItem.name().startsWith(QLatin1Char('.'))) {
pinoUnsubmitted
Done
  • use QLatin1Char instead of QChar, since it is built from a latin1 character
  • make the !hiddenFilesShown() check as first, as it is cheaper
pino: - use QLatin1Char instead of QChar, since it is built from a latin1 character - make the `!
1580​ QDialog *dialog = new QDialog(this, Qt::Dialog);
1581​ dialog->setModal(true);
1582​ oldItem.isFile() ? dialog->setWindowTitle(i18nc("@title:window", "Hide this File?"))
1583​ : dialog->setWindowTitle(i18nc("@title:window", "Hide this Directory?"));
pinoUnsubmitted
Done

"Rename and Hide"

pino: "Rename and Hide"
pinoUnsubmitted
Done

this dialog needs a parent, to be sure it stacks properly

pino: this dialog needs a parent, to be sure it stacks properly
shubhamAuthorUnsubmitted
Done

which is the parent widget in this case?

shubham: which is the parent widget in this case?
shubhamAuthorUnsubmitted
Not Done

QWidget(parent) gives an error

shubham: QWidget(parent) gives an error
pinoUnsubmitted
Not Done

which is the parent widget in this case?

this dolphin view widget itself, for example? look around in the code... you can clearly see other dialogs used, and which parent widget they use

pino: > which is the parent widget in this case? this dolphin view widget itself, for example? look…
1584​
tottoUnsubmitted
Done

This is really nitpicky, but I would not use "hence" but "so" or "therefor" to keep the English simple.

totto: This is //really// nitpicky, but I would not use "hence" but "so" or "therefor" to keep the…
1585​ KGuiItem yesGuiItem(KStandardGuiItem::yes());
pinoUnsubmitted
Not Done

a warning seems a bit too strong (there is no dangerous situation), most probably a simpler yes/no question is better

pino: a warning seems a bit too strong (there is no dangerous situation), most probably a simpler…
1586​ yesGuiItem.setText(i18nc("@action:button", "Rename and Hide"));
pinoUnsubmitted
Done

"item" here is still a string puzzle.

pino: "item" here is still a string puzzle.
shubhamAuthorUnsubmitted
Not Done

I did't get. ?

shubham: I did't get. ?
ngrahamUnsubmitted
Not Done

@pino, looks good to me now with the latest version of this diff.

ngraham: @pino, looks good to me now with the latest version of this diff.
1587​
pinoUnsubmitted
Not Done

why do you check what "item" contains? there's already oldFile.isFile() that provides this information

pino: why do you check what "item" contains? there's already `oldFile.isFile()` that provides this…
pinoUnsubmitted
Not Done

please do not hardcode the shortcut, read it from the action collection; otherwise, if the user it them, the message will be misleading

pino: please do not hardcode the shortcut, read it from the action collection; otherwise, if the user…
1588​ KGuiItem showHiddenGuiItem(KStandardGuiItem::apply());
tottoUnsubmitted
Done

Do* not ..

totto: Do* not ..
1589​ showHiddenGuiItem.setText(i18nc("@action:button", "Show Hidden Items"));
1590​
pinoUnsubmitted
Not Done

why the colon at the beginning of the string?

pino: why the colon at the beginning of the string?
shubhamAuthorUnsubmitted
Not Done

This setting will get stored in global config file

shubham: This setting will get stored in global config file
pinoUnsubmitted
Done

Then it must not be i18n'ed, otherwise it will break when switching language!

pino: Then it must not be i18n'ed, otherwise it will break when switching language!
1591​ KGuiItem cancelGuiItem(KStandardGuiItem::cancel());
pinoUnsubmitted
Not Done

ditto

pino: ditto
1592​
1593​ QDialogButtonBox *buttons = new QDialogButtonBox(QDialogButtonBox::Yes | QDialogButtonBox::Apply | QDialogButtonBox::Cancel);
pinoUnsubmitted
Done

please fix the capitalization, see the HIG

pino: please fix the capitalization, see the HIG
ngrahamUnsubmitted
Not Done

Relevant section is https://hig.kde.org/style/writing/capitalization.html

ngraham: Relevant section is https://hig.kde.org/style/writing/capitalization.html
1594​ KGuiItem::assign(buttons->button(QDialogButtonBox::Yes), yesGuiItem);
1595​ KGuiItem::assign(buttons->button(QDialogButtonBox::Apply), showHiddenGuiItem);
1596​ KGuiItem::assign(buttons->button(QDialogButtonBox::Cancel), cancelGuiItem);
pinoUnsubmitted
Done

wrong indentation

pino: wrong indentation
1597​
1598​ bool doNotAskAgain = false;
1599​
1600​ const int code = KMessageBox::createKMessageBox(dialog,
1601​ buttons,
1602​ QMessageBox::Question,
1603​ oldItem.isFile() ? i18n("Adding a dot to the beginning of this file's name will hide it from view."
1604​ "\nDo you still want to rename it?"
1605​ )
1606​ : i18n("Adding a dot to the beginning of this directory's name will hide it from view."
1607​ "\nDo you still want to rename it?"
1608​ ),
1609​ QStringList(),
1610​ i18n("Do not ask again"),
1611​ &doNotAskAgain,
1612​ KMessageBox::Notify,
1613​ QString()
1614​ );
1615​
1616​ if (code == QDialogButtonBox::Apply) {
1617​ setHiddenFilesShown(true);
1618​ return;
1619​ }
1620​
1621​ if (code == QDialogButtonBox::Cancel) {
1622​ return;
1623​ }
1624​ }
1625​#endif
1626​
1577 const bool newNameExistsAlready = (m_model->index(newUrl) >= 0); 1627​ const bool newNameExistsAlready = (m_model->index(newUrl) >= 0);
1578 if (!newNameExistsAlready) { 1628​ if (!newNameExistsAlready) {
1579 // Only change the data in the model if no item with the new name 1629​ // Only change the data in the model if no item with the new name
Context not available.
elvisangelaccioUnsubmitted
Done

questionYesNo returns a ButtonCode, not an int. (or we can just use const auto code = ...).

elvisangelaccio: `questionYesNo` returns a `ButtonCode`, not an `int`. (or we can just use `const auto code = ...
elvisangelaccioUnsubmitted
Done

Please move at the end of previous line

elvisangelaccio: Please move at the end of previous line
elvisangelaccioUnsubmitted
Done

Please remove the spaces before ),

elvisangelaccio: Please remove the spaces before `),`
elvisangelaccioUnsubmitted
Done

Please move at the end of previous line

elvisangelaccio: Please move at the end of previous line
elvisangelaccioUnsubmitted
Done

Please use QStringLiteral

elvisangelaccio: Please use `QStringLiteral`
elvisangelaccioUnsubmitted
Done

Please use a more descriptive key string. It should be in CamelCase, for example "ConfirmHide" or similar.

Also, please remove the leading :. That will put the config key in kdeglobals, but this is a Dolphin-specific dialog and the config key should go in dolphinrc instead.

elvisangelaccio: Please use a more descriptive key string. It should be in CamelCase, for example "ConfirmHide"…
elvisangelaccioUnsubmitted
Done

This argument can be removed, it's already used as default.

elvisangelaccio: This argument can be removed, it's already used as default.
elvisangelaccioUnsubmitted
Done

This is not a user-visible string, so it shouldn't be translated. Just use QStringLiteral("ConfirmHide").

elvisangelaccio: This is not a user-visible string, so it shouldn't be translated. Just use `QStringLiteral…
elvisangelaccioUnsubmitted
Not Done

Please move it at the end of previous line

elvisangelaccio: Please move it at the end of previous line
elvisangelaccioUnsubmitted
Not Done

We should use semantic markup here (instead of hardcoding \n):

xi18nc("@info", "Adding a dot to the beginning of this file's name will hide it from view.<nl/>"
                "Do you still want to rename it?")

See https://api.kde.org/frameworks/ki18n/html/prg_guide.html#kuit_markup

elvisangelaccio: We should use semantic markup here (instead of hardcoding `\n`): ``` xi18nc("@info"…
elvisangelaccioUnsubmitted
Not Done

and also here.

elvisangelaccio: and also here.