D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:9dacd46f4039: KCM Icons fix theme selected when we hit 
delete theme (authored by bport).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=71669=72447

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


  Thanks, works great now.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


  In D24847#574269 , @ngraham wrote:
  
  > 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 71669.
bport added a comment.


  Update code to take in consideration change done in D24826 


REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=71128=71669

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 71128.
bport added a comment.


  Update

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=71118=71128

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Nathaniel Graham]

ngraham added a comment.


  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?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 71118.
bport added a comment.


  Take in consideration feedbacks

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=7=71118

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp

To: bport, mart, ervin, #plasma, crossi, ngraham, davidedmundson
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 7.
bport added a comment.


  Fix

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=70453=7

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp
  kcms/icons/main.cpp
  kcms/icons/main.h
  kcms/icons/package/contents/ui/IconSizePopup.qml

To: bport, mart, ervin, #plasma, crossi, ngraham
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 70453.
bport added a comment.


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

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=69037=70453

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp
  kcms/icons/main.cpp
  kcms/icons/main.h
  kcms/icons/package/contents/ui/IconSizePopup.qml

To: bport, mart, ervin, #plasma, crossi, ngraham
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi, ngraham
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi, ngraham
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a comment.


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

INLINE COMMENTS

> iconsmodel.cpp:87
>  }
> -
>  emit pendingDeletionsChanged();

I'd say keep the empty line please.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport updated this revision to Diff 69037.
bport added a comment.


  Take Nate feedback and do a proper fix

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24847?vs=68499=69037

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Nathaniel Graham]

ngraham added a comment.


  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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a comment.


  In D24847#552665 , @bport wrote:
  
  > 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.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


  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 ?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


  By the way I can select next theme in case we delete the current instead of 
disable deletion

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Kai Uwe Broulik]

broulik added a comment.


  > 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?

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


  My bad this patch depends on D24846 

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

Re: D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

Hello,


On Tuesday, 22 October 2019 20:17:15 CEST Nathaniel Graham wrote:
>   Does this patch have unmarked dependencies? It doesn't apply for me:

I'd expect it also needs D24845 and D24846. They got all submitted together.

Regards.
-- 
Kevin Ottens, http://ervin.ipsquad.net

enioka Haute Couture - proud patron of KDE, https://hc.enioka.com/en

signature.asc
Description: This is a digitally signed message part.

D24847: KCM Icons fix theme selected when we hit delete theme

[Kevin Ottens]

ervin added a subscriber: bport.
ervin added a comment.


  Hello,
  
  - F7654931: signature.asc 

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: bport, ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, 
jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Nathaniel Graham]

ngraham added a comment.


  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!

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma, crossi
Cc: ngraham, broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, 
fbampaloukas, GB_2, ragreen, ZrenBot, alexeymin, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport added a comment.


  Yes sure, by the way the new behavior is the one implemented on other kcm 
like desktop theme

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Kai Uwe Broulik]

broulik added a comment.


  Hmm, let's see what #vdg  thinks about 
that.

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

To: bport, mart, ervin, #plasma
Cc: broulik, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D24847: KCM Icons fix theme selected when we hit delete theme

[Benjamin Port]

bport created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bport requested review of this revision.

REVISION 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

REPOSITORY
  R119 Plasma Desktop

REVISION DETAIL
  https://phabricator.kde.org/D24847

AFFECTED FILES
  kcms/icons/iconsmodel.cpp
  kcms/icons/main.cpp
  kcms/icons/package/contents/ui/main.qml

To: bport
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart