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<[email protected]>
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?
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.
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.
Guillaume
