DevHeads.net

Review Request: Fix KConfigDialogManager fails to handle subclasses of QComboBox with custom property

Review request for kdelibs, Eike Hein, Christoph Feck, and Jeremy Paul Whiting.

Description
<a href="https://git.reviewboard.kde.org/r/101486/" title="https://git.reviewboard.kde.org/r/101486/">https://git.reviewboard.kde.org/r/101486/</a> broke subclasses of QComboBox that have a USER property like KColorCombo, this patch reverts this change and introduces a different code path to ignore the USER property of QComboBox and KComboBox and make it use our custom code.

This addresses bug 293702.
<a href="http://bugs.kde.org/show_bug.cgi?id=293702" title="http://bugs.kde.org/show_bug.cgi?id=293702">http://bugs.kde.org/show_bug.cgi?id=293702</a>

Diffs
kdeui/dialogs/kconfigdialogmanager.cpp 0890c0b
kdeui/tests/CMakeLists.txt 63788f6
kdeui/tests/kconfigdialog_unittest.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/103909/diff/diff" title="http://git.reviewboard.kde.org/r/103909/diff/diff">http://git.reviewboard.kde.org/r/103909/diff/diff</a>

Testing
Ran the attached test, everything worked.

Without moving the
userproperty = getUserProperty(w);
the KColorCombo fails

Without adding the
s_propertyMap->insert( "KComboBox", "" );
the editable KComboBox fails

Thanks,

Albert Astals Cid

Comments

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/09/2012 - 19:28

(Updated Feb. 9, 2012, 11:28 p.m.)

Review request for kdelibs, Eike Hein, Christoph Feck, and Jeremy Paul Whiting.

Description
<a href="https://git.reviewboard.kde.org/r/101486/" title="https://git.reviewboard.kde.org/r/101486/">https://git.reviewboard.kde.org/r/101486/</a> broke subclasses of QComboBox that have a USER property like KColorCombo, this patch reverts this change and introduces a different code path to ignore the USER property of QComboBox and KComboBox and make it use our custom code.

This addresses bug 293702.
<a href="http://bugs.kde.org/show_bug.cgi?id=293702" title="http://bugs.kde.org/show_bug.cgi?id=293702">http://bugs.kde.org/show_bug.cgi?id=293702</a>

Diffs (updated)
kdeui/tests/CMakeLists.txt 63788f6
kdeui/tests/kconfigdialog_unittest.cpp PRE-CREATION
kdeui/dialogs/kconfigdialogmanager.cpp 0890c0b

Diff: <a href="http://git.reviewboard.kde.org/r/103909/diff/diff" title="http://git.reviewboard.kde.org/r/103909/diff/diff">http://git.reviewboard.kde.org/r/103909/diff/diff</a>

Testing
Ran the attached test, everything worked.

Without moving the
userproperty = getUserProperty(w);
the KColorCombo fails

Without adding the
s_propertyMap->insert( "KComboBox", "" );
the editable KComboBox fails

Thanks,

Albert Astals Cid

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/09/2012 - 19:28

(Updated Feb. 9, 2012, 11:28 p.m.)

Review request for kdelibs, Ben Cooksley, Eike Hein, Christoph Feck, and Jeremy Paul Whiting.

Description
<a href="https://git.reviewboard.kde.org/r/101486/" title="https://git.reviewboard.kde.org/r/101486/">https://git.reviewboard.kde.org/r/101486/</a> broke subclasses of QComboBox that have a USER property like KColorCombo, this patch reverts this change and introduces a different code path to ignore the USER property of QComboBox and KComboBox and make it use our custom code.

This addresses bug 293702.
<a href="http://bugs.kde.org/show_bug.cgi?id=293702" title="http://bugs.kde.org/show_bug.cgi?id=293702">http://bugs.kde.org/show_bug.cgi?id=293702</a>

Diffs
kdeui/tests/CMakeLists.txt 63788f6
kdeui/tests/kconfigdialog_unittest.cpp PRE-CREATION
kdeui/dialogs/kconfigdialogmanager.cpp 0890c0b

Diff: <a href="http://git.reviewboard.kde.org/r/103909/diff/diff" title="http://git.reviewboard.kde.org/r/103909/diff/diff">http://git.reviewboard.kde.org/r/103909/diff/diff</a>

Testing
Ran the attached test, everything worked.

Without moving the
userproperty = getUserProperty(w);
the KColorCombo fails

Without adding the
s_propertyMap->insert( "KComboBox", "" );
the editable KComboBox fails

Thanks,

Albert Astals Cid

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/21/2012 - 19:54

(Updated Feb. 21, 2012, 11:54 p.m.)

Review request for kdelibs, Ben Cooksley, Eike Hein, Christoph Feck, and Jeremy Paul Whiting.

Changes
Updated with apaku's suggestion to address Christoph's concerns about derived classes without USER property

Description
<a href="https://git.reviewboard.kde.org/r/101486/" title="https://git.reviewboard.kde.org/r/101486/">https://git.reviewboard.kde.org/r/101486/</a> broke subclasses of QComboBox that have a USER property like KColorCombo, this patch reverts this change and introduces a different code path to ignore the USER property of QComboBox and KComboBox and make it use our custom code.

This addresses bug 293702.
<a href="http://bugs.kde.org/show_bug.cgi?id=293702" title="http://bugs.kde.org/show_bug.cgi?id=293702">http://bugs.kde.org/show_bug.cgi?id=293702</a>

Diffs (updated)
kdeui/dialogs/kconfigdialogmanager.cpp 18bc44e
kdeui/tests/CMakeLists.txt 63788f6
kdeui/tests/kconfigdialog_unittest.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/103909/diff/" title="http://git.reviewboard.kde.org/r/103909/diff/">http://git.reviewboard.kde.org/r/103909/diff/</a>

Testing
Ran the attached test, everything worked.

Without moving the
userproperty = getUserProperty(w);
the KColorCombo fails

Without adding the
s_propertyMap->insert( "KComboBox", "" );
the editable KComboBox fails

Thanks,

Albert Astals Cid

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/21/2012 - 19:55

(Updated Feb. 21, 2012, 11:55 p.m.)

Review request for kdelibs, Ben Cooksley, Eike Hein, Christoph Feck, and Jeremy Paul Whiting.

Description
<a href="https://git.reviewboard.kde.org/r/101486/" title="https://git.reviewboard.kde.org/r/101486/">https://git.reviewboard.kde.org/r/101486/</a> broke subclasses of QComboBox that have a USER property like KColorCombo, this patch reverts this change and introduces a different code path to ignore the USER property of QComboBox and KComboBox and make it use our custom code.

This addresses bug 293702.
<a href="http://bugs.kde.org/show_bug.cgi?id=293702" title="http://bugs.kde.org/show_bug.cgi?id=293702">http://bugs.kde.org/show_bug.cgi?id=293702</a>

Diffs
kdeui/dialogs/kconfigdialogmanager.cpp 18bc44e
kdeui/tests/CMakeLists.txt 63788f6
kdeui/tests/kconfigdialog_unittest.cpp PRE-CREATION

Diff: <a href="http://git.reviewboard.kde.org/r/103909/diff/" title="http://git.reviewboard.kde.org/r/103909/diff/">http://git.reviewboard.kde.org/r/103909/diff/</a>

Testing (updated)
Ran the attached test, everything worked.

Without moving the
userproperty = getUserProperty(w);
the KColorCombo fails

Without adding the propertyIndex stuff the editable KComboBox fails

Thanks,

Albert Astals Cid

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Commit Hook at 02/23/2012 - 18:13

This review has been submitted with commit c27b939ac971f23e8cf8950e76c3f2745c634d72 by Albert Astals Cid to branch KDE/4.8.

- Commit Hook

On Feb. 21, 2012, 11:55 p.m., Albert Astals Cid wrote:

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Christoph Feck at 02/23/2012 - 17:53

Ship it!

Only one small spacing issue:

kdeui/dialogs/kconfigdialogmanager.cpp
<http://git.reviewboard.kde.org/r/103909/#comment8783>

if( x ) => if (x)

- Christoph Feck

On Feb. 21, 2012, 11:55 p.m., Albert Astals Cid wrote:

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Christoph Feck at 02/09/2012 - 20:01

Thanks Albert for looking at it. Not sure if I understand everything correctly, but what happens, when I have a subclass of Q/KComboBox, that does not have its own user property?

I am considering the following possible cases:

1) plain QComboBox
2) subclassed QComboBox without custom user property
3) subclassed QComboBox with custom user property
4) plain KComboBox
5) subclassed KComboBox without custom user property
6) subclassed KComboBox with custom user property (e.g. KColorCombo)

For 1) 2) 4) 5) it should ignore the new 4.8 user property, and use our custom code.
For 3) 6) it should respect the custom user property.

If I am following code paths correctly, the patch fails for cases 2) and 5). It does not find the class name in the map, falls back to user property (what Qt provides now since 4.8), and thus not handle our custom code.

- Christoph Feck

On Feb. 9, 2012, 11:28 p.m., Albert Astals Cid wrote:

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/20/2012 - 17:42

Yes, the code part that starts with the qobject_cast<QComboBox*> has been added to actually support combo boxes without a user property, and that is still what it does.

- Albert

On Feb. 9, 2012, 11:28 p.m., Albert Astals Cid wrote:

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Christoph Feck at 02/12/2012 - 20:39

To me it looks like the code part that starts with the qobject_cast<QComboBox*> has been added to actually support combo boxes without a user property. Qt did not handle them back then, now it does, but differently than KDE handles them.

- Christoph

On Feb. 9, 2012, 11:28 p.m., Albert Astals Cid wrote:

Re: Review Request: Fix KConfigDialogManager fails to handle sub

By Albert Astals Cid at 02/12/2012 - 18:10

"what happens, when I have a subclass of Q/KComboBox, that does not have its own user property?"
I don't think that needs to be a supported use case at all, i.e. if you don't tell KConfigDialogManager how to support your class, how are you expecting it to work? By magic?

Thus 2) and 5) are non-issues in my opinion

- Albert

On Feb. 9, 2012, 11:28 p.m., Albert Astals Cid wrote: