-
Notifications
You must be signed in to change notification settings - Fork 321
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
MRTCore will also search for [modulename].pri for unpackaged app #814
Conversation
It would be helpful for the reader if you sprinkled comments in the body. For example, a comment for this while block will help. In reply to: 837708365 In reply to: 837708365 Refers to: dev/MRTCore/mrt/Core/src/MRM.cpp:878 in 18d0d84. [](commit_id = 18d0d84, deletion_comment = False) |
MrmFreeResource(path); | ||
|
||
Assert::AreEqual(MrmGetFilePathFromName(L"something.pri", &path), S_OK); | ||
// Even if the file doesn't exist, we will still return a path. This is for those don't use PRI for resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an existing app may want to migrate to switch to WinUI3, but not ready to switch the resource management yet. They can use ResourceNotFound event to hook up their existing resource management.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding that or part of that as a comment would be good, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The comment on 963-964 of MRM.cpp was not enlightening because it didn't explain why that was the desired behavior.
dev/MRTCore/mrt/Microsoft.ApplicationModel.Resources/UnpackagedTests/TestClass.cs
Show resolved
Hide resolved
One question: in the wapproj case, will resources.pri exist alongside <csproj/vcxproj name>.pri? (resources.pri is a merge of <csproj/vcxproj name>.pri and other stuff) Assuming it does, if there is an issue generating resources.pri, our code will now succeed (it'll fall back to <csproj/vcxproj name>.pri). Don't we want it to fail instead? Otherwise the change looks good to me. |
In reply to: 840038734 |
Regarding #2 above, with wapproj-less packaged apps, we do want to use .pri, don't we? There won't be a resources.pri in that case, no? EDIT: looking at the code, it seems 2 is not true. GetDefaultPriFile() is only ever called with an empty path. So, if we fall back to the second method (in the impl of GetDefaultPriFile()), we will look for .pri (if we can't find resources.pri). I agree that 1 is true. This one edge case comes to mind: is it possible to do a clean of the wapproj only where resources.pri is removed but .pri remains? In reply to: 840038734 |
@@ -145,6 +145,7 @@ steps: | |||
!**\*TestAdapter.dll | |||
!**\TE.*.dll | |||
!**\obj\** | |||
!**\MrtCoreUnpackagedTests.dll |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
{ | ||
*filePath = nullptr; | ||
|
||
RETURN_HR_IF(E_INVALIDARG, filename == nullptr || *filename == L'\0'); | ||
|
||
wchar_t path[MAX_PATH]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not as part of this PR, but we should remove usages of MAX_PATH.
for any packaged app (wap or not), the resource file needs to be resources.pri. If it's not, we need to make that happen, or the app may have issue with system MRT. In reply to: 840116910 |
MrmFreeResource(path); | ||
|
||
Assert::AreEqual(MrmGetFilePathFromName(L"something.pri", &path), S_OK); | ||
// Even if the file doesn't exist, we will still return a path. This is for those don't use PRI for resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. The comment on 963-964 of MRM.cpp was not enlightening because it didn't explain why that was the desired behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For unpackaged app, the tool will build the resource file as [modulename].pri. It's not straightforward to update build tool. Change the runtime behavior to also look up [modulename].pri for unpackaged app.
MrtCoreUnpackagedTest only runs in command line, as the test adapter launches te.processhost.exe from nuget cache folder, thus we cannot copy manifest and pri file there.