Adding new parameters is unfortunately Binary Incompatible so this can't go in this way, for it to work you'd need to add a new function with all the parameters and then call one from the other.

Maybe what we need is one that takes two bools we should have one that takes flags so we can add more flags in the future as needed

I have to admit, I don't know how to implement a version with flags correctly.

Did I understand you correctly in that an overload with signature

addPage(QWidget *page, const QString &itemName, const QString &pixmapName = QString(), const QString &header = QString(), bool manage = true, bool fitContentHorizontally = false)

would be ok as well? This would then be called by the overload without the last option (i.e. the addPage that existed before this diff).

For the second version

addPage(QWidget *page, KCoreConfigSkeleton *config, const QString &itemName, const QString &pixmapName = QString(), const QString &header = QString(), bool fitContentHorizontally = false)

I would then do the same.

In D16137#341984, @kadabash wrote:

I have to admit, I don't know how to implement a version with flags correctly.

Look at kcmodule.h for example you'll see some flags defined there, then there's a setButtons function, this would be the same but to the addPage function

Did I understand you correctly in that an overload with signature

addPage(QWidget *page, const QString &itemName, const QString &pixmapName = QString(), const QString &header = QString(), bool manage = true, bool fitContentHorizontally = false)

would be ok as well? This would then be called by the overload without the last option (i.e. the addPage that existed before this diff).

I'm not really the kconfigwidgets maintainer so i can't say if that would be acceptable API wise, but it would be ok binary compatibility wise, yes.

It would though not compile because would mean you have two functions

addPage(QWidget *page, const QString &itemName, const QString &pixmapName = QString(), const QString &header = QString(), bool manage = true)
addPage(QWidget *page, const QString &itemName, const QString &pixmapName = QString(), const QString &header = QString(), bool manage = true, bool fitContentHorizontally = false)

which one would be called when i call

addPage(someWidget, "myItemName")

the first or the second?

To solve that you need to make your new function not have default values, but as said i'm not sure if that's ok API wise, personally i think using flags is nicer

Sorry for the duplicate updates, I messed up my Arcanist commands.

Thanks for your help so far, I tried to follow your suggestions in the above change.

from a source & binary compatibility point of view, this looks great.

I guess we should maybe plan deprecating (and at kf6 time remove) the old ones ? (But this is not an objection to the current change)

I'm not a KConfigWidgets maintainer, so I will not right now say shipit. But if no objections by others until wed or thursday, consider this an accept.

+1, nice to see this. Once it lands, feel free to start porting apps to use it--for the ones where it'll be acceptable to depend on Frameworks 5.52 as a minimum version.

That's almost good, but the flag name should be more general, think like for example you would remove the manage bool and convert it to a flag too, so that if it the future someone needs to add a new flag, they can add it to that enum and don't need a new parameter.

Am I making sense?

In D16137#342973, @aacid wrote:

That's almost good, but the flag name should be more general, think like for example you would remove the manage bool and convert it to a flag too, so that if it the future someone needs to add a new flag, they can add it to that enum and don't need a new parameter.

Am I making sense?

Did you imagine something like what I changed in D16137#343060 (the latest change) ?

Did you imagine something like what I changed in D16137#343060 (the latest change) ?

I think Albert imagined something like that. It is at least what I imagined when I read his comments. I think I also like it better.

Yep, this looks more like it :)

You're missing @since markers (in the documentation of the new functions) for the KF5 version this will be introduced, but i'd say let's wait for someone to give the final approval and then you remember to commit the correct version number for them?

In D16137#346032, @kadabash wrote:

Vertical fitting only occurs on displays that Qt deems large enough.
Displays that are just high enough to fit the content vertically
are apparently sometimes not considered large enough.

This is https://bugreports.qt.io/browse/QTBUG-3459

We already put in a hack to work around this behavior once in D15406: Manually resize KCMUtilDialog to sizeHint(). I think it's be fine to do the same thing here, as long as we write a comment explaining why that mentions the Qt bug report.

While adapting the settings window in Cantor to this proposed change I have discovered that it influences the sizeHint() of other (non-modified) pages in the same window. This leads to a worse layout than before.

As I cannot currently find the source of that behaviour, this change should probably not be considered for merging.