Add support for restoring non-terminal panes, and opening them with splitPane, newTab
#16914
Conversation
| Boolean Equals(INewContentArgs other); | ||
| UInt64 Hash(); | ||
| INewContentArgs Copy(); | ||
| String GenerateName(); |
There was a problem hiding this comment.
FWIW, I believe property-like functions are usually prefixed with Get.... Type should probably not be settable either.
There was a problem hiding this comment.
Equals, Hash, Copy, and GenerateName were all just straight lifted out of NewTerminalArgs. I can rename them if you think it's important, but also, 🤷
| [default_interface] runtimeclass GenericContentArgs : INewContentArgs{ | ||
| GenericContentArgs(); | ||
| GenericContentArgs(String type); | ||
| }; |
There was a problem hiding this comment.
I don't believe this would work well for an extension model as plugins may want arbitrary settings. It may be better to not have a string Type and instead have concrete runtimeclasses for each specific builtin Pane type. Extensions may define their own concrete types which we'll then somehow pass through to the plugin to get our IPaneContent or whatever. We can also skip that part of course and let such users create and pass IPaneContents directly to the term control constructor(s). In both cases however I would prefer concrete runtimeclasses.
There was a problem hiding this comment.
Absolutely. As of this commit, the only real INewContentArgs is the NewTerminalArgs. The settings pane and scratch pane don't really have settings, so in the name of... laziness I guess... I used the smallest possible placeholder for their internal representation. GenericContentArgs was never supposed to be anything more than an internal helper.
In the "near" future where I plan to ship that markdown pane, that'll actually have it's own MarkdownPaneArgs with like, a filePath property or something.
There was a problem hiding this comment.
If we don't want to have a INewContentArgs implementation for each content type, would it make sense to at least move the Type getter down into GenericContentArgs?
There was a problem hiding this comment.
Oh I mean, we absolutely could have SettingsContentArgs, ScratchContentArgs. They'd just basically each be exactly GenericContentArgs, just without the ctor that accepts the type.
There was a problem hiding this comment.
FWIW...
never .. more than an internal helper
Internal helpers can use winrt::implements<> so they don't go in idl files/on the public API! If you need to try_as it, that means our IWhatever is an insufficient abstraction.
There was a problem hiding this comment.
"internal" in the sense of "we can use this but I'm not really expecting others to use it"
Right now it's also used by TerminalApp to also return the type of content for a ScratchpadPane.
I could theoretically just rename it like, BaseContentArgs. Trying to make this entirely internal to TSM results in me basically just creating the same exact internal (non-projected) class up in TerminalApp
This reverts commit 893b25e.
There was a problem hiding this comment.
LGTM, apart from a few nits.
Unrelated:
I continue to feel like the Type field in the INewContentArgs interface should be removed. While I think that giving contents descriptive names is a fantastic idea (makes understanding the content simpler), I feel like the mapping between names and types is an unrelated concern to the types themselves having names. Basically I can see why we want Name -> Type, but this adds Type -> Name, and... Do we really want that? 🤔 Additionally, checking for IIDs is well established for WinRT and COM which is IMO another +.
BTW this is also similar to how the interface has a Hash / Equals method. While we need those for our architecture of actions, there's no inherent reason IMO why an action needs an ID or must be comparable in the first place (a deduplication could hypothetically be done before we ever convert JSON to types, or could be done at some other point).
I'm not arguing that we should remove Hash / Equals, but I do think that the Type property may lead us down the wrong path for identifying contents and plugins.
There was a problem hiding this comment.
I've done everything except the implementation in ActionArgs; I have some minor concerns so I'm blocking to drive them down - I am also bothered by it being stringly-typed but that's not the reason I'm blocking. I wouldn't mind that being improved (using IIDs/QI/whatever), but it's secondary to my other questions!
microsoft-github-policy-service
bot
added
Needs-Author-Feedback
There was a problem hiding this comment.
I'm okay with this for now, as it fills a need and doesn't get us too far away from the mark. As we grow I think we'll need to reconsider, but right now we don't have a plan and this is making session restore be busted.
Let's disagree and commit, then fix it later.
This changes
NewTabArgs,SplitPaneArgs, andNewWindowArgsto accept aINewContentArgs, rather than just aNewTerminalArgs. This allows a couple things:splitPane,newWindowactions, just by passing"type": "scartchpad"(for example). This is a lot more flexible than re-defining different"openScratchpad","openTasksPane", etc, etc actions for every kind of pane.The
typeproperty was added tonewTab,splitPane,newWindow. When omitted, we still just treat the json as a blob of NewTerminalArgs.There's not actually any other kinds of
INewContentArgsin this PR (other than the placeholderGenericContentArgs). Indev/migrie/fhl/md-pane, I have a type of pane that would LOVE to add some args here. So that's forward-thinking.There's really just two stealth types of pane for now:
settings, andscratchpad. Those I DON'T have as constants or anything in this PR. They probably should be? Though, I suspect around the time of the tasks & MD panes, I'll come up with whatever structure I actually want them to take.future considerations here
foocontent", for 3p content.wtCLI args were not yet updated to also accept--typeyet. There's no reason we couldn't easily do that.ICanHasCommandlineto allow arbitrary content to generate awtcommandline-serializable string. Punted on that for now.other PRs
splitPane,newTab#16914 <-- you are hereCloses #17014