D15980: Warn user before renaming the file/folder starting with a ' . '

[Shubham]

shubham created this revision.
shubham added reviewers: elvisangelaccio, broulik.
shubham added a project: Dolphin.
Herald edited subscribers, added: kfm-devel; removed: Dolphin.
shubham requested review of this revision.

REVISION SUMMARY
For normal "casual" linux users, renaming the file/folder starting with dot may get irritating, they will be wondering their file is deleted.

TEST PLAN
1. Make new file/folder.
2. Rename it to .foo
3. Warning dialog appears.

Screenshot:
F6307175: a.png <https://phabricator.kde.org/F6307175>

REPOSITORY
R318 Dolphin

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a reviewer: ngraham.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham retitled this revision from "Warn user before renaming the file/folder starting with a ' . '" to "Warn user before renaming the file/folder to start with a ' . '".

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


Please don't use `goto`.
Also this will need some i18n fixes to avoid word puzzles, see https://api.kde.org/frameworks/ki18n/html/prg_guide.html#good_text

And what if hidden files are visible?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

Post by Elvis Angelaccio
Also this will need some i18n fixes to avoid word puzzles, see https://api.kde.org/frameworks/ki18n/html/prg_guide.html#good_text

I did't get you. Do you mean to change the wordings?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Otto]

totto added inline comments.

INLINE COMMENTS

dolphinview.cpp:1584
+ const int code = KMessageBox::warningContinueCancel(nullptr,
+ i18n("The %1 name starts with a dot, hence it will be hidden by default.", item),
+ i18n("Hide this %1?", item),

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

dolphinview.cpp:1588
+ KStandardGuiItem::cancel(),
+ i18n(":Don not ask again"),
+ KMessageBox::Notify

Do* not ..

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Thomas Otto]

totto added a comment.

Post by Shubham
I did't get you. Do you mean to change the wordings?

The `i18n("Hide this %1?", item)` with the "puzzle" piece `item` should be two lines -- "Hide this file" + "Hide this folder" -- because this puzzle is not solvable correctly in all languages when "this" changes depending on what file/folder is translated to.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 42967.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=42960&id=42967

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham
Cc: totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham marked 2 inline comments as done.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


1. Add check for hiddenFilesShown( )
2. Fix typos
3. Fix i18n

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham
Cc: totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Pino Toscano]

pino requested changes to this revision.
pino added a comment.
This revision now requires changes to proceed.


In addition to the link at Elvis already provided, please check https://hig.kde.org/style/writing/index.html for the guidelines on the style.

Also, the message itself seems poor: the dialog is basically a question (and the buttons are "replies" to the question), so its text ought to be a question, not a statement.

INLINE COMMENTS

dolphinview.cpp:1583
+ KGuiItem continueGuiItem(KStandardGuiItem::cont());
+

"Rename and Hide"

dolphinview.cpp:1585
+
+ const int code = KMessageBox::warningContinueCancel(nullptr,
+ i18n("The %1 name starts with a dot, therefore it will be hidden by default.", item),

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

dolphinview.cpp:1586
+ const int code = KMessageBox::warningContinueCancel(nullptr,
+ i18n("The %1 name starts with a dot, therefore it will be hidden by default.", item),
+ item == QLatin1String("file") ? i18n("Hide this file?") : i18n("Hide this directory?"),

"item" here is still a string puzzle.

dolphinview.cpp:1587
+ i18n("The %1 name starts with a dot, therefore it will be hidden by default.", item),
+ item == QLatin1String("file") ? i18n("Hide this file?") : i18n("Hide this directory?"),
+ continueGuiItem,

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

dolphinview.cpp:1590
+ KStandardGuiItem::cancel(),
+ i18n(":Do not ask again"),
+ KMessageBox::Notify

why the colon at the beginning of the string?

dolphinview.cpp:1596
+ return;
+ }
+ }

wrong indentation

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


This feature is a good idea.

I'd like to request a bit more user-friendliness in the string, though. The string should clearly communicate what is about to happen without suggesting that it is already happened, and it should suggest a way out if you do opt to hide the file. Here's suggestion:

If it's a file:

xi18nc("@info", "Adding a dot to the beginning of this file's name will hide it from view.<nl/><nl/>To show hidden files and folders, go to <interface>View->Hidden Files</interface> or type <shortcut>alt+period</shortcut>.

If it's a directory:

xi18nc("@info", "Adding a dot to the beginning of this folder's name will hide it from view.<nl/><nl/>To show hidden files and folders, go to <interface>View->Hidden Files</interface> or type <shortcut>alt+period</shortcut>.

For details regarding what these tags do, see https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham requested changes to this revision.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham marked an inline comment as done.
shubham added a comment.

Post by Nathaniel Graham
This feature is a good idea.
For details regarding what these tags do, see https://techbase.kde.org/Development/Tutorials/Localization/i18n_Semantics

I liked your suggestion, but it is a question message box (not informatory message box)

INLINE COMMENTS

Post by Nathaniel Graham
pino wrote in dolphinview.cpp:1586
"item" here is still a string puzzle.

I did't get. ?

Post by Nathaniel Graham
pino wrote in dolphinview.cpp:1590
why the colon at the beginning of the string?

This setting will get stored in global config file

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 42974.
shubham added a comment.


Made above requested changes.
F6308032: 1.png <https://phabricator.kde.org/F6308032>

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=42967&id=42974

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Pino Toscano]

pino requested changes to this revision.
pino added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinview.cpp:1579
+ //Confirm hiding file/folder by renaming inline
+ if (newName.startsWith(QChar('.')) && !oldItem.name().startsWith(QChar('.')) && !hiddenFilesShown()) {
+ KGuiItem yesGuiItem(KStandardGuiItem::yes());

- use QLatin1Char instead of QChar, since it is built from a latin1 character
- make the `!hiddenFilesShown()` check as first, as it is cheaper

dolphinview.cpp:1583
+
+ const int code = KMessageBox::questionYesNo(nullptr,

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

dolphinview.cpp:1587
+ "go to <interface>View->Hidden Files</interface> or type "
+ "<shortcut>alt+period</shortcut>."
+ )

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

dolphinview.cpp:1591
+ "view.<nl/>Do you still want to rename it?<nl/><nl/>To show hidden files or " "directories, go to <interface>View->Hidden Files</interface> or type "
+ "<shortcut>alt+period</shortcut>."
+ ),

ditto

dolphinview.cpp:1593
+ ),
+ oldItem.isFile() ? i18n("Hide this file?") : i18n("Hide this directory?"),
+ yesGuiItem,

please fix the capitalization, see the HIG

shubham wrote in dolphinview.cpp:1590
This setting will get stored in global config file

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

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.


Now that we're using a `KMessageBox::questionYesNo`, the "No" choice is now just the word "No", which we should avoid. Instead, the button's text should be "Cancel", and it should have the appropriate cancel icon. You can use a `KStandardGuiItem::cancel()` for this.

I had another idea: instead of telling the user how to show hidden files inside the now somewhat crowded dialog box, we could show it as an inline message using `dolphinViewContainer::showMessage()`. Bonus points if you add Undo and/or Show Hidden Files button to the widget!

INLINE COMMENTS

pino wrote in dolphinview.cpp:1593
please fix the capitalization, see the HIG

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

pino wrote in dolphinview.cpp:1586
"item" here is still a string puzzle.

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

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added inline comments.

INLINE COMMENTS

pino wrote in dolphinview.cpp:1583
this dialog needs a parent, to be sure it stacks properly

which is the parent widget in this case?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added inline comments.

INLINE COMMENTS

shubham wrote in dolphinview.cpp:1583
which is the parent widget in this case?

QWidget(parent) gives an error

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

Post by Nathaniel Graham
I had another idea: instead of telling the user how to show hidden files inside the now somewhat crowded dialog box, we could show it as an inline message using `dolphinViewContainer::showMessage()`. Bonus points if you add Undo and/or Show Hidden Files button to the widget!

Then will have to use createMessageBox() instead.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Pino Toscano]

pino added inline comments.

INLINE COMMENTS

shubham wrote in dolphinview.cpp:1583
which is the parent widget in this case?
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

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

Post by Nathaniel Graham
I had another idea: instead of telling the user how to show hidden files inside the now somewhat crowded dialog box, we could show it as an inline message using `dolphinViewContainer::showMessage()`. Bonus points if you add Undo and/or Show Hidden Files button to the widget!

@pino
your thoughts on this?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, totto, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


In D15980#337813 <https://phabricator.kde.org/D15980#337813>, @ngraham wrote:
Bonus points if you add Undo and/or Show Hidden Files button to the widget!
"undo " button? The user can cancel it.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43057.
shubham edited the test plan for this revision.
shubham added a comment.


F6310225: 3.png <https://phabricator.kde.org/F6310225>

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=42974&id=43057

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.

Post by Shubham
F6310225: 3.png <https://phabricator.kde.org/F6310225>

My idea was that the "Show Hidden Files" button would be on an inline `KMessageWidget` that shows up after you do "rename and hide". It doesn't make any sense to put it in the dialog.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

Post by Nathaniel Graham
My idea was that the "Show Hidden Files" button would be on an inline `KMessageWidget` that shows up after you do "rename and hide". It doesn't make any sense to put it in the dialog.

Ok. What should I name that inline message, because that inline message would be guiding the user how to unhide files/folders, which don t make sense as the message will be having "show Hidden Files" button.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.

Post by Shubham

Post by Nathaniel Graham
My idea was that the "Show Hidden Files" button would be on an inline `KMessageWidget` that shows up after you do "rename and hide". It doesn't make any sense to put it in the dialog.

Ok. What should I name that inline message, because that inline message would be guiding the user how to unhide files/folders, which don t make sense as the message will be having "show Hidden Files" button.

The text could be something like, `<file> has now been hidden. [show hidden files button]`

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

"Undo" button won't be suited here , if the user had really wanted to not hide, they would have choosen "Cancel"

Your comment?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.

"Undo" button won't be suited here , if the user had really wanted to not hide, they would have choosen "Cancel"

If that's how human behavior worked, there would be no need for "undo" functionality at all. :) The concept of "Undo" is premised on the idea that sometimes people who are moving quickly make a mistake that they only understand moments after doing it. That's when an Undo is useful.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


@ngraham
So you want both undo and show hidden items button?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


Now that I think about it, we might want to tackle that in multiple patches:

1. In the current patch, just show a dialog box asking for user confirmation
2. In another patch, add a new `showMessageWithButtons()` function that can show a kmessagewidget with buttons
3. In a third patch, use that new function to add a message saying, "$file is now hidden. Undo Show/Hide Hidden Files"

Does that make sense?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


@ngraham makes sense to me. Will submit those subsequent patches later.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43238.
shubham added a comment.


Made above requested changes.
F6315447: Screenshot_20181009_220937.png <https://phabricator.kde.org/F6315447>

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43057&id=43238

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham retitled this revision from "Warn user before renaming the file/folder to start with a ' . '" to "Warn user before renaming the file/directory to start with a ' . '".

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.


Thanks! One more thing: let's use "folder" instead of "directory" for the user-facing strings.

Also please mark the inline comments that are fixed.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43239.
shubham added a comment.


"Folder"

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43238&id=43239

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham marked 6 inline comments as done.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


This now looks good to me in terms of both code, functionality, and presentation. Make sure all other reviewers have changed their statuses to "Accepted" before landing.

Thanks again for the patch!

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.

Post by Nathaniel Graham
Make sure all other reviewers have changed their statuses to "Accepted" before landing.

Sure.
@pino @elvisangelaccio what's your say on this now?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a subscriber: elvisangelaccio.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


@pino @elvisangelaccio are there any comments from you side?
If not, I will be landing this revison.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinview.cpp:1584
+
+ const int code = KMessageBox::questionYesNo(this,
+ oldItem.isFile() ? i18n("Adding a dot to the beginning of this file's name will hide it from view.\n"

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

dolphinview.cpp:1587
+ "Do you still want to rename it?"
+ )
+ : i18n("Adding a dot to the beginning of this folder's name will hide it from view.\n"

Please move at the end of previous line

dolphinview.cpp:1589
+ : i18n("Adding a dot to the beginning of this folder's name will hide it from view.\n"
+ "Do you still want to rename it?" ),
+ oldItem.isFile() ? i18n("Hide this File?") : i18n("Hide this Folder?"),

Please remove the spaces before `),`

dolphinview.cpp:1593
+ KStandardGuiItem::cancel(),
+ QLatin1String(":Do not ask again"),
+ KMessageBox::Notify

Please use `QStringLiteral`

dolphinview.cpp:1595
+ KMessageBox::Notify
+ );
+

Please move at the end of previous line

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43537.
shubham marked 5 inline comments as done.
shubham retitled this revision from "Warn user before renaming the file/directory to start with a ' . '" to "Warn user before renaming the file/folder to start with a ' . '".
shubham added a comment.


Made above requested changes by Elvis.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43239&id=43537

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinview.cpp:1592
+ KStandardGuiItem::cancel(),
+ QStringLiteral(":Do not ask again"),
+ KMessageBox::Notify);

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.

dolphinview.cpp:1593
+ QStringLiteral(":Do not ask again"),
+ KMessageBox::Notify);
+

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

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43731.
shubham marked 2 inline comments as done.
shubham added a comment.


1. Remove KMessageBox::Notify
2. Use more descriptive key string
3. Remove ' : '

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43537&id=43731

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, broulik, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham removed a reviewer: broulik.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino, broulik
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


Works and looks good to me!

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


@pino @elvisangelaccio Can you give green flags from your side?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinview.cpp:1592
+ KStandardGuiItem::cancel(),
+ i18n("ConfirmHide")
+ );

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

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 43927.
shubham added a comment.


Use QStringLiteral

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43731&id=43927

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


Almost there :)

INLINE COMMENTS

dolphinview.cpp:1585
+ const auto code = KMessageBox::questionYesNo(this,
+ oldItem.isFile() ? i18n("Adding a dot to the beginning of this file's name will hide it from view.\n"
+ "Do you still want to rename it?")

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

dolphinview.cpp:1587
+ "Do you still want to rename it?")
+ : i18n("Adding a dot to the beginning of this folder's name will hide it from view.\n"
+ "Do you still want to rename it?"),

and also here.

dolphinview.cpp:1593
+ QStringLiteral("ConfirmHide")
+ );
+

Please move it at the end of previous line

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham updated this revision to Diff 44687.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=43927&id=44687

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added a comment.


Thanks.

Do you have commit access?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

shubham added a comment.


@elvisangelaccio Yes, I have commit access, but what about @pino 's review?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


@pino, all good now?

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Emirald Mateli]

emateli added a comment.


The prompt "Do you still want to rename it" feels unnatural/unusual. "Continue?" or "Do you wish to continue?" or something similar might work better. Just a suggestion, I do not have a strong opinion on the matter.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Nathaniel Graham]

ngraham added a comment.

Post by Emirald Mateli
The prompt "Do you still want to rename it" feels unnatural/unusual. "Continue?" or "Do you wish to continue?" or something similar might work better. Just a suggestion, I do not have a strong opinion on the matter.

I think it's okay as-is. It's good to re-state what will happen in case people didn't read it the first time.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp

[Shubham]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R318:e68f24637ca8: Warn user before renaming the file/folder to start with a &#039; . &#039; (authored by shubham).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D15980?vs=44687&id=45023#toc

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=44687&id=45023

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, ngraham, pino
Cc: emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Christoph Feck]

cfeck added a comment.


The patch didn't land, probably because you tried to push it to non-existant `arcpatch-D15980` branch.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: cfeck, emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham added a comment.


...Yet another thing that would be made far easier by using `arc`. ;)

Here's what you need to do now:

- Set up `arc`: https://community.kde.org/Infrastructure/Phabricator#Using_Arcanist
- `arc patch D15980` (re-download the patch locally)
- `arc land` (land it using arc)

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: cfeck, emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Shubham]

shubham reopened this revision.

REPOSITORY
R318 Dolphin

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

To: shubham, elvisangelaccio, ngraham, pino
Cc: cfeck, emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Shubham]

This revision was not accepted when it landed; it landed in state "Needs Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R318:b08cea86c972: Warn user before renaming the file/folder to start with a &#039; . &#039; (authored by shubham).

CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D15980?vs=45023&id=45100#toc

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D15980?vs=45023&id=45100

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

AFFECTED FILES
src/views/dolphinview.cpp

To: shubham, elvisangelaccio, ngraham, pino
Cc: cfeck, emateli, elvisangelaccio, pino, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov