On 04/11/2016 02:25 PM, Guillaume Munch wrote:
> Le 11/04/2016 19:02, Richard Heck a écrit :
>> On 04/11/2016 12:57 PM, Guillaume Munch wrote:
>>> Le 11/04/2016 17:28, Jean-Marc Lasgouttes a écrit :
>>>> Le 11/04/2016 17:53, Scott Kostyshak a écrit :
>>>>> I can confirm that I do not see the problem in 2.1.4 and I do see it
>>>>> with 2.2.0dev.
>>>>>
>>>>> I suppose this is a dataloss bug, and that there is a rule of not
>>>>> going
>>>>> to an RC with a known dataloss bug that is a regression. Although the
>>>>> bug only shows in a specific case and has gone unnoticed for more
>>>>> than a
>>>>> year in 2.2.0dev, it is pretty easy to trigger and it would not be
>>>>> surprising at all that a user comes across it. I would like to hear
>>>>> from
>>>>> Jürgen to see if a quick fix can be made before proceeding with rc1.
>>>>
>>>> Here is a very lightly tested patch. It looks OK to me, but this
>>>> kind of
>>>> changes has potential to do more harm than good. So if we do not
>>>> have 1/
>>>> good user testing and 2/ a +1 from someone who understands this
>>>> code, I
>>>> am not sure we want it in rc1. And rc1 really needs to happen soon
>>>> IMO.
>>>>
>>>> JMarc
>>>>
>>>>
>>>>
>>>>
>>>> 0001-Fix-saving-of-unknown-flex-insets.patch
>>>>
>>>>
>>>> From 86774cd7123cbb34c235bdcab9d11eb09c8fc6b8 Mon Sep 17 00:00:00
>>>> 2001
>>>> From: Jean-Marc Lasgouttes<lasgout...@lyx.org>
>>>> Date: Mon, 11 Apr 2016 18:16:05 +0200
>>>> Subject: [PATCH] Fix saving of unknown flex insets
>>>>
>>>> After commit cfeddb9293b, flex insets without a proper inset layout
>>>> (for example because a module has been removed) are saved with name
>>>> "undefined". This can result in a data loss.
>>>>
>>>> Introduce a new method InsetFlex::hasLayout that uses the same logic
>>>> as getLayout, but only returns a bool. Use this to decide when
>>>> outputting the raw inset name is a good idea.
>>>> ---
>>>> src/insets/InsetFlex.cpp | 18 ++++++++++++++----
>>>> src/insets/InsetFlex.h | 2 ++
>>>> 2 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/insets/InsetFlex.cpp b/src/insets/InsetFlex.cpp
>>>> index 44b9b03..edff6cb 100644
>>>> --- a/src/insets/InsetFlex.cpp
>>>> +++ b/src/insets/InsetFlex.cpp
>>>> @@ -45,6 +45,18 @@ InsetFlex::InsetFlex(InsetFlex const & in)
>>>>
>>>>
>>>> // special code for InsetFlex when there is not the explicit Flex::
>>>> prefix
>>>> +bool InsetFlex::hasLayout() const
>>>> +{
>>>> + if (!buffer_)
>>>> + return false;
>>>> +
>>>> + DocumentClass const & dc = buffer().params().documentClass();
>>>> + return dc.hasInsetLayout(from_utf8(name_))
>>>> + || dc.hasInsetLayout(from_utf8("Flex:" + name_));
>>>> +}
>>>> +
>>>> +
>>>> +// special code for InsetFlex when there is not the explicit Flex::
>>>> prefix
>>>> InsetLayout const & InsetFlex::getLayout() const
>>>> {
>>>> if (!buffer_)
>>>> @@ -68,13 +80,11 @@ InsetLayout::InsetDecoration
>>>> InsetFlex::decoration() const
>>>> void InsetFlex::write(ostream & os) const
>>>> {
>>>> os << "Flex ";
>>>> - InsetLayout const & il = getLayout();
>>>> if (name_.empty())
>>>> os << "undefined";
>>>> else {
>>>> - // use il.name(), since this resolves obsoleted
>>>> - // InsetLayout names
>>>> - string name = to_utf8(il.name());
>>>> + // Using InsetLayout::name() resolves obsoleted InsetLayout
>>>> names
>>>> + string name = hasLayout() ? to_utf8(getLayout().name()) :
>>>> name_;
>>>> // Remove the "Flex:" prefix, if it is present
>>>> if (support::prefixIs(name, "Flex:"))
>>>> name = support::token(name, ':', 1);
>>>
>>>
>>> Again, why stripping "Flex:" off the beginning of name_? This is not
>>> how it is done before the regression. Or did I miss something?
>>
>> name_ wouldn't have contained "Flex:". It's there because
>> InsetLayout::name() includes it.
>
>
> I am not sure that you understood my question. Are you making the
> assumption that nobody ever wrote Flex:Flex: just to spare an "else"
> branch?
This would be a bad idea, but it saves a few cycles to do "else if"
anyway, so it's fine.
>
>>
>>> Also I think it is safer to replace support::token with support::split.
>>
>> Actually, it would be easier just to use substr(5). We know what we're
>> removing.
>>
>
> Sure, or whatever equivalent function. I can confirm that this gives
> another regression, whereby
>
> \begin_inset Flex x:y
>
> from 2.1.4 will get replaced by
>
> \begin_inset Flex x
>
> in 2.2 upon saving. In particular when the layout Flex:x:y is defined
> and Flex:x is not (or to something entirely different). This should be
> patched as well IMO.
I'm not sure I see why this is---or which version of which patch is causing
it---but I'm prepared to believe it.
> To summarise, I think the best solution in the long run is Jean-Marc's
> but with these two issues above being corrected. In the short term it is
> safer to go with my patch, which is essentially Richard's "safe" patch
> plus these two issues corrected. Let me know what you think.
That's fine with me.
JMarc, can you plan to commit your version soon in the 2.2.1 cycle?
Speaking of which, I'm inclined to open a 2.3.staging branch for such
things.
Richard
