Re: Regression in saving of flex insets in beta2?

[Guillaume Munch]

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?

Also I think it is safer to replace support::token with support::split.

I like your patch which is more logical, and I also agree that we might 
want something safer for 2.2.


Guillaume