D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

This revision was automatically updated to reflect the committed changes.
Closed by commit R858:35fae4b55fc8: Do not assign combobox currentIndex as it 
breaks binding. (authored by crossi).

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25000?vs=69055=69093

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

AFFECTED FILES
  org.kde.desktop/ComboBox.qml
  plugin/CMakeLists.txt
  plugin/kpropertywriter.cpp
  plugin/kpropertywriter_p.h
  plugin/qqc2desktopstyleplugin.cpp

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

crossi added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kpropertywriter_p.h:26
> Throwing out another option
> 
>   class KPropertyWriter : public QObject, public QQmlPropertyValueSource
>   {
>   Q_INVOKABLE bool writeProperty(QVariant value);
>   }
>   
>   writeProperty(QVariant) {
> object()->setProperty(name(), value());
> // we can't use property().write() as that'll break the binding
>   }
>   
>   
>   PropertyWriter on currentIndex {
>  id: controlRootWriter
>   }
> 
> Though it's basically the same thing, so don't feel you have to, just wanted 
> to share the suggestion as it reduces two properties.

Thank you for sharing other option, I didn't know this one.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

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


  Looks good to me. Please just wait a bit before pushing to give David a 
chance to object to my comments. ;-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kpropertywriter_p.h:26
> Throwing out another option
> 
>   class KPropertyWriter : public QObject, public QQmlPropertyValueSource
>   {
>   Q_INVOKABLE bool writeProperty(QVariant value);
>   }
>   
>   writeProperty(QVariant) {
> object()->setProperty(name(), value());
> // we can't use property().write() as that'll break the binding
>   }
>   
>   
>   PropertyWriter on currentIndex {
>  id: controlRootWriter
>   }
> 
> Though it's basically the same thing, so don't feel you have to, just wanted 
> to share the suggestion as it reduces two properties.

Didn't think about that one, clever trick indeed. :-)

That being said I think it'd be more work in the end, indeed setTarget is pure 
virtual in there, and also I'm not sure that calling write() on QQmlProperty 
doesn't break bindings.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added a comment.


  In D25000#556761 , @davidedmundson 
wrote:
  
  > There's a Plasma rule that if we're working round a Qt bug, there should be 
a Qt bug created and linked before accepting a workaround.
  >
  > From the sounds of it we want a QQuickControls::ComboBox::setIndex(int) 
invokable that doesn't update the binding?
  >  Or is it a more generic problem of somehow exposing 
QQmlPropertyData::WriteFlags ?
  
  
  It's the former, not the latter. And good luck having setIndex invokable, it 
would never go through a Qt review IMHO (and for good reason). Now we need this 
really because our own combo style insists on emulating a corner case of 
QComboBox behavior, it's fairly unusual and specific.
  I don't think it warrants putting something in Qt directly.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> kpropertywriter_p.h:26
> +
> +class KPropertyWriter : public QObject
> +{

Throwing out another option

  class KPropertyWriter : public QObject, public QQmlPropertyValueSource
  {
  Q_INVOKABLE bool writeProperty(QVariant value);
  }
  
  writeProperty(QVariant) {
object()->setProperty(name(), value());
// we can't use property().write() as that'll break the binding
  }
  
  
  PropertyWriter on currentIndex {
 id: controlRootWriter
  }

Though it's basically the same thing, so don't feel you have to, just wanted to 
share the suggestion as it reduces two properties.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[David Edmundson]

davidedmundson added a comment.


  There's a Plasma rule that if we're working round a Qt bug, there should be a 
Qt bug created and linked before accepting a workaround.
  
  From the sounds of it we want a QQuickControls::ComboBox::setIndex(int) 
invokable that doesn't update the binding?
  Or is it a more generic problem of somehow exposing 
QQmlPropertyData::WriteFlags ?

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
Cc: davidedmundson, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

crossi updated this revision to Diff 69055.
crossi added a comment.


  Missing const ref, rename private header accordingly

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25000?vs=69053=69055

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

AFFECTED FILES
  org.kde.desktop/ComboBox.qml
  plugin/CMakeLists.txt
  plugin/kpropertywriter.cpp
  plugin/kpropertywriter_p.h
  plugin/qqc2desktopstyleplugin.cpp

To: crossi, #plasma, ervin, mart, bport, broulik
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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

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


  Almost there! Glad we're getting near a proper fix. Can you confirm this 
works with *and* without D24916  applied?
  Can you also confirm this doesn't break the KScreen KCM? (as mentioned in 
Kai's comment on D24916 ).
  
  I want to make sure we don't introduce a regression for those who had started 
to workaround that bug on their side.

INLINE COMMENTS

> kpropertywriter.h:1
> +/*
> + *   Copyright (C) 2019 Cyril Rossi 

This file should be named kpropertywriter_p.h since it's not installed and the 
class not exported from a library.

> kpropertywriter.h:39
> +
> +Q_INVOKABLE bool writeProperty(QVariant value);
> +

const QVariant &

> kpropertywriter.h:43
> +void setTarget(QObject *target);
> +void setPropertyName(QString propertyName);
> +

const QString &

> kpropertywriter.h:47
> +void targetChanged(QObject *target);
> +void propertyNameChanged(QString propertyName);
> +

const QString &

> kpropertywriter.h:50
> +private:
> +QObject *m_target = nullptr;
> +QString m_propertyName;

If you want to play like this, you could have had a "using QObject::QObject;" 
for the ctor declaration and have no ctor definition in the cpp file. ;-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

crossi updated this revision to Diff 69053.
crossi added a comment.


  Remove assignement to currentIndex.
  Add a C++ bypass to setCurrentIndex without breaking the binding.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25000?vs=68874=69053

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

AFFECTED FILES
  org.kde.desktop/ComboBox.qml
  plugin/CMakeLists.txt
  plugin/kpropertywriter.cpp
  plugin/kpropertywriter.h
  plugin/qqc2desktopstyleplugin.cpp

To: crossi, #plasma, ervin, mart, bport, broulik
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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

crossi added inline comments.

INLINE COMMENTS

> ervin wrote in ComboBox.qml:92
> What about that one? :-)

Yes, this one breaks as well.

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, bport, broulik
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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Kevin Ottens]

ervin added a comment.


  Good start, there's one case still missing... which looks hard to reach, I 
wonder how to get there.

INLINE COMMENTS

> ComboBox.qml:92
>  if (indexUnderMouse > -1) {
>  controlRoot.currentIndex = indexUnderMouse;
>  controlRoot.activated(indexUnderMouse);

What about that one? :-)

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

To: crossi, #plasma, ervin, mart, 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

D25000: Do not assign combobox currentIndex as it breaks binding.

[Cyril Rossi]

crossi created this revision.
crossi added reviewers: Plasma, ervin, mart, bport.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
crossi requested review of this revision.

REVISION SUMMARY
  Binding with combobox index was broken

REPOSITORY
  R858 Qt Quick Controls 2: Desktop Style

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

AFFECTED FILES
  org.kde.desktop/ComboBox.qml

To: crossi, #plasma, ervin, mart, 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