The patch fixes current random order of plugin's config pages. New version extends API to keep config pages list in sorted state not depending on the plugins loading/unloading order.
Details
- Reviewers
antonanikin mwolff - Group Reviewers
KDevelop - Commits
- R32:e8b3ee4a2541: Output config subpages alphabetically, instead of order in which corresponding…
Works as expected on my neon-useredition system (Ubuntu Xenial) with Qt 5.11.1 and kdevelop (git/master).
Diff Detail
- Repository
- R32 KDevelop
- Branch
- config_pages_sorted
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 6910 Build 6928: arc lint + arc unit
Never noticed that randomness, but agree that is not perfect and should be fixed.
Not perfect though yet would be also the proposed sorting by localized name IMHO:
- it might separate and reorder child pages belonging to one plugin and being delivered in an order done by design
- it results in different orders in different languages, making support slightly more difficult between people using different UI locales
I have not yet looked at the actual logic how the pages are updated on enabling/disabling plugins, so no idea yet where the "randomness" is inserted. But I would hope we could have one place which creates a generic normalized order of plugins which then is reflected also in the settings page order (besides the main category pages which already have a hardcoded order IIRC).
kdevplatform/shell/configdialog.cpp | ||
---|---|---|
150 | Why do we ever want it not sorted? |
Test:
- Load both Cppcheck and Heaptrack plugin
- Unload Cppcheck plugin
- Load Cppcheck plugin back
- Cppcheck's config page will be placed after the Heaptrack one.
So the current config pages order depends on plugins loading order :(
kdevplatform/shell/configdialog.cpp | ||
---|---|---|
150 | Hmm... I check KDevelop's code and it seems you are right - this method now is used only for pages which should be sorted. So we can set true as default value for sortedAppend parameter and simplify the patch code. What are you think about this? |
I have tried this out on master and it works for me. If no one objects in next few days, please push this.
kdevplatform/shell/configdialog.h | ||
---|---|---|
62 | this should probably be sorted as well, no? |
The problem with current revision is that pages added in uicontroller.cpp:530 aren't sorted.
There is a proposition to add another top-level page to group "other" config pages, but I'm unsure how to call it, since there is already "Plugin" page.
On the other hand, only CMake plugin reaches that else branch, and moreover the page doesn't get deleted when CMake plugin is unloaded, so I'd just leave it as is.
so, I believe we need to change some things here...
kdevplatform/shell/projectcontroller.cpp | ||
---|---|---|
157 ↗ | (On Diff #49159) | hmm looking at this again. this old code here sorts already, so in principle it should be fine already? |
kdevplatform/shell/uicontroller.cpp | ||
511 | the list above is "sorted" already based on the sort order we impose, so this is fine and non-random too | |
535–537 | these lines should all be moved into the array on top I believe | |
542 | so the source of randomness can only come from here, where we iterate over the loaded plugins. fixing that would be simple, too: just collect the pages here, then sort them, then add them. that should fix the randomness without changing the whole API and changing the order of the basic config pages |
kdevplatform/shell/uicontroller.cpp | ||
---|---|---|
542 | This approach would sort pages when the configuration window is opened. It doesn't sort pages when you load/unload plugins from Plugins page. When I uncheck and then check Clazy plugin there, it gets appended to the end of Analyzers pages list. But if I then close and open the configuration window again, it gets back on its place. If you are OK with this, so do I. |
I don't remember details of this revision, but at least it seems to do like how we agreed.
if it applies cleanly, you can also push to 5.3, otherwise master is fine - it's not a really urgent bug fix after all (imo)