Re: Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/#review10817 --- This review has been submitted with commit 576e13d355c34858e8a254a28a100a8b9f7876a8 by Albert Astals Cid to branch KDE/4.8. - Commit Hook On Feb. 20, 2012, 10:33 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- (Updated Feb. 20, 2012, 10:33 p.m.) Review request for kdelibs and David Faure. Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs - kdeui/widgets/ktoolbar.h 69c482e kdeui/widgets/ktoolbar.cpp d17ff39 kdeui/xmlgui/kxmlguibuilder.cpp 6773c31 kdeui/xmlgui/kxmlguifactory_p.cpp 2f81f18 Diff: http://git.reviewboard.kde.org/r/103812/diff/ Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
Re: Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- (Updated Feb. 20, 2012, 10:15 p.m.) Review request for kdelibs and David Faure. Changes --- New patch is up, not sure i like much the new code in BuildHelper::processContainerElement but it is the only way i found to do what you asked for. Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs (updated) - kdeui/widgets/ktoolbar.h 69c482e kdeui/widgets/ktoolbar.cpp d17ff39 kdeui/xmlgui/kxmlguibuilder.cpp 6773c31 kdeui/xmlgui/kxmlguifactory_p.cpp 2f81f18 Diff: http://git.reviewboard.kde.org/r/103812/diff/ Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
Re: Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- (Updated Feb. 20, 2012, 10:22 p.m.) Review request for kdelibs and David Faure. Changes --- Another minor whitespace change Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs (updated) - kdeui/widgets/ktoolbar.h 69c482e kdeui/widgets/ktoolbar.cpp d17ff39 kdeui/xmlgui/kxmlguibuilder.cpp 6773c31 kdeui/xmlgui/kxmlguifactory_p.cpp 2f81f18 Diff: http://git.reviewboard.kde.org/r/103812/diff/ Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
Re: Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/#review10773 --- Ship it! A bit too many spaces inside parenthesis in new code, but the overall code looks fine now. kdeui/widgets/ktoolbar.h http://git.reviewboard.kde.org/r/103812/#comment8748 Should say @deprecated use addXMLGUIClient. on its own line. - David Faure On Feb. 20, 2012, 10:33 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- (Updated Feb. 20, 2012, 10:33 p.m.) Review request for kdelibs and David Faure. Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs - kdeui/widgets/ktoolbar.h 69c482e kdeui/widgets/ktoolbar.cpp d17ff39 kdeui/xmlgui/kxmlguibuilder.cpp 6773c31 kdeui/xmlgui/kxmlguifactory_p.cpp 2f81f18 Diff: http://git.reviewboard.kde.org/r/103812/diff/ Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
Re: Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/#review10444 --- I was surprised that there was no way to go from the action to its collection, but indeed, it's not possible, because an action could be in multiple collections. In such a case, your patch might do the wrong thing, picking the wrong collection. But indeed the xmlguiclient member variable isn't much help either, it's the last xmlgui client that had a toolbar tag pointing to this toolbar (no misuse there btw). I think the cleanest solution would be to search for the action in the N xmlguiclients that added actions to this toolbar. Which means, deprecating KToolBar::setXMLGuiClient and adding a KToolBar::addXMLGuiClient, calling that from kxmlguibuilder instead, and keeping a list internally. Then you can iterate over that list to find the action, which will prevent finding it in completely unrelated action collections (e.g. the one used for a popup menu, like in konqueror). - David Faure On Jan. 28, 2012, 3:28 p.m., Albert Astals Cid wrote: --- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- (Updated Jan. 28, 2012, 3:28 p.m.) Review request for kdelibs and David Faure. Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs - kdeui/widgets/ktoolbar.cpp cce242b Diff: http://git.reviewboard.kde.org/r/103812/diff/diff Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
Review Request: Write to the correct xmlFile in KToolBar::Private::slotContextShowText()
--- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103812/ --- Review request for kdelibs and David Faure. Description --- KToolBar::Private::slotContextShowText() was assuming that the xmlgui file it had to write was KGlobal::mainComponent.componentName() + ui.rc; which is obviously wrong since we have a setXMLFile function for a reason. I tried using xmlguiClient-xmlFile() directly but in Okular we use the same the same toolbar name defined in two xml files, so that still did not work because this means we end up with just one KToolbar (yes i know that might be a misuse of the API). So i ended up going through the actioncollections to find the action and get the correct client from there. This addresses bug 292574. http://bugs.kde.org/show_bug.cgi?id=292574 Diffs - kdeui/widgets/ktoolbar.cpp cce242b Diff: http://git.reviewboard.kde.org/r/103812/diff/diff Testing --- Fixes the issue in Okular, i tested it does still work with Kate that is using the ui.rc scheme. Thanks, Albert Astals Cid
