Different solution: Push the patch in 5.3 with i18n() replaced by plain QString() and only in master use i18n() again? Sounds like this patch should rather hit 5.3.0, otherwise the plugin is unusable?

Myself has no objections, rather positive on it.

@kossebau Are you Ok with this change? I'm not sure I'll get a review from the domain experts, but I'm fairly confident now that the changes are right.

In D15694#333087, @rjvbb wrote:

My point here was that making a whatever-you-call it more or less obligatory in something that is (used to be) shared with a closely related language introduces unnecessary confusion ("wait, am I writing C or C++ here"). And extra overhead when reusing algorithms between those languages.

Sorry, you are right. I forgot the most important part of the patch. Sorry for the noise :(

And on openSUSE TW for me:

• CLANG_INCLUDE_DIRS: /usr/include (no cpuid.h below that)
• @CLANG_LIBRARY_DIRS@/clang/@CLANG_VERSION@/include: /usr/lib64/clang/6.0.1/include (has cpuid.h in that very dir)

Note: This is likely the wrong fix to this problem.

And what does CLANG_INCLUDE_DIRS resolve to on Gentoo?

Does it work with clang 6.0?

Another fix for the generic Makefile project manager; I had missed the fact that URLInfo::isDir is undefined when URLInfo::isValid is false. This lead to personalised project files of the type `/path/to/projectFoo/projectFoo.kdev4/customname.kdev4```.

Updated as discussed. This variant is less different from stock code than my previous proposal. It complements the current checks with a comparison using canonicalised paths. As a result it should catch all cases that are not caught by either one of the comparisons when performed in isolation.

I think there might be a simpler solution to that situation

In D15565#333756, @antonanikin wrote:

Is there a chance those aspects could be split out into separate patches?
Also still hoping for a variant of the bug fix patch which does not need a string freeze break, if possible.

Hi, Friedrich, sorry for delay. I think we should push it as is. Since current (wrong) logic of plugin controller we can have execute plugin unloaded with heaptrack loaded in same time. But execute plugin is really needed for plugin work so to don't break strings freeze we can only silently stop heaptrack analysis which is wrong and ugly. I think user should receive normal feedback.

So I suggest wait for 5.3 release and push this after. The patch will be available at 5.3.1 correcting release. Your opinion?

Is there a chance those aspects could be split out into separate patches?
Also still hoping for a variant of the bug fix patch which does not need a string freeze break, if possible.

Hi, René. Thanks for patch, but your new version also produce new problems :)

The QSet include is redundant now.

Updated with requested changes.

Updated as discussed: any previous project settings file in .kdev4 will be removed if and before overriding/writing it (instead of simply trashing the entire .kdev4 directory).

@sagnikchaudhuri: Hi, how are you? Still busy exploring the wonderful world of Qt? :)

This looks good to me overall, but see a few inline comments.

I'm doing it now and will test it as part of this patch; afterward there are 3 options

A proper solution would be to delete just the settings file, if overwriting is to be done.

Attributes are not keywords — they're a (C++-specific) generic syntax that was introduced with C++11.
Again, the idea is not starting from scratch, but introducing small changes that reduce the number of errors programmers

In any situation where we'd already be overwriting the settings file but where there are other projects defined in the same sourcetree. The summary of this DR argues how and why that could be the case.

I don't quite get it. In which situation do we want to override the project (delete .kdev file), but do not clear .kdev dir?

(In any case I don't see how it could *not* be recreated given that it will hold the new settings file).

Attributes are not keywords — they're a (C++-specific) generic syntax that was introduced with C++11. There are numerous compiler-specific attributes, usually prefixed with the compiler name, and some standardized attributes. See cppreference.com for a list of those.

This fixes a mix-up that slipped through and above all, puts deleting the .kdev4 under user control.
Apparently there are situations where this directory should be deleted because its presence (empty) can confuse the project manager when overwriting an existing project (?!, T6262). /Methinks that's something that must be fixed in the confused place(s), not worked around by removing .kdev4. The whole point of this patch is to allow the creation of multiple projects in a single source tree, after all.

Exactly how can the project manager be confused when you don't remove the .kdev4 directory, knowing that it will be recreated almost immediately after deleting it? (In any case I don't see how it could *not* be recreated given that it will hold the new settings file).

You can use comments <https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/>, or `__attribute__((fallthrough))` in C.

In D15694#330932, @rjvbb wrote:

My gripe is more with having to change my habits in ways I don't necessarily agree with entirely, and which might interfere with coding in other contexts.
It may not in KDevelop's code, but that's not all that all of us work on. Coding style and guidelines for KDevelop itself could reflect good practice you'd like to advocate in general.

Using Q_UNREACHABLE

Final update with requested changes.

Given that languageOption also has Q_UNREACHABLE(), and both are always called together, any crash that could happen would have already happened before. The function is called from CompilerProvider::{defines,includes} and both catch the Other case before calling into the ICompiler implementation. Maybe we should document the constraint (type != Utils::Other) in the ICompiler interface, but we should do it in a separate commit.

Besides the two nitpicks, happy with the result, good work. Works here as intended, across multiple styles I tested (also at runtime).

Latest version still doesn't restore the crash for me, so LG.

__builtin_unreachable() does have strange effects:

Pass style parent in the constructor for Ideal Layout, and install event filter
on it to monitor style changes and set spacing.

In D15625#332551, @kossebau wrote:

When it comes to updating the spacing on style changes, something like this should work:

void IdealButtonBarLayout::changeEvent(QEvent* event)
{
    QBoxLayout::changeEvent(event);
    if (event->type() == QEvent::StyleChange) {
        setSpacing(buttonSpacing());
    }
}

I think this could be Q_UNREACHABLE().

Patch to move the key set as a member:

diff --git a/plugins/externalscript/externalscriptplugin.cpp b/plugins/externalscript/externalscriptplugin.cpp
index e1009457c8..3f2bddeeff 100644
--- a/plugins/externalscript/externalscriptplugin.cpp
+++ b/plugins/externalscript/externalscriptplugin.cpp
@@ -120,6 +120,7 @@ ExternalScriptPlugin::ExternalScriptPlugin( QObject* parent, const QVariantList&
       item->action()->setShortcut( QKeySequence( script.readEntry( "shortcuts" ) ) );
       item->setShowOutput( script.readEntry( "showOutput", true ) );
       m_model->appendRow( item );
+      m_keySet.insert( script.name() );
     }
   }
   //END load config
@@ -350,6 +351,7 @@ void ExternalScriptPlugin::rowsAboutToBeRemoved( const QModelIndex& /*parent*/,
     const ExternalScriptItem* const item = static_cast<ExternalScriptItem*>( m_model->item( row ) );
     KConfigGroup child = config.group( item->key() );
     qCDebug(PLUGIN_EXTERNALSCRIPT) << "removing config group:" << child.name();
+    m_keySet.remove( item->key() );
     child.deleteGroup();
   }
   config.sync();
@@ -361,7 +363,10 @@ void ExternalScriptPlugin::updateItem( const ExternalScriptItem* item )
   Q_ASSERT( index.isValid() );

Updated with requested changes.

I didn't test it, but it looks fine to me.

Looks good to me in general, from what I tested works as intended (though I cannot test the crash as before, never had it reproduced)
Some code tweaking might be nice to have, see inline comments.

Updated as suggested: handle all known language types in languageStandard()

IdealButtonBarLayout is now derived from QBoxLayout and delegates all operations to it.

reuse the C(++) parser options for ObjC(++).

• Do we want a -std= flag, does Clang even accept that?

So I think there are two questions to decide here:

• Do we want a -std= flag, does Clang even accept that?
• Should we just take the ObjC flags from the C flags, and ObjC++ flags from the C++ flags, or should they be set independently?

Removed left-over x-objchdr reference (sorry about that).

Removed the x-objchdr mimetype (to avoid additional confusion what a .h file could be) and added my current x-objc+src definition to kdevclang.xml .

In D15625#332028, @amhndu wrote:

Which could happen independently of us providing proper minimumSizeHints, by code that is broken or confused (think custom styles) or otherwise forced to go beyond our hints. The API dox of QLayoutItem::setGeometry(const QRect &r) does not forbid to set rects smaller than the minimumSizeHint() or even a 0 size rect.

From http://doc.qt.io/qt-5/qlayout.html#details:

You should also implement minimumSize() to ensure your layout isn't resized to zero size if there is too little space.

And http://doc.qt.io/qt-5/qlayout.html#SizeConstraint-enum

From my understanding, if we set the size constraint in the layout (analogous to how we set it in the tool button, but I missed setting it here in layout) appropriately, Qt classes will follow it. And like the original bug, would resize the window if necessary.
Though I'm not against doing a rect.width() < minimumSize().width check in the doHorizontalLayout, even if redundant, which should be cheap.

Which could happen independently of us providing proper minimumSizeHints, by code that is broken or confused (think custom styles) or otherwise forced to go beyond our hints. The API dox of QLayoutItem::setGeometry(const QRect &r) does not forbid to set rects smaller than the minimumSizeHint() or even a 0 size rect.

Still, the crash as I have understood is roots in IdealButtonBarLayout::setGeometry(QRect) being called with some zero rect.

So let's first please do some effort to have a MIME type id agreed on by others (ideally this would be something done in the Objective-C++ community where you might have your contacts to poke)

In D15530#330911, @rjvbb wrote:

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.

I don't understand, that sounds like a contradiction. As soon as s-m-i has some version accepted we could stop providing our own code (when that version has become common in distroland), is that what you wanted to say?

In D15625#331810, @rjvbb wrote:

I am slighty uneasy with the explosion of code we add now

Seems to me you're the one who asked for it ;)

I am slighty uneasy with the explosion of code we add now

In D15625#331307, @amhndu wrote:

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.

Refactored external script to store the config label inside the model
as discussed with @flherne on irc
Inserting/Deleting/Updating scripts now works as expected.

I couldn't find anything to move a group or rename a group, so I'm copying and deleting.
Is there a better way ?