CUrrently when we delete a theme the next one is selected. With this fix we stay on the currently selected one and to simplify code don't allow to delete the currently selected theme
Details
- Reviewers
mart ervin crossi ngraham davidedmundson - Group Reviewers
Plasma - Commits
- R119:9dacd46f4039: KCM Icons fix theme selected when we hit delete theme
Diff Detail
- Repository
- R119 Plasma Desktop
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 19227 Build 19245: arc lint + arc unit
Yes sure, by the way the new behavior is the one implemented on other kcm like desktop theme
Does this patch have unmarked dependencies? It doesn't apply for me:
$ arc patch D24847 INFO Base commit is not in local repository; trying to fetch. created and checked out branch arcpatch-D24847. This diff is against commit 38806dbe3f9a65aea6ae031b959ff4adac9bf444, but the commit is nowhere in the working copy. Try to apply it against the current working copy state? (528388bfeb69fc22a259845e28fbef2a375e7bd6) [Y/n] y Checking patch kcms/icons/package/contents/ui/main.qml... Checking patch kcms/icons/main.cpp... error: while searching for: m_model->load(); m_model->setSelectedTheme(KIconTheme::current()); updateNeedsSave(); } void IconModule::save() error: patch failed: kcms/icons/main.cpp:148 Checking patch kcms/icons/iconsmodel.cpp... Applied patch kcms/icons/package/contents/ui/main.qml cleanly. Applying patch kcms/icons/main.cpp with 1 reject... Rejected hunk #1. Applied patch kcms/icons/iconsmodel.cpp cleanly. Patch Failed! Usage Exception: Unable to apply patch!
Yes sure, by the way the new behavior is the one implemented on other kcm like desktop theme
When I hit delete in desktop theme KCM I also have it move to the next theme?
The only KCM where I can a complaint about removal is cursor theme where it even shows a message box instead.
Don't know why but is not the case here.
And even if this is the case, I will need explaination on why we want to change theme selection when I delete another theme
I mean if we have 3 themes A B and C. If A is selected, I delete B why the selected theme will be C...
By the way I can select next theme in case we delete the current instead of disable deletion
I will update this patch to allow to delete current selected theme but remove bug
So if we have A B C, A selected
- we delete B : B pending deletion A still selected
- we delete A : A and B pending deletion C selected
What happen if we delete the last one ?
It's a bit theoretical in practice you always have at least one coming from the system which can't be deleted. Anyway, assuming you'd end up in such a situation: if it's the last one I'd say: don't allow deletion in the first place.
I don't like the change to prevent deletion of the current theme. It's awkward and feels unnatural to me. I don't see what was wrong with the prior behavior.
nitpick, but otherwise LGTM, giving time to others (in particular Nate) to chip in.
kcms/icons/iconsmodel.cpp | ||
---|---|---|
87 | I'd say keep the empty line please. |
The behavior is now fine, but your ability to actually delete icon themes has regressed. Once you click on a delegate's inline delete button, the apply button at the bottom of the page never becomes enabled.
Indeed don't know why even if isSaveNeeded return true, button is not updated. I will investigate
Thanks for the catch
So what is this patch doing now? When I delete the selected icon theme, the selection rect moves to the first one in the grid, causing the theme to change when you click Apply. This seems non-ideal.
Could it move selection over to the currently *in use* theme, maybe?
Not sure why you had this behavior
So currently behavior is :
- if you delete a theme the current one stay selected
- if you delete the current one, next one is selected