KCM Icons fix theme selected when we hit delete theme
ClosedPublic

Authored by bport on Oct 22 2019, 6:56 AM.

Details

Summary

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

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bport created this revision.Oct 22 2019, 6:56 AM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 22 2019, 6:56 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bport requested review of this revision.Oct 22 2019, 6:56 AM

Hmm, let's see what VDG thinks about that.

bport added a comment.Oct 22 2019, 7:03 AM

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!
ervin added a subscriber: bport.Oct 23 2019, 5:39 AM

Hello,

My bad this patch depends on D24846

broulik added a comment.EditedOct 23 2019, 11:39 AM

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

  1. we delete B : B pending deletion A still selected
  2. we delete A : A and B pending deletion C selected

What happen if we delete the last one ?

ervin added a comment.Oct 23 2019, 1:13 PM

I will update this patch to allow to delete current selected theme but remove bug
So if we have A B C, A selected

  1. we delete B : B pending deletion A still selected
  2. 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.

bport updated this revision to Diff 69037.Oct 30 2019, 10:38 AM

Take Nate feedback and do a proper fix

nitpick, but otherwise LGTM, giving time to others (in particular Nate) to chip in.

kcms/icons/iconsmodel.cpp
91

I'd say keep the empty line please.

ngraham requested changes to this revision.Oct 30 2019, 4:02 PM

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.

This revision now requires changes to proceed.Oct 30 2019, 4:02 PM
bport added a comment.Oct 30 2019, 5:32 PM

Indeed don't know why even if isSaveNeeded return true, button is not updated. I will investigate
Thanks for the catch

bport updated this revision to Diff 70453.Nov 27 2019, 6:53 PM

Fix regression (apply button not activated when eeded) and ervin feedback

davidedmundson accepted this revision.Dec 9 2019, 10:30 AM
bport updated this revision to Diff 71118.Dec 9 2019, 12:29 PM

Take in consideration feedbacks

davidedmundson accepted this revision.Dec 9 2019, 4:13 PM

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?

bport updated this revision to Diff 71128.Dec 9 2019, 4:54 PM

Update

bport updated this revision to Diff 71669.Dec 16 2019, 3:17 PM

Update code to take in consideration change done in D24826

ervin accepted this revision.Mon, Dec 23, 11:10 AM
bport added a comment.Thu, Dec 26, 8:31 AM

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
ngraham accepted this revision.Mon, Dec 30, 3:56 PM

Thanks, works great now.

This revision is now accepted and ready to land.Mon, Dec 30, 3:56 PM
This revision was automatically updated to reflect the committed changes.