Fix tools loading at start, preventing performance issue on commands update

[ReMinoer]

I encountered huge performance issue (if not a performance "blocker") at application start when I had at least one hidden tool in the restored layout. I was unable to do anything because of how busy the application was.

After some profiling, I found it was coming from the call to ViewLocator.LocateForModel in CommandRouter.GetCommandHandlerForLayoutItem. The problem is that when hidden tools are restored at loading, they are set as ActiveLayoutItem despite not being shown and having no view attached to the view model. So when the ViewLocator.LocateForModel is called, it doesn't find a view from the view model provided by ActiveLayoutItem and instantiate a new view instance (without attaching it). And it does it every time commands are updated (on every click) and for every existing command, explaining the performance issue.

The more that view is costly to instantiate and the more commands you have, the more your application will be busy. The fact that Gemini.Demo has few commands and, more importantly, open some documents by default also explain why it was not encountered before (since ActiveLayoutItem will be set after layout loading with opened documents).

I suggest to fix it by replacing the callback to IShell.ShowTool by simply IShell.Tools.Add when loading layout. Calling ShellViewModel.ShowTool is setting ActiveLayoutItem and that's why we are encountering the issue in the end.

Here is my thinking :

1. We don't have to set ActiveLayoutItem at layout loading because it has no useful behavior or visual effects on application starting state. (ILayoutItem.IsSelected property is the one restoring the top tool for each docking space.)
2. A null ActiveLayoutItem should not be an issue since CommandRouter is the only class outside of the shell itself to use ActiveLayoutItem and already handle a null reference.
3. LayoutUtility.LoadLayout was already doing works similar to IShell.ShowTool : it sets IsVisible & IsSelected with restored layout data and call ActivateAsync if necessary.

Considering all those points, the provided addToolCallback only have to be IShell.Tools.Add. I also fixed the condition of call to ActivateAsyncat layout loading : LayoutContent.IsActive is actually not restored (it has a XmlIgnore attribute) and is always false. To keep it coherent with IShell.ShowTool behavior, I suggest to replace it with a check on LayoutAnchorable.IsVisible.

EDIT: I added a AddTool method to LayoutItemStatePersister to also check if tool already exist, as ShowTool was doing. Without that, loading a layout at runtime was not working (anymore ?). I didn't add it to the IShell interface because it's a loading-only behaviour and it would be a bit ambiguous with ShowTool.

[tgjones]

Thanks for the fix, and the clear explanation 👍

[ReMinoer]

You are welcome. Thank you for the really cool framework. ❤️

[kornman00]

By not calling ShowTool, this broke functionality I added in PR #318 where the tool's ActivateAsync is called on first showing. E..g this is where I have tools subscribe (and unsubscribe) to the event aggregator only while active. ActivateAsync comes from Tool inherting from CM's Screen class, which implements IActivate. If the ShellViewModel does not call this, Tools never have their ActivateAsync invoked.

This change then leads to mismatched behavior when there's no ApplicationState.bin. On the application's first load, it won't find one so it uses DefaultTools which will be called with ShowTool which will have their ActivateAsync called.

After ApplicationState.bin is used, this calls a method that circumvents ShowTool to 'Add' the tool.

This fix thus introduced a bug.

[ReMinoer]

Hi @kornman00 :)

One of the point of my change was that ActivateAsync should not be called for every tool restored from ApplicationState.bin. The stored layout can contains hidden tools (tools opened but then closed at some point) and those should not be activated at loading because they are not shown yet or will never be. That's why I just "add" them at that point.

Also, LayoutUtility.LoadLayout is already doing the job to activate only the visible tools at layout loading. We don't need to use ShowTool to call ActivateAsync in that case.

But there is a case where hidden tools are indeed never activated with my change and I suspect that you encountered it. When a hidden tool was simply "added", the current ShowTool code indeed prevent it to activate when shown later on.

I have an already opened pull request #342 to address it. With this change, a tool can now still be activated even if it was already register at loading (what I called previously "add").

Can you two can take a look ? @tgjones @kornman00 Thanks.

[kornman00]

Thanks @ReMinoer ! With the problem I was hitting, the tool I was encountering the lack of ActivateAsync behavior with was one of the tools that opened at startup. Possibly due to ShouldReopenOnStart being true by default: https://github.com/tgjones/gemini/blob/master/src/Gemini/Framework/Tool.cs#L64 ? It would show up but the ActivateAsync wouldn't be called and then it wouldn't subscribe itself with the EventAggregator. So it wasn't that it was a hidden tool.

For further context, it's basically a module that was inspired by the Errors module, but setup to hook .NET tracing (which includes verbose/information/warning/error/critical levels). I originally prototyped something in this program: https://github.com/KornnerStudios/PhxStudio/blob/master/PhxStudio/Modules/TraceList/TraceListViewModel.cs

Although I'm using a updated/fixed version in a closed source application https://gist.github.com/kornman00/629c47dcfe10d6d9aedcfcf505c3a830

[ReMinoer]

I tried to reproduce your issue but everything is looking good with latest Gemini source + my pull request #342 (also reverting your pull request #346).

ShouldReopenOnStart property only decide if the layout item should be saved in ApplicationState.bin to be restored next time. As you saw, document items are not saved and tool are saved by default. But it will not force a tool to open at launch if it wasn't open last time. A tool can have ShouldReopenOnStart set to true and still be saved and restored as hidden (so not opened at loading).

https://github.com/tgjones/gemini/blob/e69ebe2ed622f5e47ecac2ea34051a4db12bc6f7/src/Gemini/Modules/Shell/Services/LayoutItemStatePersister.cs#L31-L34

Make sure you didn't encounter the hidden tools case fixed by #342 because it's quite common to have an hidden tool at loading. Tools tends to be kept as hidden in ApplicationState.bin because they are still save/restored at each session even if you don't open them. In other words: once a tool has been opened, it will always be stored in ApplicationState.bin between sessions, hidden or not.

I would be interested into seeing the up-to-date source of your PhxStudio repo, in another branch for example, because your source code on GitHub is a bit old and you have reimplemented some Gemini layout-related code in it. It's hard to debug the issue without your current code. ^^"

It's important that we find another way to fix your issue. We shouldn't activate not opened tools.

[ReMinoer]

@kornman00 Any news about it ? Do you still have issues with #342 ? And if that's the case, can you share more up-to-date code ?

Just-in-time tool activation is important for performance and I would like to restore it with #342.

[kornman00]

I think #342 should be merged and I can evaluate what to do on my end, assuming anything does need to be done, and report back :).

Right now, nuget isn't being updated (I commented here https://github.com/tgjones/gemini/commit/e69ebe2ed622f5e47ecac2ea34051a4db12bc6f7#commitcomment-82452580, maybe @tgjones 's key expired?) so I'm not actively trying to test Gemini upgrades on the closed source application (it's different from PhxStudio)