From kde-commits Tue Mar 31 21:53:31 2009 From: David Faure Date: Tue, 31 Mar 2009 21:53:31 +0000 To: kde-commits Subject: KDE/kdelibs/kdeui Message-Id: <1238536411.467538.32669.nullmailer () svn ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-commits&m=123853642832496 SVN commit 947617 by dfaure: will Stephenson found an interesting bug in kxmlgui, where wouldn't be merged with the uistandards . Tag name comparison has to be done case-insensitively, since for some reason xmlgui was made [somewhat] case insensitive. M +13 -0 tests/kxmlgui_unittest.cpp M +33 -37 xmlgui/kxmlguiclient.cpp M +1 -1 xmlgui/kxmlguiversionhandler.cpp --- trunk/KDE/kdelibs/kdeui/tests/kxmlgui_unittest.cpp #947616:947617 @@ -436,6 +436,19 @@ << (QStringList() << "file_open" << "options_configure_toolbars" << "foo_action") << (QStringList() << "file" << "foo" << "settings"); + QTest::newRow("Bille's testcase: menu patch + menu edit") + << xmlBegin + "\n" + + "\n" + + xmlEnd + << (QStringList() << "file_open" << "patch_generate" << "edit_foo") + << (QStringList() << "file" << "edit" << "patch"); + QTest::newRow("Bille's testcase: menu patch + menu edit, lowercase tag") + << xmlBegin + "\n" + + "\n" + + xmlEnd + << (QStringList() << "file_open" << "patch_generate" << "edit_foo") + << (QStringList() << "file" << "edit" << "patch"); + // Tests for noMerge="1" QTest::newRow("noMerge empty file menu, implicit settings menu") << xmlBegin + "\n" + xmlEnd --- trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiclient.cpp #947616:947617 @@ -272,6 +272,11 @@ setXMLGUIBuildDocument( QDomDocument() ); } +// if (equals(a,b)) is more readable than if (a.compare(b, Qt::CaseInsensitive)==0) +static bool equals(const QString& a, const QString& b) { + return a.compare(b, Qt::CaseInsensitive) == 0; +} + bool KXMLGUIClientPrivate::mergeXML( QDomElement &base, QDomElement &additive, KActionCollection *actionCollection ) { static const QString &tagAction = KGlobal::staticQString( "Action" ); @@ -296,8 +301,6 @@ base.parentNode().replaceChild(additive, base); base = additive; } else { - QString tag; - // iterate over all elements in the container (of the global DOM tree) QDomNode n = base.firstChild(); while ( !n.isNull() ) @@ -307,12 +310,11 @@ if (e.isNull()) continue; - tag = e.tagName(); + const QString tag = e.tagName(); // if there's an action tag in the global tree and the action is // not implemented, then we remove the element - if ( tag == tagAction ) - { + if (equals(tag, tagAction)) { const QString name = e.attribute(attrName); if (!actionCollection->action(name) || !KAuthorized::authorizeKAction(name)) @@ -325,18 +327,16 @@ // if there's a separator defined in the global tree, then add an // attribute, specifying that this is a "weak" separator - else if ( tag == tagSeparator ) - { + else if (equals(tag, tagSeparator)) { e.setAttribute( attrWeakSeparator, (uint)1 ); // okay, hack time. if the last item was a weak separator OR // this is the first item in a container, then we nuke the // current one QDomElement prev = e.previousSibling().toElement(); - if ( prev.isNull() || - ( prev.tagName() == tagSeparator && !prev.attribute( attrWeakSeparator ).isNull() ) || - ( prev.tagName() == tagText ) ) - { + if (prev.isNull() || + (equals(prev.tagName(), tagSeparator) && !prev.attribute(attrWeakSeparator).isNull() ) || + (equals(prev.tagName(), tagText))) { // the previous element was a weak separator or didn't exist base.removeChild( e ); continue; @@ -346,8 +346,7 @@ // the MergeLocal tag lets us specify where non-standard elements // of the local tree shall be merged in. After inserting the // elements we delete this element - else if ( tag == tagMergeLocal ) - { + else if (equals(tag, tagMergeLocal)) { QDomNode it = additive.firstChild(); while ( !it.isNull() ) { @@ -356,7 +355,7 @@ if (newChild.isNull() ) continue; - if ( newChild.tagName() == tagText ) + if (equals(newChild.tagName(), tagText)) continue; if ( newChild.attribute( attrAlreadyVisited ) == attrOne ) @@ -372,7 +371,7 @@ // the global file. if it does, then we skip it as it will // be merged in, later QDomElement matchingElement = findMatchingElement( newChild, base ); - if ( matchingElement.isNull() || newChild.tagName() == tagSeparator ) + if (matchingElement.isNull() || equals(newChild.tagName(), tagSeparator)) base.insertBefore( newChild, e ); } } @@ -381,18 +380,19 @@ continue; } + else if (equals(tag, tagText)) { + continue; + } + else if (equals(tag, tagMerge)) { + continue; + } + // in this last case we check for a separator tag and, if not, we // can be sure that it is a container --> proceed with child nodes // recursively and delete the just proceeded container item in // case it is empty (if the recursive call returns true) - else if ( tag != tagMerge ) - { - // handle the text tag - if ( tag == tagText ) - continue; - + else { QDomElement matchingElement = findMatchingElement( e, additive ); - if ( !matchingElement.isNull() ) { matchingElement.setAttribute( attrAlreadyVisited, (uint)1 ); @@ -452,8 +452,8 @@ // do one quick check to make sure that the last element was not // a weak separator QDomElement last = base.lastChild().toElement(); - if ( (last.tagName() == tagSeparator) && (!last.attribute( attrWeakSeparator ).isNull()) ) - { + if (equals(last.tagName(), tagSeparator) && + (!last.attribute(attrWeakSeparator).isNull())) { base.removeChild( last ); } } @@ -473,8 +473,7 @@ const QString tag = e.tagName(); - if ( tag == tagAction ) - { + if (equals(tag, tagAction)) { // if base contains an implemented action, then we must not get // deleted (note that the actionCollection contains both, // "global" and "local" actions @@ -483,8 +482,7 @@ break; } } - else if ( tag == tagSeparator ) - { + else if (equals(tag, tagSeparator)) { // if we have a separator which has *not* the weak attribute // set, then it must be owned by the "local" tree in which case // we must not get deleted either @@ -496,14 +494,12 @@ } } - else if ( tag == tagMerge ) - { + else if (equals(tag, tagMerge)) { continue; } // a text tag is NOT enough to spare this container - else if ( tag == tagText ) - { + else if (equals(tag, tagText)) { continue; } @@ -535,16 +531,16 @@ if (e.isNull()) continue; + const QString tag = e.tagName(); // skip all action and merge tags as we will never use them - if ( ( e.tagName() == tagAction ) || ( e.tagName() == tagMergeLocal ) ) - { + if (equals(tag, tagAction) + || equals(tag, tagMergeLocal)) { continue; } // now see if our tags are equivalent - if ( ( e.tagName() == base.tagName() ) && - ( e.attribute( attrName ) == base.attribute( attrName ) ) ) - { + if (equals(tag, base.tagName()) && + e.attribute(attrName) == base.attribute(attrName)) { return e; } } --- trunk/KDE/kdelibs/kdeui/xmlgui/kxmlguiversionhandler.cpp #947616:947617 @@ -38,7 +38,7 @@ QList toolbars; QDomElement parent = doc.documentElement(); for (QDomElement e = parent.firstChildElement(); !e.isNull(); e = e.nextSiblingElement()) { - if (e.tagName() == "ToolBar") { + if (e.tagName().compare(QLatin1String("ToolBar"), Qt::CaseInsensitive) == 0) { toolbars.append(e); } }