Warn user before renaming the file/folder to start with a ' . '
ClosedPublic

Authored by shubham on Oct 6 2018, 6:08 AM.

Details

Summary

BUG: 356770
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. Question dialog appears.

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
pino added inline comments.Oct 6 2018, 7:33 PM
src/views/dolphinview.cpp
1579
  • use QLatin1Char instead of QChar, since it is built from a latin1 character
  • make the !hiddenFilesShown() check as first, as it is cheaper
1583

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

1587

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

1591

ditto

1593

please fix the capitalization, see the HIG

This revision now requires changes to proceed.Oct 6 2018, 7:33 PM
ngraham requested changes to this revision.Oct 6 2018, 11:19 PM

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!

src/views/dolphinview.cpp
1586

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

1593
shubham added inline comments.Oct 7 2018, 4:40 AM
src/views/dolphinview.cpp
1583

which is the parent widget in this case?

shubham added inline comments.Oct 7 2018, 4:49 AM
src/views/dolphinview.cpp
1583

QWidget(parent) gives an error

shubham added a comment.EditedOct 7 2018, 4:54 AM

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!

"Show Hidden Files" button would be redundant because the inline message would have communicated that piece of information already.

pino added inline comments.Oct 7 2018, 5:00 AM
src/views/dolphinview.cpp
1583

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

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?

totto removed a subscriber: totto.Oct 7 2018, 11:01 AM

Bonus points if you add Undo and/or Show Hidden Files button to the widget!
"undo " button? The user can cancel it.

shubham updated this revision to Diff 43057.Oct 7 2018, 4:41 PM
shubham edited the test plan for this revision. (Show Details)

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.

shubham added a comment.EditedOct 8 2018, 2:02 AM

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.

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

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]

This comment was removed by shubham.

"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.

shubham added a comment.EditedOct 9 2018, 6:50 AM

@ngraham
So you want both undo and show hidden items button.
The "Undo" would just be deleting the first character(.) from the newName to unhide it.

And "Show Hidden Items" button would be just setting setHiddenFilesShow() to true.

Can you help me out how to add those two butons to KMessageWidget?

Also the DolphinViewContainer constructor asks for parent widget. Which is its parent widget here?

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?

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

shubham updated this revision to Diff 43238.Oct 9 2018, 4:40 PM

Made above requested changes.

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 ' . '.Oct 9 2018, 4:41 PM

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.

shubham updated this revision to Diff 43239.Oct 9 2018, 4:44 PM
shubham marked an inline comment as done.

"Folder"

shubham marked 6 inline comments as done.Oct 9 2018, 4:47 PM
ngraham accepted this revision.Oct 9 2018, 8:58 PM

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!

Make sure all other reviewers have changed their statuses to "Accepted" before landing.

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

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

elvisangelaccio requested changes to this revision.Oct 13 2018, 10:27 AM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1584

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

1587

Please move at the end of previous line

1589

Please remove the spaces before ),

1593

Please use QStringLiteral

1595

Please move at the end of previous line

This revision now requires changes to proceed.Oct 13 2018, 10:27 AM
shubham updated this revision to Diff 43537.Oct 13 2018, 12:18 PM
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 ' . '.

Made above requested changes by Elvis.

elvisangelaccio requested changes to this revision.Oct 14 2018, 10:10 AM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1592

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.

1593

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

This revision now requires changes to proceed.Oct 14 2018, 10:10 AM
shubham updated this revision to Diff 43731.EditedOct 16 2018, 12:39 PM
shubham marked 2 inline comments as done.
  1. Remove KMessageBox::Notify
  2. Use more descriptive key string
  3. Remove ' : '

@elvisangelaccio Is it good to go now?

ngraham accepted this revision.Oct 16 2018, 9:44 PM

Works and looks good to me!

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

elvisangelaccio requested changes to this revision.Oct 19 2018, 2:27 PM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1592

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

This revision now requires changes to proceed.Oct 19 2018, 2:27 PM
shubham updated this revision to Diff 43927.Oct 19 2018, 2:44 PM

Use QStringLiteral

shubham marked an inline comment as done.Oct 19 2018, 2:45 PM
elvisangelaccio requested changes to this revision.Oct 20 2018, 10:16 AM

Almost there :)

src/views/dolphinview.cpp
1585

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

1587

and also here.

1593

Please move it at the end of previous line

This revision now requires changes to proceed.Oct 20 2018, 10:16 AM
shubham updated this revision to Diff 44687.Nov 2 2018, 3:49 AM
elvisangelaccio accepted this revision.Nov 2 2018, 10:30 AM

Thanks.

Do you have commit access?

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

ngraham accepted this revision.Nov 2 2018, 1:49 PM

@pino, all good now?

emateli added a subscriber: emateli.Nov 2 2018, 4:33 PM

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.

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.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 7 2018, 9:53 AM
This revision was automatically updated to reflect the committed changes.
cfeck added a subscriber: cfeck.Nov 8 2018, 12:05 AM

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

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

Here's what you need to do now:

shubham reopened this revision.Nov 8 2018, 8:45 AM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 8 2018, 8:49 AM
This revision was automatically updated to reflect the committed changes.
shubham edited the summary of this revision. (Show Details)Dec 17 2018, 1:54 PM