Differential D15980 Diff 43927 src/views/dolphinview.cpp

Changeset View

Standalone View

View Options

src/views/dolphinview.cpp

Context not available.
1575 QUrl newUrl = oldUrl.adjusted(QUrl::RemoveFilename); 1575​ QUrl newUrl = oldUrl.adjusted(QUrl::RemoveFilename);
1576 newUrl.setPath(newUrl.path() + KIO::encodeFileName(newName)); 1576​ newUrl.setPath(newUrl.path() + KIO::encodeFileName(newName));
1577 1577​
1578​#ifndef Q_OS_WIN
1579​ //Confirm hiding file/directory by renaming inline
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​ if (!hiddenFilesShown() && newName.startsWith(QLatin1Char('.')) && !oldItem.name().startsWith(QLatin1Char('.'))) {
1581​ KGuiItem yesGuiItem(KStandardGuiItem::yes());
1582​ yesGuiItem.setText(i18nc("@action:button", "Rename and Hide"));
1583​
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​ const auto code = KMessageBox::questionYesNo(this,
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…
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 = ...
1585​ oldItem.isFile() ? i18n("Adding a dot to the beginning of this file's name will hide it from view.\n"
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…
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"…
1586​ "Do you still want to rename it?")
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​ : i18n("Adding a dot to the beginning of this folder's name will hide it from view.\n"
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…
elvisangelaccioUnsubmitted
Done

Please move at the end of previous line

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

and also here.

elvisangelaccio: and also here.
1588​ "Do you still want to rename it?"),
tottoUnsubmitted
Done

Do* not ..

totto: Do* not ..
1589​ oldItem.isFile() ? i18n("Hide this File?") : i18n("Hide this Folder?"),
elvisangelaccioUnsubmitted
Done

Please remove the spaces before ),

elvisangelaccio: Please remove the spaces before `),`
1590​ yesGuiItem,
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​ KStandardGuiItem::cancel(),
pinoUnsubmitted
Not Done

ditto

pino: ditto
1592​ QStringLiteral("ConfirmHide")
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 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…
1593​ );
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
elvisangelaccioUnsubmitted
Done

Please use QStringLiteral

elvisangelaccio: Please use `QStringLiteral`
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
Not Done

Please move it at the end of previous line

elvisangelaccio: Please move it at the end of previous line
1594​
1595​ if (code == KMessageBox::No) {
elvisangelaccioUnsubmitted
Done

Please move at the end of previous line

elvisangelaccio: Please move at the end of previous line
1596​ return;
pinoUnsubmitted
Done

wrong indentation

pino: wrong indentation
1597​ }
1598​ }
1599​#endif
1600​
1578 const bool newNameExistsAlready = (m_model->index(newUrl) >= 0); 1601​ const bool newNameExistsAlready = (m_model->index(newUrl) >= 0);
1579 if (!newNameExistsAlready) { 1602​ if (!newNameExistsAlready) {
1580 // Only change the data in the model if no item with the new name 1603​ // Only change the data in the model if no item with the new name
Context not available.