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.
Details
- Make new file/folder.
- Rename it to .foo
- Question dialog appears.
Diff Detail
- Repository
- R318 Dolphin
- Lint
Lint Skipped - Unit
Unit Tests Skipped
src/views/dolphinview.cpp | ||
---|---|---|
1579 |
| |
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 |
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 | Relevant section is https://hig.kde.org/style/writing/capitalization.html |
src/views/dolphinview.cpp | ||
---|---|---|
1583 | which is the parent widget in this case? |
src/views/dolphinview.cpp | ||
---|---|---|
1583 | QWidget(parent) gives an error |
"Show Hidden Files" button would be redundant because the inline message would have communicated that piece of information already.
src/views/dolphinview.cpp | ||
---|---|---|
1583 |
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 |
Bonus points if you add Undo and/or Show Hidden Files button to the widget!
"undo " button? The user can cancel it.
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"
The text could be something like, <file> has now been hidden. [show hidden files button]
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.
@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:
- In the current patch, just show a dialog box asking for user confirmation
- In another patch, add a new showMessageWithButtons() function that can show a kmessagewidget with buttons
- 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?
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.
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!
@pino @elvisangelaccio are there any comments from you side?
If not, I will be landing this revison.
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. |
- Remove KMessageBox::Notify
- Use more descriptive key string
- Remove ' : '
@elvisangelaccio Is it good to go now?
src/views/dolphinview.cpp | ||
---|---|---|
1592 | This is not a user-visible string, so it shouldn't be translated. Just use QStringLiteral("ConfirmHide"). |
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 |
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.
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:
- 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)