ironfede / openmcdf

Microsoft Compound File .net component - pure C# - netstandard 2.0
Mozilla Public License 2.0
308 stars 76 forks source link

Null padding of strings is an implementation detail, should not be exposed to the consumer #121

Closed rmsimpson closed 5 months ago

rmsimpson commented 7 months ago

User-defined property names and any other ole property string values that require null termination should be automatically wrapped and unwrapped by the implementation and not returned back to the user this way. If I as a consumer write a user-defined property as "ABC" I expect to be able to find "ABC" when I read the properties back from the file, and not "ABC\0".

Tests like this one, for example: https://github.com/ironfede/openmcdf/blob/c58985f1a7d85f8e128b7f5bd6be371c6b3e5c6d/sources/Test/OpenMcdf.Extensions.Test/OLEPropertiesExtensionsTest.cs#L150

Consumers shouldn't have to deal with trailing zero's due to the implementation-specific detail of them being stored that way. The underlying implementation should be trimming trailing zero's during a Read operation, and adding (as needed) trailing zero's during a Write operation automatically.

Numpsy commented 7 months ago

On write, the null terminators should be getting added internally in the latest release if they aren't already present (there was a bug in the previous version where they sometimes weren't)

Numpsy commented 7 months ago

Also, as far as the tests go, some of the writing tests are effectively using the null terminators being returned from the read to check that they were actually written by the write, and there's probably scope to test that more directly.

ironfede commented 7 months ago

Please @rmsimpson, let me know if PR could be considered a fix for this issue. Many thanks, Federico

rmsimpson commented 6 months ago

I had some comments in the PR that were addressed, and don't have any other outstanding issues with it. Thanks!