Fix random order in "Analyze Current File/Project With" menus
Needs ReviewPublic

Authored by antonanikin on Aug 29 2018, 9:57 AM.

Details

Reviewers
None
Group Reviewers
KDevelop
Summary

The patch fixes current random order "Code" -> "Analyze Current File/Project With" menus. Since KXmlGui framework doesn't support menu auto-sorting we have random order of menu items, which depends on plugin loading order.

Steps to reproduce:

  1. Load both Clazy and Cppcheck plugins
  2. Reload Clazy plugin
  3. Clazy's menu items will be placed after the Cppcheck one.
Test Plan

Tested on master branch, works as expected

Diff Detail

Repository
R32 KDevelop
Branch
arcpatch-D15140
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2747
Build 2765: arc lint + arc unit
antonanikin created this revision.Aug 29 2018, 9:57 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptAug 29 2018, 9:57 AM
antonanikin requested review of this revision.Aug 29 2018, 9:57 AM

Given we might have quite some places where items are pulled from the plugins and thus the order of plugins predefines the order, could we perhaps ensure a normalized order of the plugins in the plugin controller?

Though otherwise, having the menu sorted alphabetically might be nice to have in general, so this patch still seems useful. Not yet looked at implementation, will see to do later this week.

could we perhaps ensure a normalized order of the plugins in the plugin controller?

I think it's impossible in general. We can try to force plugin order during KDevelop startup, but user can also load/unload plugins at any time.

First round of code feedback. Still need to check whether kxmlgui is fine with us twisting the generated QMenu behind its back, or if that will confuse things.

kdevplatform/shell/mainwindow_p.cpp
88

-> sortMenuAlphabetically

125

Please add a comment in the code why this is done, like:

// Ensure alphabetical order in "Analzye Current * With" submenus
127

why static? that might be fragile, there is no promise in the kxmlgui API that submenu instances and their actions persist all the time, or?

128

Analyzer plugins are not required to add an action to the main menu, so if there is no plugin which added an action to the submenu, this assert will wrongly fail, no?
So for what I understand no menu/action should be an expected condition and normally handled by skipping.

131

static?

132

See above, no action could be expected.

kossebau added inline comments.Sep 10 2018, 11:04 AM
kdevplatform/shell/mainwindow_p.cpp
128

From my testing it seems that the kxmlgui item for the submenu is always created, even when there is no content added. Had not found this documented though, but perhaps we can run with the QASSERT for now.
In any case though let's use official KXmlGui API to get the submenu :)

auto fileMenu = static_cast<QMenu*>(m_mainWindow->guiFactory()->container(QStringLiteral("analyze_file"), m_mainWindow));
        auto projectMenu = static_cast<QMenu*>(m_mainWindow->guiFactory()->container(QStringLiteral("analyze_project"), m_mainWindow));

Hi, Friedrich.

Thanks for your review. I think about ordering problem and found another interesting solution. We can use KXMLGUIClient::plugActionList() method to solve our case. We can implement common base class, for example ICodeAnalyzerPlugin, with appropriate methods which return file/project analyze actions and use them to (re)build sorted menus and plug them into our main menubar. Such approach can be also used for another plugins, like Heaptrack/Valgrind (run-time checkers/analyzers).

What are you think about such way?

Hi, Friedrich.

Thanks for your review. I think about ordering problem and found another interesting solution. We can use KXMLGUIClient::plugActionList() method to solve our case. We can implement common base class, for example ICodeAnalyzerPlugin, with appropriate methods which return file/project analyze actions and use them to (re)build sorted menus and plug them into our main menubar. Such approach can be also used for another plugins, like Heaptrack/Valgrind (run-time checkers/analyzers).

What are you think about such way?

Actions added with KXMLGUIClient::plugActionList() are not properly known to KXMLGUI, which e.g. means they cannot be added to the toolbar persistently or (not sure) be edited in the key shortcut dialog. Not sure if those loses are worth the gain.
Right now I would favour your initial proposal.

And for the future I would see us extending KXMLGUI to support some flag for action groups, which define some order (or perhaps even a callback id, so we can inject custom sorting algorithms).
Having a random/undefined order in the menu on adding other XmlGuiClients is a generic challenge, so other users of KXmlGui might find the option to define orders good to have as well.

antonanikin marked 7 inline comments as done.
  • Address comments

Right now I would favour your initial proposal.

Ok.

Having a random/undefined order in the menu on adding other XmlGuiClients is a generic challenge, so other users of KXmlGui might find the option to define orders good to have as well.

Yes, this will be best solution for all.

I was about to say Ship-It, but first went to ask for adding a note to the code comment in MainWindowPrivate::addPlugin() that this is a hack behind KXmlGui's back, but should be fine, and in the future this should be a functionality of KXmlGui itself.
Typing those words though, I found myself in conflict with my personal principle of "if-not-a-data-loss-bug-fix-upstream-instead-of-fragile-workaround".

@antonanikin Given this is not a grave bug, but just some slightly annoying inconsistency. would you agree we should rather live with the current situation instead of adding potentially fragile code (which needs maintenance by future people), but instead spend efforts on adding support to KXmlGui to allow menu sorting mechanisms?
When it comes to the Analyzers menus, I have a personal TODO item for a KXmlGui feature to solve in any case, as I would like to get rid of the repeated "Analyze * With" in the actual analyze action menu entries, if listed in that submenu.

kdevplatform/shell/mainwindow_p.cpp
88

Add also static, to keep the funtion symbol private to this compilation unit, which would be better behaviour.
IIUC, as we are in the KDevelop namespace here, not the anonymous one, so the symbol is exported from the compilation unit, and might result in clashes with similar util methods from other compilation units on creating the linkage unit (or whatever the term is).

Hi, Friedrich. Ok, we can skip the patch and wait for upstream KXmlGui fix. I should then abandon the revison?

pino added a subscriber: pino.Sep 17 2018, 6:01 AM
pino added inline comments.
kdevplatform/shell/mainwindow_p.cpp
96–97

please use KLocalizedString::removeAcceleratorMarker() instead of removing & (see its API doc to understand why it is important)

127

this sounds better as QLatin1String, since a real QString is not needed

antonanikin added inline comments.Sep 29 2018, 10:05 AM
kdevplatform/shell/mainwindow_p.cpp
96–97

Don't know about this method earlier, thanks!