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

Rework the property factory to allow stream specific factories, and tweak the logic for deciding whether a property value should be padded. #108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Feb 12, 2024

A few experiments based on #99 and the comments in #98 -

Instead of having a single, singleton instance of a property factory, which is accessed in several places, instead pass an instance of the factory around, and decide which factory to use at a higher level.

When creating a property via the factory, pass the identifier of the property into the factory, so that it can make property specific choices in which property type to use when needed (mainly to use VtUnalignedString for a couple of string properties, rather than VtString)

Add an additional property factory for the DocumentSummaryInformation stream which has specific knowledge of a couple of unaligned string properties.

Also added the unit test and extra test file from #99 / #98 as a test case for a file that isn't currently read correctly.

@ironfede
Copy link
Owner

ironfede commented Feb 19, 2024

Many thanks @Numpsy :I don't think that a solution for the underlying issue can be "super-clean" from a code perspective since the issue derives from a tangled specifc in itself. So I think you should go on on with your experiments towards a fully working solution and only after that starting to think about a clean-up refactoring. I'm sorry for being not-active on this thread but I'm currently out of time to better follow all discussions and evolution of code. Thank you anyway for your developments.

{
private static ThreadLocal<PropertyFactory> instance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands, the factory class is stateless, so I don't know why it needs to be ThreadLocal? (if not, that change could be broken out into a separate PR to make things simpler here)

@Numpsy
Copy link
Contributor Author

Numpsy commented Feb 25, 2024

I'll have another look now the other changes are merged (I realised whilst testing those that the current code here is wrong, as it tries to use DocumentSummaryInfoPropertyFactory for user defined properties, which doesn't work because you can't determine the behavior based on property identifier for that)

@Numpsy Numpsy force-pushed the users/rw/factory_rework branch 2 times, most recently from 198be02 to 98c03ee Compare February 27, 2024 23:55
…weak the logic for deciding whether a property value should be padded.
@Numpsy Numpsy changed the title [WIP] Experiments with the property factory, and the logic to decide … Rework the property factory to allow stream specific factories, and tweak the logic for deciding whether a property value should be padded. Mar 7, 2024
@Numpsy
Copy link
Contributor Author

Numpsy commented Mar 7, 2024

Rebased onto the other recent changes, and tweaked a little to minimze the changes (the changes in TypedPropertyValue.cs look bigger than they really are due to bracing / indentation changes)

@Numpsy Numpsy marked this pull request as ready for review March 7, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants