Fix random order of plugin's config pages
ClosedPublic

Authored by arrowd on Jul 26 2018, 5:44 AM.

Details

Summary

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.

Test Plan

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 2068
Build 2086: arc lint + arc unit
antonanikin created this revision.Jul 26 2018, 5:44 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptJul 26 2018, 5:44 AM
antonanikin requested review of this revision.Jul 26 2018, 5:44 AM
antonanikin edited the summary of this revision. (Show Details)Jul 26 2018, 5:45 AM

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).

apol added a subscriber: apol.Jul 27 2018, 1:15 PM
apol added inline comments.
kdevplatform/shell/configdialog.cpp
162

Why do we ever want it not sorted?

Never noticed that randomness, but agree that is not perfect and should be fixed.

Test:

  1. Load both Cppcheck and Heaptrack plugin
  2. Unload Cppcheck plugin
  3. Load Cppcheck plugin back
  4. 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
162

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?

  • Enable sorting by default
  • Simplify code
arrowd accepted this revision as: arrowd.Jan 3 2019, 5:32 PM
arrowd added a subscriber: arrowd.

I have tried this out on master and it works for me. If no one objects in next few days, please push this.

This revision is now accepted and ready to land.Jan 3 2019, 5:32 PM
arrowd commandeered this revision.Jan 10 2019, 2:31 PM
arrowd edited reviewers, added: antonanikin; removed: arrowd.
This revision now requires review to proceed.Jan 10 2019, 2:31 PM
arrowd updated this revision to Diff 49157.Jan 10 2019, 2:35 PM

Remove insert API and use append everywhere.

mwolff added a subscriber: mwolff.Jan 10 2019, 2:45 PM
mwolff added inline comments.
kdevplatform/shell/configdialog.h
70

this should probably be sorted as well, no?

arrowd updated this revision to Diff 49159.Jan 10 2019, 3:02 PM

Fix docs, rename append* methods to add*.

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.

mwolff requested changes to this revision.Jan 24 2019, 4:25 PM

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 ↗(On Diff #49159)

the list above is "sorted" already based on the sort order we impose, so this is fine and non-random too

535 ↗(On Diff #49159)

these lines should all be moved into the array on top I believe

542 ↗(On Diff #49159)

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

This revision now requires changes to proceed.Jan 24 2019, 4:25 PM
arrowd added inline comments.Jan 27 2019, 5:55 PM
kdevplatform/shell/uicontroller.cpp
542 ↗(On Diff #49159)

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.

yes I would be fine with that personally!

arrowd updated this revision to Diff 55029.Mar 29 2019, 1:04 PM
arrowd marked 5 inline comments as done.

Remove API changes and just sort plugins list.

I don't remember details of this revision, but at least it seems to do like how we agreed.

mwolff accepted this revision.Apr 1 2019, 10:46 AM
This revision is now accepted and ready to land.Apr 1 2019, 10:46 AM

Onto master?

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)

This revision was automatically updated to reflect the committed changes.