Note: Reported as https://bugs.kde.org/show_bug.cgi?id=399048 -- @Rene: Please push your fix(?) -- looks legit.

Some styling fixes.

Looks like you nailed it this time.

Forgot to update one qCWarning into qCDebug.

Re-implemented Ideal Layout's minimumSize which accumulates the newly added
minimumSize in Ideal Button.
The previous buggy implementation of minimumSize in the Ideal Layout called the doVerticalLayout
with a zero rect, although the height is passed from a property, but it is never set to anything
non-zero, which is what caused problems with some QtCurve configs.

Then I don't understand the question.

It appears this bug is because minimumSize calls doVerticalLayout with a zero rect

Then I don't understand the question.

I don't see how that answers my question?

If I may: can someone please tell me why it should be impossible that only small items are found, IOW why there would also have to be something to shrink?

Ideally, Layout::sizeHint should tell us the total space the buttons would occupy (given that it sums up the button size hints), comparing that to the rect should make it make it possible to predict.
I've been assuming if the buttons were to all fit, rect.height() should be equal to sizeHint().height()

If I may: can someone please tell me why it should be impossible that only small items are found, IOW why there would also have to be something to shrink?

In D15625#331133, @amhndu wrote:

The reasoning was that Items that have maximumWidth size needn't be shrinked, as that's the maximum size that's allowable.

See also https://bugs.kde.org/show_bug.cgi?id=399025

Note: Seems to cause https://bugs.kde.org/show_bug.cgi?id=399025

Can you guarantee that rect.width (height) will never be equal to the RHS in the equation from my previous reply?

are you sure the comparison should be itemWidth <= maximumWidth and not a simple < instead?

The reasoning was that Items that have maximumWidth size needn't be shrinked, as that's the maximum size that's allowable.

Another thought: are you sure the comparison should be itemWidth <= maximumWidth and not a simple < instead? That ought to address the situation I run into, though I still don't believe it'd be a guarantee that there will never be other situations where all items are considered to be small items.

Interestingly, I'm getting the same issue on Linux (with the same QtCurve style though a slightly different font selection).
Only here it's maximumHeight that is concerned, not the width.

if you want to bite in deeper I can share my settings

In D15625#331064, @rjvbb wrote:

How exactly is maximumWidth calculated to be 0?

How would I know?

How exactly is maximumWidth calculated to be 0?

In D15625#331028, @rjvbb wrote:

It works and can go in with just one minor change request.

I still need the fallback protection though. I added some additional debug output (see below) which shows what is really going on for me: maximumWidth is calculated to be 0. I haven't tried to figure out if that's an anomaly, but it explains why I'm getting only small items.

This happens only with my QtCurve style; if you want to bite in deeper I can share my settings (and fonts, if needed).

It works and can go in with just one minor change request.

In D15625#329398, @rjvbb wrote:

Looks good at first glance. The failsafe should work evidently but I'll have to test to be certain if the other fix prevents the situation I ran into. Maybe this afternoon, otherwise tomorrow.

As discussed earlier, switch statements do have a weird syntax and I'm completely with you if you don't like it. They are basically a glorified `goto` cascade. The case labels actually behave like `goto` labels in a lot of ways, which is the basis of oddities like Duff's device

When it comes to no `break;` after a `return;`, that also matches the idea to not have an `else` after an `return` in the branch before.

Sure, I am mainly interested here that we use MIME type ids which also accepted elsewhere, and not invent our own here and ignore the rest of the world :)
Once e.g. shared-mime-info has some version accepted, we can then provide fall-back variants in the kdevelop s-m-i files installed.

After testing the GitHub provider and the QmlJS code completion I'm somewhat confident that these changes are correct, but I want to give the experts on that code a chance to weigh in before I merge this. Now onto the discussion.

In D15530#330605, @rjvbb wrote:

Doesn't KDevelop already add (install) a few mimetypes of its own? I think I mentioned before that I do have a .xml file for SMI, adding the x-objc++src definition and I presume that's why I am not seeing mimetype warnings about ObC (I'm seeing others though.)

Installing our own is the best way to be certain the definition is there - cf. the discussion around the astyle library and e.g. Debian Stable.

In D15694#330761, @rjvbb wrote:

I'll point out that we're no longer living with Q_FOREACH and that nothing forbids anyone to provide a less conspicuous pattern than Q_FALLTHROUGH.

Can you explain in more detail how an unreachable `break` after a `return` statement makes the code more readable?

In D15694#330687, @rjvbb wrote:

As mentioned on another ticket, I don't like the idea of not putting breaks, no matter how clever the compiler and hard-coded choice of compiler options.

As mentioned on another ticket, I don't like the idea of not putting breaks, no matter how clever the compiler and hard-coded choice of compiler options; I don't think you gain anything from it, and it doesn't improve code readability either for me (esp. when switch cases aren't indented w.r.t the switch context, as in the last hunk in plugins/qmljs/codecompletion/context.cpp.
Making it obligatory to add a token confirming that fallthrough is intended is a good idea, though using a largish term in all-caps like Q_FALLTHROUGH also doesn't improve readibilty. And what about the most common type of fallthrough, are we really going to have to write something like the following?

Remove comment in plugins/ghprovider/ghproviderwidget.cpp, as the fallthrough is probably intended, and add break in plugins/qmljs/codecompletion/context.cpp, as it is probably not intended. Now the code completion doesn't suggest variables of the same type as the last parameter in functions calls, which seems about right.

Doesn't KDevelop already add (install) a few mimetypes of its own? I think I mentioned before that I do have a .xml file for SMI, adding the x-objc++src definition and I presume that's why I am not seeing mimetype warnings about ObC (I'm seeing others though.)

In D15694#330503, @kossebau wrote:

Yes, needs to be included for such source file, because the min Qt dependency version is still 5.5, and Q_FALLTHROUGH was only introduced in 5.8 (see note at http://doc.qt.io/qt-5/qtglobal.html#Q_FALLTHROUGH)

As mentioned on https://phabricator.kde.org/D15532#330528, text/x-objchdr & text/x-objc++src are MIME types no-where defined from what I saw. Which makes things irritating for future code readers, as well as resulting in annpying warnings in the runtime log.
Please keep those MIME types out of this patch, until that has been resolved.

Closing this, as patch has been abandoned in favour of (landed) D14870

In D15532#326462, @kossebau wrote:

Thanks. Learned about ObjC++ this way... the world is full of strange things :)

New version also checks launch configuration type and starts analysis only for native applications.

In D15694#330222, @aaronpuchert wrote:

@kossebau It seems you did something similar in D6301, maybe you can review this change. Should I also include qtcompat_p.h when Q_FALLTHROUGH is used?

In D15532#330269, @rjvbb wrote:

I have no (big) problems with a fallthrough attribute, but if it's going to be obligatory I'd want the complementary attribute (the break) to be obligatory too...

I have no (big) problems with a fallthrough attribute, but if it's going to be obligatory I'd want the complementary attribute (the break) to be obligatory too...

@kossebau It seems you did something similar in D6301, maybe you can review this change. Should I also include qtcompat_p.h when Q_FALLTHROUGH is used?

There are two suspicious fallthroughs, maybe someone with a better understanding than me can comment on that.

In D15532#330024, @rjvbb wrote:

I prefer to remind myself of such things and that I'm putting it there anyway (because the compiler won't be able to tell me I really forgot to use one, not unless you make some kind of "no break here" statement obligatory).

In D15530#329968, @aaronpuchert wrote:

You should probably also add a few lines to the custom-definesandincludes plugin.

Eventually we have to compile our code anyway, so aren't we all dependent on the compiler?

In D15530#329377, @rjvbb wrote:

You should probably also add a few lines to the custom-definesandincludes plugin.

In a different review I presume?

In D15532#329308, @rjvbb wrote:

We could activate -Wunreachable-code so the compiler tells you where break isn't needed.

Evidently I was talking about a general principle, not about introducing a dependency on the compiler telling you what (not) to do. If I wanted that I'd be coding in Modula-2 ...

• use custom explicit checkToolsInPath code for qdbus variants
• avoid error print from "which" in kdevelop!

In D15606#328516, @fvogt wrote:

In D15606#328495, @kossebau wrote:

In D15606#328493, @fvogt wrote:

IMO it should just check for both and prefer qdbus-qt5 if available.

Yes, that was my starting patch, but then I got unsure about the great picture.
If going this route, any shell script wizard proposal how to change checkToolsInPath in kdevplatform_shell_environment.sh to check for both variants, without adding too much boilerplate code?

What about this:

qdbus_exe=$(which qdbus-qt5 qdbus 2>/dev/null | head -n1)

if [ -z "${qdbus_exe}" ]; then checkToolsInPath qdbus-qt5; fi

$qdbus_exe $KDEV_DBUS_ID etc.

Oh yes, sorry about that, they say misleading documentation is worse than no documentation :D

Refactored for the 5.3 branch + completed.

Thanks. Patch fine with me, feature still works.
(and now that I know about it I also see the unused modulo pixels from the maxheight calculation ;) something to be solved on a boring day in need for entertainment)

Add requested comments and warning

"If you have inherited this class to access the formatter, you will need to add a method similar to getPeekStart() in the ASStreamIterator class in astyle_main.h." from http://astyle.sourceforge.net/news.html is the only info I have, which leaves lots of room.
Have you seen any docs for that method?

In D15605#329363, @rjvbb wrote:

Would this be OK as a fix?

Updated as suggested. No switch yet but a layout that looks more like one.

Looks good at first glance. The failsafe should work evidently but I'll have to test to be certain if the other fix prevents the situation I ran into. Maybe this afternoon, otherwise tomorrow.

You should probably also add a few lines to the custom-definesandincludes plugin.

Would this be OK as a fix?

Is there some API dox what one should implement for getPeekStart(), especially which values are valid?

We could activate -Wunreachable-code so the compiler tells you where break isn't needed.

With asserts enabled, I get a crash with this on opening the settings or sometimes only when selecting Objective-C in the formatting settings

kdevelop: /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/astyle/3rdparty/libastyle/ASFormatter.cpp:3841: std::__cxx11::string astyle::ASFormatter::peekNextText(const string&, bool, std::shared_ptr<astyle::ASPeekStream>) const: Assertion `sourceIterator->getPeekStart() == 0 || streamArg != nullptr' failed.

In D15532#328829, @rjvbb wrote:

C is not a subset of C++ (let me point to Mr. Stroustrup who should know it :)

I wouldn't call it that either (it'd be degrading for C ;)), but C++ does inherit from C (and was a layer on top of C in its early days, IIRC with a sort of preprocessor).

In fact, Mr Stroustrup does say basically the same thing:

Except for a few examples [...], C++ is a superset of C.

You should probably also add a few lines to the custom-definesandincludes plugin.

A small additional change to the plugin json, and a request for a bit of help.

Updated diff solves the buttonSpacing issue, but leaving the check for any future edge-cases I might've missed

Please also fix the possibly actual bug, which is

const int maximumWidth = rect.width() / _items.size();

and its counterpart not taking the button spacing into account.

Thanks for looking into this!

C is not a subset of C++ (let me point to Mr. Stroustrup who should know it :)

In D15532#328812, @rjvbb wrote:

... shared-mime-info has this questionable inheritage for C++ and Objective-C from C.

I don't think that's so questionable; doesn't the C++ entry also inherit from C? That would be logical at least historically speaking (and properly written C can still be compiled as C++ AFAIK).

... shared-mime-info has this questionable inheritage for C++ and Objective-C from C.

In D15532#328544, @kossebau wrote:

In D15532#328524, @kossebau wrote:

For some reason the preview uses the "C" highlighting mode for me when ObjC is selected, so the preview has some highlighting issues. That seems a bug with KTextEditor I have to explore more.

Nope, this is actually some bug in KDevelop which should be fixed first, somewhere in`SourceFormatterStyle::modeForMimetype(...)` the wrong mode is chosen. Still digging.

Updated as requested, waiting for final a-ok.

Please update the Diff according to our requests, then I think this can still go into 5.3 branch. Friedrich, you still agree this could go into 5.3? :)

Go for it, please push to 5.3 branch.