Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PTRun] Allow preventing usage based ordering results #37491

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CoreyHayward
Copy link
Contributor

Summary of the Pull Request

Implement an additional functionality on the Result model allowing the prevention of selected count retrieval.

PR Checklist

Detailed Description of the Pull Request / Additional comments

This PR adds an additional property to the Result.cs model which allows for disabling the functionality which fetches the SelectedCount data from PowerToys. This enables the ability for plugins to prevent the SelectedCount property from being populated for individual results meaning that the sort order can be manipulated as desired by the Plugin giving more control back to the plugin developers.

Validation Steps Performed

@CoreyHayward CoreyHayward marked this pull request as ready for review February 16, 2025 17:38
Copy link
Collaborator

@htcfreek htcfreek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote some feedback on the property naming.

But I am wondering about the use case and feature implementation.

  • If you like to enforce a fixed oder, wouldn't you need to disable all types of automatic sorting (weighting boost, selecten based sorting, scoring)?
  • Have you tested that your implementation doesn't breaks the history (plugin)?

@CoreyHayward
Copy link
Contributor Author

Wrote some feedback on the property naming.

But I am wondering about the use case and feature implementation.

  • If you like to enforce a fixed oder, wouldn't you need to disable all types of automatic sorting (weighting boost, selecten based sorting, scoring)?
  • Have you tested that your implementation doesn't breaks the history (plugin)?

@htcfreek Thanks for the feedback!
The aim really is to disable the dynamic ordering for a result based on the number of times it has been selected. I think the updated implementation makes this a bit clearer. By doing this we achieve the goal of having a custom/fixed order result list which can be determined by the plugin developers if desired (the default behaviour acts as if this change was not implemented).
Please correct me if I am wrong but the WeightBoost is scoped to the plugin as a whole and is applied to every single result returned by a single plugin so this does not affect ordering within the scope of a plugin and the Score is already determined by the Plugin creating the result so again this allows for control over the ordering whilst still allowing the plugin developers to control it. The only thing completely out of the plugin developers control is whether the selection based scoring + multiplier is applied to the result which is what this change aims to resolve.

I have tested this manually with one of my own plugins and it does have the desired result when enabled whilst also not causing any affect on results that do not make use of the feature.

@htcfreek
Copy link
Collaborator

@CoreyHayward
Thank you for the chages. Now things are much easier to understand.

I wrote a small suggestion for renaming the new method.

Co-authored-by: Heiko <61519853+htcfreek@users.noreply.github.com>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

@CoreyHayward CoreyHayward changed the title [PTRun] Allow preventing selected result data retrieval [PTRun] Allow preventing usage based ordering results Feb 20, 2025
@moooyo
Copy link
Contributor

moooyo commented Feb 21, 2025

LGTM. I'm not familiar with the sort part implementation. But at least in my view it will not break anything and just add a useful setting for plugin author.

Copy link
Collaborator

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code does not break anything indeed. However, this setting should be on a plugin level, not a result level. That change probably needs a bit more work, but overall it will make more sense

@CoreyHayward
Copy link
Contributor Author

The code does not break anything indeed. However, this setting should be on a plugin level, not a result level. That change probably needs a bit more work, but overall it will make more sense

I don't necessarily disagree with your point as it makes sense for my plugin (though I don't know if any other plugin dev's may have a weird use case for this where they want only certain results to be affected by selection weight), however I don't believe there is currently a mechanism for this in Plugin system to set dynamic configuration at a plugin level which can then be applied to the result. The only mechanism I am aware of for this currently is the PluginMetadata class but this is pulled from the plugin.json file which can't really be changed or updated.
My goal with this change is to allow users to configure my plugin via a custom PluginAdditionalOption which will allow them to turn this behaviour on or off depending on their preference.

Please do let me know if I am missing something that would help with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants