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

Investigate IInvokableCommand.Invoke(object sender) #307

Closed
zadjii-msft opened this issue Jan 12, 2025 · 4 comments · Fixed by #395
Closed

Investigate IInvokableCommand.Invoke(object sender) #307

zadjii-msft opened this issue Jan 12, 2025 · 4 comments · Fixed by #395
Assignees
Labels
Area-API Anything having to do with the actual interface the extensions use to communicate with the host

Comments

@zadjii-msft
Copy link
Owner

Look at this:
https://github.com/zadjii/CmdPalExtensions/blob/f946ff79ddad4777dccd2860e60ef652ccf66837/src/extensions/SegoeIconsExtension/Pages/SegoeIconsExtensionPage.cs#L68-L91

Each of those list items all creates 4 nested commands within it (the default action, and three more).

That kinda seems like a lot of memory?

What if instead, we could create just four commands:

  • Copy code \u...
  • Copy character
  • Copy for XAML
  • Copy Name

and each of those commands implemented Invoke with a sender parameter. That's only four commands for all however many thousand icons there are.

Then in the host, we'd pass in the IListItem1 to Invoke.

Back in the extension, they could cast the sender back to the IconListItem, get the IconData back out of it, then copy the thing that's relevant to that particular sender.

For a bunch of lists of things that can all do the same thing, this seems like it would save a bunch of effort.

The limitation is that the verbs for each of these items would necessarily be limited to the same Title/Subtitle/Icon for every single item, but that seems very acceptable as a tradeoff.

Footnotes

  1. Not sure what object to pass in quite yet. That's the open question here.

@zadjii-msft zadjii-msft added the Area-API Anything having to do with the actual interface the extensions use to communicate with the host label Jan 12, 2025
@zadjii-msft
Copy link
Owner Author

Open question: What is the sender?

Where do ICommands appear?

  • TopLevelCommands (and fallbacks)
    • Sender is the ICommandItem for the top-level command that was invoked
  • IListPage.GetItems
    • Sender is the IListItem for the list item selected for that command
  • ICommandItem.MoreCommands
    • Sender is the IListItem which the command was attached to for a list page, or the ICommandItem if this is the top level.
  • IMarkdownPage.Commands
    • Sender is the IMarkdownPage itself
  • ITag.Command
    • (this property I'm still considering removing)
    • It could be the IListItem the tag appears on
    • Or for a IDetailsTags in an IDetailsElement in an IDetails, it's whoever owns the Details - either the MarkdownPage or the IListItem

@zadjii-msft
Copy link
Owner Author

Open question: Does this make sense for a Page somehow?

i just dont know

@zadjii-msft
Copy link
Owner Author

zadjii-msft commented Jan 13, 2025

Open Question: What if we took it a step even further

Is there a higher-level abstraction like, rather than the item having all of its commands in, the provider returning which commands are applicable to which items

Or, the commands using some sort of ID system like the shell does - we learned this in ZipFldr work. You don’t give 10000 COM objects back when browsing a ZIP file. You give one, plus a bucket of IDs. Shell asks, “what is the display name of this ID?” “What is the file size of this ID?” “Invoke this ID”
the One is your ShellFolder Implementation

It was annoying to think about but it made sense once we realized. A folder could have 24,000 files in it. Creating that many MAYBE REMOTE com objects is highly expensive and wasteful especially if the user is not gonna scroll to see them all

Should commands be like this instead? “What is the display name of hackernews.opencomments?”

okay this might sound stupid - but the COM object's address is kinda just the ID, right? Like, if I set up a command cache on the host side, then a listItem hands back a pair of objects which live at [0x0001, 0x0002], then I check the cache to see if we already got 0x001, 0x002

yes and no. You don’t need to AddRef an ID, or track its lifetime
There is also the minor risk that the same object will have different RPC facades

(I.e. the pointer will differ. I don’t know if this is a real problem in practice but BriAl will!)

(So this still requires one AddRef per object during RPC marshaling etc)

May just do like a, GetItemIds() -> string[], and then GetItem(string id) -> IListItem


I bet we could even wrap things up so that we had like

interface IBigListPage requires IPage, INotifyItemsChanged
{
    string[] ItemIds { get; };
    IListItem GetItem(string id);
}

and then, I bet we could have Helpers.ListPage implement IBigListPage, so that the ItemIds getter would call the existing Helpers.ListPage.GetItems(), cache the results, then return back the IDs of those items (or generate a hash of the item). Then when the host calls IBigListPage.GetItem, the Helpers.ListPage.GetItem() implementation is hidden from the implementers, so it just fetches it from the cache.

I bet we could do that to silently make the x-proc part less chatty, though it would still instantiate all those objects in the extension. A more clever extension could wait to instantiate the item until when GetItem is actually called

@zadjii-msft
Copy link
Owner Author

I think I'm going to keep this change, but I also don't think this is the load-bearing hot path here.

My notes in #388 seem to suggest that it's just the raw number of x-proc calls needed to build the context menu that lead to the icons page taking so long to load.

This change could help in the sense that we could still build a command item cache, and try to look up the properties from that cache first for a given command. With this change, the context menu of each item would already be in the cache.

But more importantly is the "don't load the context items till we need them" bit.

@zadjii-msft zadjii-msft self-assigned this Feb 3, 2025
zadjii-msft added a commit that referenced this issue Feb 3, 2025
## BREAKING API CHANGES APPROACHING

Closes #307 
Closes #238 
Removes `Command` from `ITag`
Adds `IDetailsCommand` (to achieve the same goal as the above originally had)
Adds `ITreeContent`, based on a hunch
Adds `ShowToast` and `Confirm` to `CommandResult` too, but without UX yet

Extensions from before this change will need to be updated. 
* The `InvokableCommand` change should be trivial - the helpers should abstract that delta away for you.
* The `ContentPage` change should be pretty easy to make. 
  * Both `MarkdownPage` and `FormPage` are now just `ContentPage`
  * `FormPage.Forms()` -> `ContentPage.GetContent()`
  * `MarkdownPage.Bodies()` -> `ContentPage.GetContent()`
  * `IForm`s become `IFormContent`. Methods become properties (not that bad)
* I'm only deprecating the old Markdown and Form pages - I'll fully remove them before we OSS, but I'll give everyone a couple weeks to port them.
* No one was using `Tag.Command` and it always seemed iffy at best - better not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-API Anything having to do with the actual interface the extensions use to communicate with the host
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant