Differential D5123

Try to fix call to KXMLGuiWindow::setupGUI
AbandonedPublic
Actions

Authored by kfunk on Mar 21 2017, 10:14 PM.

Details

Reviewers
mlaurent
dfaure
Group Reviewers
KDevelop
Summary

KXmlGuiWindow documentation tells us to not pass the 'Create' flag:

* NOTE: when using KParts::MainWindow, remove this flag from
* the
* setupGUI call, since you'll be using createGUI(part) instead.
* @code
*     setupGUI(ToolBar | Keys | StatusBar | Save);
* @endcode

This in fact fixes a real problem. Before this patch duplicated standard
actions (e.g. the 'help' action is created twice internally).

There's a problem, though, this changes messes up KDevelop's menu bar:

This is how it looks like normally:

I need this patch in order to fix another issue (I'd like unbind the default shortcut for the F1 key).

Diff Detail

Repository
R33 KDevPlatform
Branch
5.1
Lint
No Linters Available
Unit
No Unit Test Coverage
kfunk created this revision.Mar 21 2017, 10:14 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMar 21 2017, 10:14 PM
kfunk planned changes to this revision.Mar 21 2017, 10:15 PM
kfunk edited the summary of this revision. (Show Details)
kfunk added a reviewer: KDevelop.

Does anyone have an idea why this is happening? I just debugged this for a few hours but couldn't figure it out. KXMLGui is black magic to me...

apol added a subscriber: apol.Mar 21 2017, 10:32 PM

Have you tried removing the cached rc files you might have in .local? It's working properly here, I doubt it's a problem in the code...

kfunk edited the summary of this revision. (Show Details)Mar 21 2017, 10:39 PM
kfunk added a comment.Mar 21 2017, 10:44 PM

CC'd David & Laurent: Do you guys have an idea what could be wrong here? Looks like kdevelopui.rc from kdevelop.git/app is not obeyed anymore after this change. Why?

In D5123#96579, @apol wrote:

Have you tried removing the cached rc files you might have in .local? It's working properly here, I doubt it's a problem in the code...

Yep, this is not the problem.

kfunk edited the summary of this revision. (Show Details)Mar 21 2017, 11:29 PM
apol added a comment.Mar 21 2017, 11:42 PM

I get this with your patch:

dfaure edited edge metadata.Mar 22 2017, 7:54 AM

This looks like ui_standards.rc isn't loaded.
Does this code call createGUI(part) anywhere? Do you get into KParts::MainWindow::createShellGUI(), and does it end up calling setXMLFile(KXMLGUIClient::standardsXmlFileLocation()); in kparts/src/mainwindow.cpp:168 ?

kfunk added a comment.Mar 22 2017, 8:17 AM
In D5123#96600, @dfaure wrote:

This looks like ui_standards.rc isn't loaded.

Yes, later the day I figured this could be the reason.

Does this code call createGUI(part) anywhere? Do you get into KParts::MainWindow::createShellGUI(), and does it end up calling setXMLFile(KXMLGUIClient::standardsXmlFileLocation()); in kparts/src/mainwindow.cpp:168 ?

I'll debug. Am I correct in the assumption that ui_standards.rc is the first file which needs to be loaded in kxmlgui? If not this is probably our issue -- we do call createGUI(part) at some point, but it's probably too late.

dfaure added a comment.Mar 22 2017, 8:20 AM

createGUI(part) takes care of loading ui_standards.rc, see the bit of code I pointed to in my last comment.

kfunk added a comment.Mar 22 2017, 8:26 AM
In D5123#96602, @dfaure wrote:

createGUI(part) takes care of loading ui_standards.rc, see the bit of code I pointed to in my last comment.

Sorry, you misunderstood me. Yes, I know createGUI(part) eventually loads ui_standards.rc. Debugged through half of kxmlgui already ;)

I think our issue is that we call createGUI(part) to late during startup, while other XML files (e.g. those from plugins) have already been loaded.

dfaure added a comment.Mar 22 2017, 8:28 AM

Oh, yeah, that sounds backwards.

Those are like layers: mainwindow, active part, plugins, on top of each other in that order.

You want the mainwindow layer and its active part first (createGUI(part) does both) and only then you want to load plugins on top of that.

kfunk abandoned this revision.Oct 28 2018, 5:56 PM

No time to work on that right now.

kossebau added a subscriber: kossebau.Dec 4 2019, 12:35 PM

Ran into this again this week, wondering why F1 shortcut was still working after assigning another shortcut (and then found there are mutiple instances).
Once I had a patch, it triggered memories, and indeed there is this one. 9c4b8e752937dd4523b04b57f237ac20c3bc3e45 should be the correct fix, by simply doing a createGUI(nullptr); call right after setupGUI(), so with a nullptr, an approach taking also in other KParts::MainWindow usages during startup, before going to actually load any document, e.g. on user request, and thus creating a kpart.

dfaure added a comment.Dec 22 2019, 9:50 PM

Looks fine to me.

Revision Contents
Changeset List

PathPackages
Mshell/mainwindow.cpp (2 lines)

Diff 12670

View Options

shell/mainwindow.cpp

Loading...