Sort plugin manifests in alphabetical order for registration #7266
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7266 +/- ##
==========================================
- Coverage 92.83% 92.75% -0.09%
==========================================
Files 623 623
Lines 57072 57159 +87
==========================================
+ Hits 52980 53015 +35
- Misses 4092 4144 +52 ☔ View full report in Codecov by Sentry. |
| for mf in manifests: | ||
| sorted_manifests = sorted( | ||
| manifests, | ||
| key=lambda mf: mf.display_name if mf.display_name else mf.name, |
There was a problem hiding this comment.
We may provide here better key function.
def _get_plugin_sort_key(mf: PluginManifest) -> str:
if len(mf.contributions.widgets) == 1:
return mf.contributions.widgets[0].display_name
if mf.display_name:
return mf.display_name
return mf.nameThis will solve the problem of ordering single widget contribution.
There was a problem hiding this comment.
Omg duh 😂 thanks @Czaki will fix up
There was a problem hiding this comment.
Hmm actually this will solve the Plugins menu case, but then Samples menu may be incorrectly ordered, right? I don't think there's a way we can get both correct at this stage in the code. We'd have to collect all contributions and sort before registering the actions with app-model.
There was a problem hiding this comment.
I think I know how to do it with minimal intervention actually. I'll follow up. If this PR gets merged beforehand that's fine
There was a problem hiding this comment.
I think that, the best solution, will be to improve build_qmodel_menu and QModelMenu For get dict of id -> sorting function
There was a problem hiding this comment.
Ok I'd say let's merge this PR as is, and we'll follow up for the single-contribution case
|
Do we want to add a test..? (Can be done in the follow up I guess, it would be good ensure we are doing the right thing for samples and plugins menu) |
|
Since I just added tests now I'll re-start the 24hr clock and merge tomorrow if there are no objections! |
Co-authored-by: Grzegorz Bokota <bokota+github@gmail.com>
for more information, see https://pre-commit.ci
…d of `mock_app_model` and `get_app_model` (#7283) # References and relevant issues CI over main is failing due to the issue this PR fixes # Description Since PR #7269 and PR #7266 where being worked on in parallel merging both caused some tests to fail (due to their usage of `mock_app` and `get_app`)
References and relevant issues
Closes #5500
Description
This PR ensures that when the viewer is launched, plugin manifests (and therefore their actions) are registered in alphabetical order of manifest's display name (if available) or plugin name.
Note:
I think just merging this PR as-is is still a great improvement, and I can follow up with the re-sorting in a separate PR. Otherwise if folks think this is not acceptable, we can leave this open while I figure out the re-ordering on plugin install/enable, and the single action sorting.