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

Add more properties to the 'DocumentSummaryInfo' PropertyIdentifiers set #266

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

Numpsy
Copy link
Contributor

@Numpsy Numpsy commented Dec 3, 2024

Adds more properties listed at https://learn.microsoft.com/en-us/openspecs/office_file_formats/ms-oshared/3ef02e83-afef-4b6c-9585-c109edd24e07

As an example, structured storage explorer currently shows the properties in this file with two numerical identifiers as property names

image

but afterwards they all have a name:

image

Copy link
Collaborator

@jeremy-visionaid jeremy-visionaid left a comment

Choose a reason for hiding this comment

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

Generally as a wider point when looking at this code, it seems to me like all the things for a property set should be encapsulated together rather than some bits as a ContainerType and some in PropertyIdentifiers and perhaps expose some kind of registry for additional sets. Since other property sets are possible, it doesn't seem particularly extensible as is... But that's probably something for another day and another pull request.

OpenMcdf.Ole/PropertyIdentifiers.cs Outdated Show resolved Hide resolved
@@ -43,7 +43,18 @@ public static class PropertyIdentifiers
{0x0000000D, "PIDDSI_DOCPARTS" },
{0x0000000E, "PIDDSI_MANAGER" },
{0x0000000F, "PIDDSI_COMPANY" },
{0x00000010, "PIDDSI_LINKSDIRTY" }
{0x00000010, "PIDDSI_LINKSDIRTY" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should CodePageString also be updated to PIDDSI_CODEPAGE? And similarly PIDSI_CODEPAGE for SummaryInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what the historical reason was for it being different, but the new major version seems like the right time to change it if wanted

@Numpsy
Copy link
Contributor Author

Numpsy commented Dec 4, 2024

it doesn't seem particularly extensible as is

It's a bit painful because there are some special cases where you really need specific knowledge of a property set to process everything correctly - e.g. the situation with aligned/unaligned strings which both have the same property type.

Allowing callers to provide their own property factory might be one way of handling that, but the API would need some thought.

Really I don't know if OlePropertiesContainer should try to do full processing of arbitrary streams ('AppSpecific;') or if there should be one or more focussed types at the top level, with a more generic layer underneath as building blocks for users to build their own containers on.

@jeremy-visionaid jeremy-visionaid merged commit 3753eaa into ironfede:master Dec 4, 2024
4 checks passed
@Numpsy Numpsy deleted the more_props branch December 8, 2024 10:56
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.

2 participants