Add an option to confirm trash emptying into settings.
BUG: 340572
Details
Details
- Reviewers
ngraham markg - Group Reviewers
Dolphin - Commits
- R318:436ad965e934: Add an option to confirm trash emptying into settings
Diff Detail
Diff Detail
- Repository
- R318 Dolphin
- Branch
- empty-trash-confirmation
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Works fine, but I would suggest to move the option just below the option for "Moving to trash", because it is hardly the most important option (which should be at the top). This way it would still be close to the other "trash" option, and the whole list would be ordered by severity of the action.
Comment Actions
Works fine and the code looks good too. It's so nice to to see you tackle so many longstanding bugs and feature requests, Roman!
Now I wonder if we can't improve the usability a bit here in a follow-up patch. After this patch lands this patch, 3/4 of the confirmation checkboxes will be related to the trash and deleting things. I think it might make more sense to put them all under the Trash page (where people will naturally go looking for them), rather than hidden away in General → Confirmations.
That will leave only two confirmations under the Confirmations tab. We might consider removing the Confirmations tab entirely, moving the remaining two confirmation checkboxes onto the Behavior tab. We could make room for them by removing that tab's existing group boxes and headers, and making the radio button controls look more like those in Gwenview and modern KCMs: with the label to the left of the radio buttons, and all the text written so it can be read in the form of a sentence, e.g.:
Sort (*) Naturally
() Alphabetically (case insensitive)
() Alphabetically (case sensitive)What do folks think of that?
Comment Actions
Now I wonder if we can't improve the usability a bit here in a follow-up patch.
Let's open new tasks on the workboard to discuss follow-ups in the future ;)
What do folks think of that?
Makes sense, but don't forget about the "in all KDE applications" caveat. Splitting this makes it harder to communicate, IOW you'd have to duplicate the label and in one place it would only affect a single random item on a tab. Are there plans to move/duplicate/integrate this wrt. to Systemsettings?
| src/settings/general/confirmationssettingspage.h | ||
|---|---|---|
| 47 ↗ | (On Diff #28404) | Should be adapted too, to be in the same order as in the UI. |
| src/settings/general/confirmationssettingspage.h | ||
|---|---|---|
| 47 ↗ | (On Diff #28404) | Fixed |
Comment Actions
Looks fine, nice to have :)
Feel free to ship it without updating the review for that one little pointer position change.
Or you can update the diff and ship right after, your call.
| src/settings/general/confirmationssettingspage.h | ||
|---|---|---|
| 48 | Consistency. issue, nothing big. | |
Comment Actions
Kind of, see T5970. Feel free to extend the task or open new ones. Global settings that affect all applications should not be configurable only from Dolphin.