msohn

There's an excellent blog post by Vincent Parret: https://www.finalbuilder.com/resources/blogs/code-signing-with-usb-tokens

You only need to make sure you compile a universal binary. Packing and deployment is unaffected. Ensure the ARM64 platform is enabled and the compiler option set as described here: https://docwiki.embarcadero.com/RADStudio/Athens/en/Delphi_Considerations_for_Multi-Device_Applications#Universal_Binaries

Yes there is. Have a look at TSelectionEditor and its RequiresUnits method: https://docwiki.embarcadero.com/Libraries/Alexandria/en/DesignEditors.TSelectionEditor Many 3rd party products do this (e.g. DevExpress) especially when components have event handlers containing types that are defined in another unit. Because adding such an event handler will otherwise break compiling and the user going on a hunt for the unit.

You're missing TObject.InitInstance which ensures all fields are initialised before the constructor is called. Edit: link for convenience https://docwiki.embarcadero.com/Libraries/Athens/en/System.TObject.InitInstance

I'm glad I took the time to explain how I read the VTune flame graph and that you tried and verified most of what I expected (well guessed tbh). And that you now saw first-hand why the original code was written the way it was. Welcome to the world of maintaining legacy projects😉 I'm sure you figured out how to get a copy of VTune, if not refer to the thread by Anders which I linked to in the flame graph post. It contained all I needed to get going. I read your post like you reverted your changes for now, but you might want to consider a few things which could improve the solution you tried: 1. Make sure all the loaded info is kept, i.e. when opening the dialog a second time, everything is already there. That way the user has to endure the delay only once. If that proves that memory usage is of no real concern (which I'd expect), you could go ahead and 2. Consider loading the info asynchronously, i.e. in a thread/threadpool as soon as the app starts - then, if the users opens the dialog before the loading has finished, show a progress and wait for it to finish. This should behave great when it works, but making that codebase thread safe is an enormous task, especially since there don't seem to be unit- and functional tests which help you make changes with confidence

Absolutely correct. To see where the time is spent, look at the VTune screenshot I posted here a few day ago: You have to read it from the bottom up. The x axis is time spent and it's only showing the time spent in ...BaseMenuScreen.DoLevelSelect, i.e. clicking the button to show the dialog. There's basically 3 main "pillars". The right-most shows all the way from TCustomForm.Show, via your TFLevelSelect.FormShow event handler calling SetInfo. The LoadNodeLabels part is what I was referring to in my last post where only visible, uninitialised nodes are updated. That is something that I would recommend joining with .InitializeTreeView - but do it for all nodes - and show a progress while you do it. The caching happens in LemLevelNeoPack, for example in TNeoLevelEntry.GetTitle. All these property getters call LoadLevelFileData, which checks LoadState telling it what was already loaded and then only load the missing parts or do nothing. Finally, to be clear: someone (you?) already put quite some effort into optimising the LevelSelect dialog, i.e. to only spend as little time as possible with filling the GUI initially (think InitializeTreeView) and then load all other parts upon user-interaction (SetInfo calls). It just turned out to be too slow (maybe too many levels/groups/packs now) and a mess (calling SetInfo at too many places). Now the task is to undo all of that, do the slower loading of everything while showing a progress and giving your applications window message pump more time to "breathe" and fix the freezing. The result hopefully is a better, more responsive compromise: opening the dialog the first time will take a lot longer than now - the progress will move slowly - but subsequently, a lot quicker.

It already does this. Again, I don't have the code at hand right now, but I remember something like a FLoadLevel which signalled what kind of data had already been loaded so you could call the method again, and if it already loaded the info, skipped the loading, i.e. a cache. So my idea would be to fully update the tree including all the loading in InitializeTreeView, i.e. you would merge SetInfo (edit: only the part that updates the tree) and InitializeTreeView and show a progress while you do that. That would also allow to to get rid of the OnExpanded node handler you added earlier in the thread here. Because of the above cache, it wouldn't actually reload it. Again, I might be wrong, if the user-data does not have the aforementioned cache. Also because e.g. the tree nodes are only updated once - for all future calls to SetInfo a node update is skipped if it's text is non-empty. That way all the initially blank nodes created in InitializeTreeView are updated only once, including image index etc. If any of the user data influences the node text or its image, then it wouldn't be properly updated. Again - might be wrong, can't check. Fair enough - this was merely directed at considering 3rd party tree views. Also because I'm still optimistic it isn't needed.

From my limited experience with the code, I would say this is definitely the approach to take. Bear in mind though that some information might actually get changed in the dialog (it does, right?) - I had the impression that there were already optimisations in place that would prevent this from working, but I did not get to the bottom of this, so I might be wrong. I would do this first - and only if the result is still not acceptable, would I consider other options like 3rd party trees. But I'm quite positive that won't be necessary with preloaded info. Also: since your end goal is a totally different UI without a tree, why bother? Actually - that UI works so different that maybe you wouldn't even need to cache info at all since there is a lot less info which is displayed once at a time.

I was looking for an excuse to use VTune again 😉 (thanks to Anders map2pdb!) and here's a flame graph of what's going on while the form is shown: (this is without any sounds being played in the background since I don't have BASS dll lying around - and I'm glad I don't) So the Sleep in ApplicationIdle wastes about one third of the total time. And while FormCreate does spend quite some time in InitializeTreeView, half of it is now because the OnExpanded handler is called during it (that's what @Lars Fosdal was hinting at). But in the end most of the time is spent reading and parsing the level info that is spread over hundreds of text files.

Not with the built-in tools, but here's an excellent summary by Remy of what you need to do: https://stackoverflow.com/a/1130506/386473 I just used exactly that approach and was able to find and fix the memory leaks in like half an hour. I'll try to make a pull request on GitHub as the patch is a bit larger due to spacing when I introduced new try/finally blocks.

Yes I see that now - didn't properly test my changes. Have a look at the attached patch - before exiting, it makes the talisman box now visible. Fixes the issue that you described. SuperLemmixPlayer-21-25-34.patch

Are you using a GUI git client? Mine offers to apply patches (I highly recommend https://fork.dev). But after all it's a text file, it's not that difficult to read and understand it, especially since the changes are small and I described what I changed. And then there's also the command line, see https://git-scm.com/docs/git-apply

@Willicious: have a look at the .patch I attached. That are 2 relatively small changes that improve the situation a lot from what I can see. It caches the group for which the PackTalismanInfo has been generated so it can skip that time-consuming process if the selected group is still the same. And it uses the .OnExpanded handler I mentioned above. SuperLemmixPlayer-18-27-08.patch

Yes, I see that now too. SetInfo really does too many things - it not only shows the details about the level/group, it also manages the tree (i.e. LoadNodeLabels) - but only the visible ones, and it does so every time, even if most of the nodes will end up unchanged because they were previously updated. Edit: I see now that it skips nodes with a non-empty text, but that renders it quite useless - I thought the whole point of doing it so often, i.e. in SetInfo, is to be able to react to changes? If there are no changes, when why not fill everything properly in InitializeTreeView? A better place for calling SetInfo might be in OnExpanded - as that is the only place where uninitialised tree nodes become visible (also AFAICS). I made that change (uncommenting the Click and DblClick handlers and adding a SetInfo call in a new Expanded handler) and it seemed to work OK! The whole fInfoForm seems very inefficient to me and it also doesn't really behave very user-friendly (no mouse-wheel support, only the buttons are clickable). The whole thing could be done as an owner drawn listbox with a bit of extra code to react to the "buttons". This will certainly take some effort, but it would improve things a lot since not all x00 entries need a button and two labels being created.

@Willicious: I managed to get all relevant code from github and have it compiling and running. How do I reproduce this? The only bit of a hang I've seen is when I click one of the larger level group nodes - then it takes quite a while because of hundreds of buttons and labels being created on the fly. And another thing which makes this worse, there are many updates, i.e. SetInfo calls, even though the selected tree node hasn't even changed - a simple cache of what was previously selected would improve that situation - or at least remove the Click and DblClick handlers which call SetInfo - any click will rebuild the GUI even though the selected node has not changed.