Break everything out of App except Xaml platform init #3465
Conversation
…AppLogic is the new home.
miniksa
requested review from
DHowett-MSFT and
zadjii-msft
and removed request for
DHowett-MSFT
…ere to support universal launch).
… because winrt objects are magic.
| dialog.CloseButtonText(buttonText); | ||
|
|
||
| _ShowDialog(nullptr, dialog); | ||
| static AppLogic logic; |
There was a problem hiding this comment.
Why is this not a member of App? You could wait to instantiate it until this is first called.
There was a problem hiding this comment.
@DHowett-MSFT, as far as I can see, the magic static is safer than the alternative as it cannot accidentally be referenced from elsewhere in the class without being allocated and will only be allocated deferred to the first call.
Doing what Mike says opens us up to a null check on the instance variable, theoretically. But this is indeed magic and maybe we shouldn't magic?
| const auto& warnings = _settings->GetWarnings(); | ||
| for (const auto& warning : warnings) | ||
| // if this is a UWP... it means its our problem to hook up the content to the window here. | ||
| if (_isUwp) |
There was a problem hiding this comment.
I'm assuming this path isn't supposed to work right now, correct? It looks to me like at the end of this block, the logic is going to have it's refcount decremented and it'll get cleaned up. Seems like the App would need to hang onto it as a member... but I could be wrong.
There was a problem hiding this comment.
It doesn't work right now in that there's no Universal head showing in the project. But this code does totally work correctly if you invoke from a Universal context.
It's not going to be cleaned up. It has an outstanding reference in the static right now due to the "magic static" as far as I know after the first call and won't be cleaned, just wait for future use.
|
So @zadjii-msft, @DHowett-MSFT, I think none of us care strongly enough about this to change it. Can I just leave it then as is so I can get onto the good bit of making the UWP work and not worrying about App? |
DHowett-MSFT
changed the title
|
🎉 Handy links: |
Summary of the Pull Request
This removes anything in App that isn't the base stuff to set up XAML. This will be necessary for dual-heading it with Centennial & UWP hosts
References
This is part of #3236 but I had to re-do it anyway because of merges. I want to get it submitted so people stop adding stuff to App.
PR Checklist
Detailed Description of the Pull Request / Additional comments
@ocalvo tells me that app shouldn't have anything in it but just the init and onload. So I moved everything else to
AppLogicand glued it back together.Validation Steps Performed
Ran the project and launched it locally.