Read the PropertySet Locale property into the PropertyContext #259
Conversation
OpenMcdf.Ole/PropertySet.cs
Outdated
| int codePageOffset = (int)(propertySetOffset + PropertyIdentifierAndOffsets.First(pio => pio.PropertyIdentifier == 1).Offset); | ||
| br.BaseStream.Seek(codePageOffset, SeekOrigin.Begin); | ||
|
|
||
| var vType = (VTPropertyType)br.ReadUInt16(); | ||
| br.ReadUInt16(); // Ushort Padding | ||
| PropertyContext.CodePage = (ushort)br.ReadInt16(); | ||
|
|
||
| // Read the Locale, if present | ||
| PropertyIdentifierAndOffset? localeProperty = PropertyIdentifierAndOffsets.FirstOrDefault(pio => pio.PropertyIdentifier == 0x80000000); |
There was a problem hiding this comment.
It's not necessarily related to this commit, since it happens elsewhere, but it would be nice to have a constant for LOCALE_PROPERTY_IDENTIFIER (renamed to follow dotnet conventions of course) rather than using a magic number...
There was a problem hiding this comment.
There's a similar situation with the Behavior proprerty as well, though nothing actually checks that as it stands
jeremy-visionaid
changed the title
|
I could go either way on making it nullable. I don't really like using a reference type to store something trivial. On the otherhand, the locale might be present but invalid, and it would probably be better to handle that as well as possible rather than nit-picking performance/memory. |
…property is present in the file
Numpsy
force-pushed
the
populate_locale
branch
from
13a8a5a to
9463e0a
Compare
| @@ -65,7 +65,8 @@ public OlePropertiesContainer(CfbStream cfStream) | |||
|
|
|||
| Context = new PropertyContext() | |||
| { | |||
| CodePage = pStream.PropertySet0.PropertyContext.CodePage | |||
| CodePage = pStream.PropertySet0.PropertyContext.CodePage, | |||
There was a problem hiding this comment.
Oh wait. Should Context even be a member of OlePropertiesContainer? Can the one from PropertySet be used instead? I'm not sure of the reason for interaction, but its seems just like duplication at a glance.
There was a problem hiding this comment.
Hmm, don't know why it was cloning the context originally, and I didn't think to just use the existing context instance directly on read :-(
The write side at
is already just using the same context instead of cloning it, so it'd make sense to make them consistent
…property is present in the file
As long as PropertyContext has a Locale property, I think it should be populated where possible, and as it happens the existing 'Issue134' test file contains a Locale property that can be used for testing.
A thought whilst looking though - the Microsoft docs at https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-oleps/bb452c07-79f7-46e0-bfb7-122c347cc5c8 state that Locale is an optional property, so I wonder if the PropertyContext.Locale property should be nullable rather than being 0 if there is no locale specified?