Changeset View
Standalone View
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
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 | | ||||
pino: "Rename and Hide" | |||||
pino: this dialog needs a parent, to be sure it stacks properly | |||||
shubham: which is the parent widget in this case? | |||||
shubham: QWidget(parent) gives an error | |||||
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, | ||||
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… | |||||
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() ? xi18nc("@info", "Adding a dot to the beginning of this file's name will hide it from view." | ||||
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… | |||||
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 | "<nl/>Do you still want to rename it?") | ||||
pino: "item" here is still a string puzzle. | |||||
shubham: I did't get. ? | |||||
@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 | : xi18nc("@info", "Adding a dot to the beginning of this folder's name will hide it from view." | ||||
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… | |||||
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… | |||||
elvisangelaccio: Please move at the end of previous line | |||||
elvisangelaccio: and also here. | |||||
1588 | "<nl/>Do you still want to rename it?"), | ||||
totto: Do* not .. | |||||
1589 | oldItem.isFile() ? i18n("Hide this File?") : i18n("Hide this Folder?"), | ||||
elvisangelaccio: Please remove the spaces before `),` | |||||
1590 | yesGuiItem, | ||||
pino: why the colon at the beginning of the string? | |||||
shubham: This setting will get stored in global config file | |||||
pino: Then it must not be i18n'ed, otherwise it will break when switching language! | |||||
1591 | KStandardGuiItem::cancel(), | ||||
pino: ditto | |||||
1592 | QStringLiteral("ConfirmHide")); | ||||
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"… | |||||
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 | | ||||
pino: please fix the capitalization, see the HIG | |||||
Relevant section is https://hig.kde.org/style/writing/capitalization.html ngraham: Relevant section is https://hig.kde.org/style/writing/capitalization.html | |||||
elvisangelaccio: Please use `QStringLiteral` | |||||
elvisangelaccio: This argument can be removed, it's already used as default. | |||||
elvisangelaccio: Please move it at the end of previous line | |||||
1594 | if (code == KMessageBox::No) { | ||||
1595 | return; | ||||
elvisangelaccio: Please move at the end of previous line | |||||
1596 | } | ||||
pino: wrong indentation | |||||
1597 | } | ||||
1598 | #endif | ||||
1599 | | ||||
1578 | const bool newNameExistsAlready = (m_model->index(newUrl) >= 0); | 1600 | const bool newNameExistsAlready = (m_model->index(newUrl) >= 0); | ||
1579 | if (!newNameExistsAlready) { | 1601 | if (!newNameExistsAlready) { | ||
1580 | // Only change the data in the model if no item with the new name | 1602 | // Only change the data in the model if no item with the new name | ||
Context not available. |