Added support for STRINGS property in CMake cache editor
ClosedPublic

Authored by gracicot on Oct 9 2017, 2:58 AM.

Details

Summary

CMake had support of options with a dropdown choice since version 2.8. This patch add support for displaying cmake variable that has a STRINGS property as an editable
dropdown that mimic the behavior of cmake-gui.

Here's an example of a cmake dropdown option:

set(Color "Red" CACHE STRING
  "Color chosen by the user at CMake configure time")

set(ColorValues "Red;Orange;Yellow;Green;Blue;Violet" CACHE STRING
  "List of possible values for the Color cache variable")

set_property(CACHE Color PROPERTY STRINGS ${ColorValues})

All other info about cmake STRINGS property on this kitware blog post and on the cmake documentation

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
gracicot created this revision.Oct 9 2017, 2:58 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptOct 9 2017, 2:58 AM
gracicot edited the summary of this revision. (Show Details)Oct 9 2017, 3:01 AM
gracicot updated this revision to Diff 20507.Oct 9 2017, 3:53 AM
gracicot edited the summary of this revision. (Show Details)
  • Fix a bug where STRINGS were not filled when cache variable also marked as advanced
apol added a subscriber: apol.Oct 9 2017, 9:11 AM

Why does it need to be in a different column? Am I missing something?

apol added a comment.Oct 9 2017, 9:11 AM

A screenshot would be great, in fact.

In D8215#153611, @apol wrote:

Why does it need to be in a different column? Am I missing something?

This is analogous to the Advanced column. That Column is there so when the delegate reads it, it knows it's a advanced value.

Likewise, since the strings properties are now parsed and the delegate must know about it to display a dropdown, another column has been added to store that property of the CMake variable.

If you find another way to do that please tell me, I'll be glad to edit this patch into a better one.

I'll upload the screenshots soon.

Here's two screenshot of the new ui:


Here's a screenshot of cmake-gui:

kfunk added a comment.Oct 9 2017, 3:29 PM

Do I understand correctly that all those columns are just here for storing data? They're not shown in the end.

Then one should rather refactor this to set the data on each individual item via http://doc.qt.io/qt-5/qstandarditemmodel.html#setData instead of creating (hidden) columns. Even better: Turn this whole CMakeCacheModel into a proper model inheriting from QAbstractItemModel (more work). That way you're inherently required to implement this more cleanly :)

Bonus points for introducing an enum for Name, Type, Value, ....

@gracicot Not your fault of course, you just followed the existing pattern here.

kfunk added a comment.Oct 9 2017, 3:31 PM

So in general I think this code looks good (apart from the bit of code duplication for handling "STRINGS" in cmakecachemodel.cpp). But it'd need to be refactored afterwards...

gracicot updated this revision to Diff 20523.Oct 9 2017, 3:42 PM
  • Remove code duplication in cmakecachemodel.cpp
kfunk accepted this revision.Oct 9 2017, 4:19 PM

LGTM.

Now if someone is willing to refactor this code to use a) enums instead of magic numbers, b) QAbstractItemModel instead of QStandardItemModel that'd be great!

This revision is now accepted and ready to land.Oct 9 2017, 4:19 PM

Can you push that for me? I don't have the rights for pusing. My email is gufideg at gmail.com. Thanks.

This revision was automatically updated to reflect the committed changes.