Re: Revert "tdf#106283: Registry settings are not read properly on Windows"

On 03/06/2017 09:43 PM, Tamas Zolnai wrote:
> Anyway, you must be right with that it's not a good idea to change the staticType_, so I added an 
> other fix which does not affect staticType_, but uses a typeHint only for parsing a specific value 
> from XCU file.
> Can you review this change please? I hope you're happy with that:
> https://gerrit.libreoffice.org/#/c/34933/

see below

> On Monday, March 06, 2017 18:40 GMT, "Tamas Zolnai" <tamas.zolnai@collabora.co.uk> wrote:
> > So it's clear that the generated xcu file from Windows registry is incomplete\invalid, so it can be 
> > a good idea to fix it, but this would take an amount of time since registry keys used for LO are 
> > always strings as I see (see comments in configmgr/source/winreg.cxx) and I guess it will work on 
> > the same way for the end of the time for compatibility reasons. So it is impossible to find out the 
> > type from the registry key. To find out  that we need to parse the corresponding xcd file here too 
> > (as configmgr code does).

The existing mapping from the Windows registry to the configmgr data 
model is naive, which inevitably gives rise to issues like tdf#106283.

If you/anybody wants to promote that Windows registry feature, it would 
make sense to invest time into a proper mapping between the two data 
models.  (I can help with that and/or you can look at the comment at the 
head of configmgr/source/dconf.cxx for an idea of what to look out 
for---though that mapping is even more complicated as it is 
bidirectional.)  Sure, that may take time (what's the issue with that), 
and may unfortunately be complicated by backwards-compatibility 
concerns.  (An alternative would have been to not allow the existing 
Windows registry feature in in its current naive form, but then again 
the restricted amount of things that can reasonably be done with it in 
its current form may have been sufficient for its initially envisioned 
users.)

(I have only superficial knowledge of the Windows registry, but from 
looking at 
<https://en.wikipedia.org/wiki/Windows_Registry#Keys_and_values> one 
backwards-compatible way to encode configmgr value+type might be with a 
two-element REG_MULTI_SZ list whose first element encodes the type of 
value in the second element.  Or, even simpler, from looking at the 
comment at the head of configmgr/source/winreg.cxx, in addition to 
"Value" and "Final" keys introduce a "Type" key.  Wouldn't that be 
rather trivial to implement?)

> > It can be a solution, but as I see you created a bug report (tdf#92755) about avoiding using these temporary 
> > xcu files for Windows registry, so I'm not sure it worth to try to make these xcu files valid (having type 
> > information), if they will be removed later, right? So I'm not sure why you suggest to fix the code which 
> > generates these xcu files. (your comment on bugzilla: "[...] For such extension props it must generate 
> > an oor:type attribute.[...]") Also fixing this bug by replacing xcu generation with a better 
> > implementation which using configmgr's internal data would also take me very far from fixing the bug I 
> > intended to (tdf#106283).

Whether or not we go via intermediary temporary xcu files, the mapping 
from the Windows registry to the configmgr data model must be fixed. 
(That is, for such an extension prop, the mapping code needs to provide 
a type that it somehow extracts from the Windows registry data model, 
whether that type is then stored in an intermediary xcu file or directly 
used when instantiation a configmgr::PropertyNode.  That's what I meant 
with "For such extension props it must generate an oor:type attribute.")

Cleaning up those intermediary xcu files is of course orthogonal to 
fixing tdf#106283.

> > "[...]On the other hand, it is important that the PropertyNode representing
> > such an extension prop has a staticType_ of TYPE_ANY, so that later layers or
> > programmatic calls can store values of different type."
> >
> > So this part is also not clear to me. What do you mean later? When can it happen that the same 
> > property get a different type, which is defined in the specific xcu/xcd file?

<http://web.archive.org/web/20101103025920/http://util.openoffice.org/common/configuration/oor-document-format.html>: 
"[...] dynamically added properties behave as if defined in the schema 
as nillable, non-localized property of type any."  (And properties of 
type any can take on values of different types over time.)

> > What do you mean programmatic calls can store values of different type? When a programmatic call 
> > would store a different typed value for a typed property? One specific property always defined with 
> > a type even if this property is part of an extensible group, right? So I can't see why this type 
> > would be overriden by a programmatic call? Or if this is a use case (using properties as something 
> > in which you can store anything) then I guess this also must be true for all properties, not only a 
> > member of an extensible group. What's the difference?
> >
> > I also tested that case when an extensible group has properties with different types 
> > (xs:boolean-xs:long) and it also works. I thought you might thinking of that case and later means 
> > later when the specific extensible group is extended with a new property with a different type. In 
> > this case my change works. So I would appreciate if you can point out a use case when this code 
> > change is problematic.

So, for such an extension prop (a "dynamically added property", as the 
cited document calls them) having e.g.

  <prop oor:name="foo" oor:type="xs:boolean"><value>false</value></prop>

in one layer and

  <prop oor:name="foo" oor:type="xs:int"><value>0</value></prop>

in a higher layer would no longer work with your change.

So coming back to <https://gerrit.libreoffice.org/#/c/34933/>, I'm not 
too excited about that hack.  It's a quick-fix to work around one 
specific shortcoming of the naive Windows registry mapping, but in a way 
that would require that hack to stay for backwards-compatibility reasons 
even when/if a more principled fix for that mapping is done.