[Differential] [Updated] D1924: KXMLGui: Fix merge indices when removing xmlgui clients with actions in groups.

[dfaure (David Faure)]

dfaure marked 2 inline comments as done.
dfaure added inline comments.

INLINE COMMENTS

> dhaumann wrote in kxmlgui_unittest.cpp:305
> Interesting, this looks as if there were invalid reads before?

Not exactly. QList's [i] asserts when called out of bounds. It didn't happen before, because when the test passes, actions == expectedActions ;)
It simply happened to me while working on the unittest and getting less actions than expected.

> svuorela wrote in kxmlgui_unittest.cpp:306
> Isn't it better to move the last QCOMPARE(count,count); up first?  Or is it to  be able to easier debug if something went wrong?

Right it's at the end because when "A,B,C" is matched against "A,D,B,C" I'd rather be told "at position 1 I expected D but got B" than "expected 3, got 4". Easier to debug.

> dhaumann wrote in kxmlgui_unittest.cpp:411
> Is this qDebug() intentional?

It was, but ok, let's remove it.

> svuorela wrote in kxmlguibuilder.h:83
> And use what instead? maybe mark deprecated?

And use nothing, it was only used internally and it's not used anymore.

AFAICS nobody uses this, but yes, good idea to add deprecation macro to make sure it stays that way.

> dhaumann wrote in kxmlguifactory_p.cpp:717
> I'm not sure I get this in detail: but as you already mentioned yesterday, it seems this saves iterators. It seems to me this code was written at a time (?) when QList iterators were stable, but since a QList behaves similar to a QVector for ints (iirc), this is rather dangerous, right? I guess that is what you imply with your comment here. In that case, it's a wonder all this is still working ;)

I actually looked at the kdelibs3 code and it was using QValueList already ;)

Yes it's dangerous to store iterators into a QList or QVector, however what this code does (right under this comment) is to recalculate these two iterators when adding to the QList, which solves exactly that risk. My commit simply makes that more explicit in the comment.

Hmm, I forgot to check for code _removing_ from the list. Although I doubt QList reallocates when shrinking. Checked now, there isn't.

BTW I also searched bugzilla for crashes related to ContainerNode and BuildHelper and didn't find anything ;)

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D1924

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dfaure, svuorela, dhaumann
Cc: kde-frameworks-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160617/ee3ec077/attachment.html>