Skip to content
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

[UWP] Split the UWP object model from the renderer #6067

Merged
merged 10 commits into from
Jul 31, 2021

Conversation

RebeccaAnne
Copy link
Contributor

@RebeccaAnne RebeccaAnne commented Jul 7, 2021

Related Issue

Fixes: #5885

Description

Separated the Object Model portion of the UWP renderer into a separate library that can be consumed independently of the main renderer component. The object model has been moved from the AdaptiveCards.Rendering.Uwp namespace to the AdaptiveCards.ObjectModel.Uwp namespace. This involved pulling all object model interface definitions/code/utilities into a new library. Downstream changes to make sure everything still builds/works include

  • Updating namepaces,
  • Updating the Renderer component to operate on the API surface rather than accessing the object model implementation classes directly.
  • Exposing InternalId via that SDK to allow access by the Renderer.
  • Update FeatureRegistration handling to operate on the API surface rather than peaking into and using the shared model implementation

How Verified

  • Ran all existing UWP unit tests. Split the object model tests into a separate library to confirm that they run and pass against the new object model library.
  • Opened ALL the files in \samples to ensure that they rendered successfully and sanity check rendering results.
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the no-recent-activity label Jul 13, 2021
@ghost
Copy link

ghost commented Jul 13, 2021

Hi @RebeccaAnne. This non-spec pull request has had no recent activity for the past 5 days . Please take the necessary actions (review, address feedback or commit if reviewed already) to move this along.


In reply to: 879362616

@ghost ghost removed the no-recent-activity label Jul 19, 2021
@ghost
Copy link

ghost commented Jul 19, 2021

Hi @RebeccaAnne; Thanks for taking action on your previously stale pull request. Resetting staleness.


In reply to: 882698973

<ClCompile Include="lib\AdaptiveTableRow.cpp" />
</ItemGroup>
<ItemGroup>
<ClInclude Include="lib\AdaptiveCard.h" />
Copy link
Contributor

Choose a reason for hiding this comment

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

why are some of these files duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which ones are dups? I'm not noticing them.

@@ -17,7 +17,7 @@ namespace AdaptiveCards
static InternalId Current();
static constexpr unsigned int Invalid = 0;

std::size_t Hash() const { return std::hash<unsigned int>()(m_internalId); }
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this change being done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The instigating change here was that i needed to expose internal id out of the public API surface and i declared it as UINT32. When you build this for x64 you get errors assigning tying to assign a (64 bit) size_t to a UINT32, whereas a (32bit) unsigned int (which is what we're storing it in anyway) works fine.

I removed the std::hash call because (a) it returned size_t and (b) the hash of the integer is itself. That said, I admit there might be a good reason to do this that i'm not aware of. @paulcam206 is there a reason this is a Bad Idea?

Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

should this file be included?

source/uwp/AdaptiveCardsObjectModel/dll/RCa09988

Copy link
Member

@paulcam206 paulcam206 left a comment

Choose a reason for hiding this comment

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

just a few minor nits

@RebeccaAnne
Copy link
Contributor Author

Removed


In reply to: 715106367

@RebeccaAnne RebeccaAnne merged commit 0a8cf2a into main Jul 31, 2021
@RebeccaAnne RebeccaAnne deleted the rebecch/uwprefactor branch July 31, 2021 05:36
@andrewleader
Copy link
Contributor

I saw this on my main GitHub home activity feed, awesome to see this happen! Always wished the UWP object model and renderer was split like this! ❤️

michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
* [UWP] Split the UWP object model from the renderer

* build break fix

* PR feedback

* Removed unneeded file and update .gitignore to ignore generated header

* delete generated header

* More cleanup

* Add InternalId.h to the project
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
* [UWP] Split the UWP object model from the renderer

* build break fix

* PR feedback

* Removed unneeded file and update .gitignore to ignore generated header

* delete generated header

* More cleanup

* Add InternalId.h to the project
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate UWP Object Model from UWP Renderer
4 participants