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

[dhaumann (Dominik Haumann)]

dhaumann accepted this revision.
dhaumann added a comment.
This revision is now accepted and ready to land.


  I think this patch looks good (given we sat in front of this issue together, I think I also know what's going on). There are still open questions, but that's rather unrelated to this patch.
  
  Tested with this patch in Kate, it seems to work. All kxmlgui unit tests still pass.
  
  I'm fine with committing. And btw, thanks a lot!

INLINE COMMENTS

> kxmlgui_unittest.cpp:305
>      for (int i = 0; i < expectedActions.count(); ++i) {
> -        QAction *action = actions[i];
> +        if (i >= actions.count())
> +            break;

Interesting, this looks as if there were invalid reads before?

> kxmlgui_unittest.cpp:411
> +    for (int i = 0 ; i < 5 ; ++i) {
> +        qDebug() << "addClient, iteration" << i;
> +        factory.addClient(&partClient);

Is this qDebug() intentional?

> kxmlguifactory_p.cpp:717
> +    // re-calculate the running default and client merging indices
> +    // (especially important in case the QList data got reallocated due to growing!)
>      m_state.currentDefaultMergingIt = parentNode->findIndex(defaultMergingName);

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 ;)

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/05238367/attachment.html>