Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Get image layer working with cppwinrt. #354

Merged
merged 3 commits into from
Sep 23, 2020
Merged

Conversation

simeoncran
Copy link
Contributor

Has some issues if the images are not found, but it works otherwise.
Includes some untested cleaning up of the CX code.

Has some issues if the images are not found, but it works otherwise.
Includes some untested cleaning up of the CX code.
@simeoncran simeoncran requested a review from a team as a code owner September 22, 2020 23:55
In some non-release cases, cppwinrt does not allow classes to be final because it subclasses them..
@scott-moore-ms
Copy link

scott-moore-ms commented Sep 23, 2020

Would it be useful/helpful to have a checked-in reference file? Then you can look at the actual code being generated and have context to reason about what the codegen tool is doing #Resolved

priv.WriteLine("bool m_isAnimatedVisualSourceDynamic{};");
priv.WriteLine("bool m_isImageLoadingCompleted{};");
priv.WriteLine("bool m_isTryCreateAnimatedVisualCalled{};");
priv.WriteLine("bool m_isImageLoadingStarted{};");
Copy link

@scott-moore-ms scott-moore-ms Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an amount of redundant state stored on this object which is simply derived from the completion count relative to the total count. Was it an explicit choice to have progress/started/completed be state that needs to be updated and kept in sync, vs simply computed when requested? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the isImageLoadingCompleted being the same as loadCompleteEventCount == number_of_images? Or are there others?
I don't remember all the decisions that went into the design - it was the result of people asking for features for handling various loading states, but I think they changed their minds and never used them. And I think we changed the policy a bit at one point and the code probably could have been simplified then.


In reply to: 493105017 [](ancestors = 493105017)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I was originally talking also about m_imageSuccessfulLoadingProgress, but looking at the generated code for that, it actually appears on the surface to be buggy?

    m_loadCompleteEventCount++;
    if (e.Status() == LoadedImageSourceLoadStatus::Success)
    {
        m_imageSuccessfulLoadingProgress = (double)m_loadCompleteEventCount / c_loadedImageSurfaceCount;
        m_PropertyChanged(*this, Windows::UI::Xaml::Data::PropertyChangedEventArgs(L"ImageSuccessfulLoadingProgress"));
    }

If I have 5 images, and 4 of them fail to load, but the 5th one loads successfully, that would result in a progress of 1.0 rather than the (I think?) intended 0.2?


In reply to: 493109988 [](ancestors = 493109988,493105017)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that looks wrong to me. I'll remove the image loading progress. #355
Not doing it with this change because it affects all of the code generators rather than just C++.


In reply to: 493804067 [](ancestors = 493804067,493109988,493105017)

@simeoncran
Copy link
Contributor Author

Yes, but I don't have a file that I can share publicly. I've sent you an example output in mail.


In reply to: 697048111 [](ancestors = 697048111)

Copy link

@scott-moore-ms scott-moore-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@simeoncran simeoncran merged commit 0b9501b into master Sep 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the IDynamicCppwinrt_1 branch September 23, 2020 19:55
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants