Read the PropertySet Locale property into the PropertyContext - #259
jeremy-visionaid merged 1 commit into
Conversation
Sorry, something went wrong.
| 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...
Sorry, something went wrong.
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
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
…property is present in the file
| 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.
Sorry, something went wrong.
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 https://github.com/ironfede/openmcdf/blob/319c003b7b71cc232683b3048c4622ef528fa11e/OpenMcdf.Ole/OlePropertiesContainer.cs#L243 is already just using the same context instead of cloning it, so it'd make sense to make them consistent
Sorry, something went wrong.
…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?