Le 06/01/2016 20:51, Georg Baum a écrit :
Guillaume Munch wrote:
Le 03/01/2016 11:04, Georg Baum a écrit :
Really? The amount of code changes is big, and I cannot see from the
patch that it has no influence on the .tex input/output or mathml/xhtml
output. If it can not be 100% guaranteed that only the display is changed
then it is too dangerous IMHO. If only the display is changed then it
would be OK (but only because the current display is quite broken,
otherwise it would be too dangerous as well IMHO).
When you say "the patch", which one of the three are you discussing?
All.
The first one is just refactoring and you should be able to check
function by function that it does not change the meaning (apart from the
addition of the isMutable() check). Of course, I would be happier if it
was not necessary to do this sort of clean-up before working on the code.
The first one is more than refactoring. The behaviour of
InsetMathHull::display(), InsetMathHull::numberedType() and
InsetMathHull::currentMode() changed. currentMode() is used for parsing, so
I would not change it between alpha and beta. Did you find a bug with the
old behavior, or did you change it for theoretical reasons?
Also, the embedded whitespace and return value changes (as in
InsetMathHull::standardFont()) make it more difficult to verify whether the
introduction of hullUnknown was really purely mechanical. BTW, the explicit
cases in InsetMathHull::ams() were done on purpose, so that the compiler
forces us to think whether any ney type that is introduced in the future is
an AMS type or not.
You are right, I forgot that these small changes were not
straightforward. I have now made a corrected version, but I decided to
put this first patch aside for now since it is long and not essential.
For the other two, I was wondering about mathml/xhtml until I realized
that alignment is not implemented at all (I checked by reading the code
too). But, to be extra safe, I can reintroduce the default align and
spacing values to be extra sure that nothing else changes apart from the
display.
The second patch looks incomplete to me: It removes two defaultColAlign()
methods, but not the virtual one in the base class. Since defaultColAlign()
is not only used for display I don't think such a change should go in right
now.
I think that these comments miss the point of the patches. The logic of
storing the values is that inserting or deleting columns is going to
shift the values appropriately by one column. This is not the logic of
the environments under discussion, whose alignment and spacing only
depend on the column number, not the history of how the columns have
been introduced.
I replace stored alignment and spacing values with computed values, only
in the case of specific grids under discussion (Eqnarray, AMS...). Thus,
defaultColAlign() can still make sense in the future for grids that rely
on stored alignment and spacing, and I do not see what makes it useful
to remove it entirely.
About "defaultColAlign() is not only used for display": you mean that it
is risky to change the behaviour of stored values. Let us admit that
there is still a risk despite all my checks. Then, I proposed to keep
the default values to be extra safe just above your remark which makes
me think that you may have missed it.
(But in any case these stored values are necessarily wrong since they
are not saved to the file for the environments under consideration. I am
highly sceptical that a bug could remain open for so long if it caused
more than a display bug to which users got accustomed.)
defaultColSpace() is used when creating new columns or resetting existing
ones to the default. If it does not work then it should be repaired and not
removed as in the third patch.
Again this misses the point. There is nothing to "repair" about
defaultColSpace(). If there is still any buggy behaviour regarding the
spacing or alignment then they should be changed to read the computed
values that I introduce. Same remark about keeping defaultColSpace in
order to be extra safe.
I apologize for being such a nitpicker and "Spaßbremse",
If anything, please continue! Also, this is a software that many people
(including me) depend on professionally, so I do not understand this
notion that developing LyX is supposed to be "Spaß".
but if you ever
fixed a minor bug that caused a major regression that was only noticed after
release, and felt the embarrassement when investigating a bug report and
discovering the reason of the regression, you hopefully understand that.
I am sorry about that.
Also, we should be aware that any change we are doing right now throws away
testing effort of the alpha testers. This is fine for obvious local bug
fixes, but critical for more fundamental changes.
I am aware of this! The reason for doing these quick patches
about a month ago was that they only change the display in a
straightforward manner (yet addressed an annoying and long-standing
bug). Therefore in principle it was very safe and appropriate for
between alpha and beta. In fact, we are now back essentially to the
original patch which did not change the behaviour of stored values.
Somehow I deviated from my original goal of having a risk-free patch
during the discussion with Jean-Marc (which caused me to investigate
further and conclude that defaultCol* could be removed).
See the attached for two "safe" (and easy to read) patches, if you agree
that a safe patch that we have been discussing for a month can still get
in for 2.2.
Guillaume
>From a0a7cfbfe695c65e20cf166c9863b2d9d9692487 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Sun, 13 Dec 2015 03:32:32 +0000
Subject: [PATCH 1/2] Display the correct horizontal alignment in AMS
environments
A longstanding problem... (related: #1861)
The columns in AMS math environments have a fixed alignment (colAlign() in
InsetMathGrid.cpp). We set this alignment for display (Georg's
displayColAlign()) in InsetMathHull and InsetMathSplit. This is done according
to tests and documentation for the various environments.
There is also some mechanical code factoring via colAlign().
Finally, I disable setting the horizontal alignment in InsetMathSplit, which has
no impact on the LaTeX output, and has no longer any impact on the screen. (As
for vertical alignment I discovered that it was in fact customisable for
\aligned & friends! I hope that the more faithful interface will let other
users discover that too.)
---
src/mathed/InsetMathGrid.cpp | 25 +++++++++++++++++++++++
src/mathed/InsetMathGrid.h | 5 +++--
src/mathed/InsetMathHull.cpp | 47 ++++++++++++++++++++++++++++++++++---------
src/mathed/InsetMathSplit.cpp | 37 ++++++++++++++++++++++++++++------
src/mathed/InsetMathSplit.h | 2 ++
5 files changed, 98 insertions(+), 18 deletions(-)
diff --git a/src/mathed/InsetMathGrid.cpp b/src/mathed/InsetMathGrid.cpp
index 536f4bd..fca5722 100644
--- a/src/mathed/InsetMathGrid.cpp
+++ b/src/mathed/InsetMathGrid.cpp
@@ -1838,4 +1838,29 @@ bool InsetMathGrid::getStatus(Cursor & cur, FuncRequest const & cmd,
}
+// static
+char InsetMathGrid::colAlign(HullType type, col_type col)
+{
+ switch (type) {
+ case hullEqnArray:
+ return "rcl"[col % 3];
+
+ case hullMultline:
+ case hullGather:
+ return 'c';
+
+ case hullAlign:
+ case hullAlignAt:
+ case hullXAlignAt:
+ case hullXXAlignAt:
+ case hullFlAlign:
+ return "rl"[col & 1];
+
+ default:
+ return 'c';
+ }
+}
+
+
+
} // namespace lyx
diff --git a/src/mathed/InsetMathGrid.h b/src/mathed/InsetMathGrid.h
index bd3066d..709f492 100644
--- a/src/mathed/InsetMathGrid.h
+++ b/src/mathed/InsetMathGrid.h
@@ -258,10 +258,11 @@ protected:
virtual docstring eocString(col_type col, col_type lastcol) const;
/// splits cells and shifts right part to the next cell
void splitCell(Cursor & cur);
- /// Column aligmment for display of cell \p idx.
+ /// Column alignment for display of cell \p idx.
/// Must not be written to file!
virtual char displayColAlign(idx_type idx) const;
-
+ /// The value of a fixed col align for a certain hull type
+ static char colAlign(HullType type, col_type col);
/// row info.
/// rowinfo_[nrows()] is a dummy row used only for hlines.
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index 097a344..b859999 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -349,28 +349,34 @@ bool InsetMathHull::idxLast(Cursor & cur) const
}
+//FIXME: This has probably no effect and can be removed.
char InsetMathHull::defaultColAlign(col_type col)
{
- if (type_ == hullEqnArray)
- return "rcl"[col];
- if (type_ == hullMultline)
- return 'c';
- if (type_ == hullGather)
- return 'c';
- if (type_ >= hullAlign)
- return "rl"[col & 1];
- return 'c';
+ return colAlign(type_, col);
}
char InsetMathHull::displayColAlign(idx_type idx) const
{
- if (type_ == hullMultline) {
+ switch (type_) {
+ case hullMultline: {
row_type const r = row(idx);
if (r == 0)
return 'l';
if (r == nrows() - 1)
return 'r';
+ return 'c';
+ }
+ case hullEqnArray:
+ case hullGather:
+ case hullAlign:
+ case hullAlignAt:
+ case hullXAlignAt:
+ case hullXXAlignAt:
+ case hullFlAlign:
+ return colAlign(type_, col(idx));
+ default:
+ break;
}
return InsetMathGrid::displayColAlign(idx);
}
@@ -1241,6 +1247,27 @@ void InsetMathHull::setType(HullType type)
}
+bool InsetMathHull::isMutable(HullType type)
+{
+ switch (type) {
+ case hullNone:
+ case hullSimple:
+ case hullEquation:
+ case hullEqnArray:
+ case hullAlign:
+ case hullFlAlign:
+ case hullAlignAt:
+ case hullXAlignAt:
+ case hullXXAlignAt:
+ case hullMultline:
+ case hullGather:
+ return true;
+ default:
+ return false;
+ }
+}
+
+
void InsetMathHull::mutate(HullType newtype)
{
//lyxerr << "mutating from '" << type_ << "' to '" << newtype << "'" << endl;
diff --git a/src/mathed/InsetMathSplit.cpp b/src/mathed/InsetMathSplit.cpp
index 5c425fb..28a339f 100644
--- a/src/mathed/InsetMathSplit.cpp
+++ b/src/mathed/InsetMathSplit.cpp
@@ -48,20 +48,41 @@ Inset * InsetMathSplit::clone() const
}
+//FIXME: This has probably no effect and can be removed.
char InsetMathSplit::defaultColAlign(col_type col)
{
- if (name_ == "split")
- return 'l';
if (name_ == "gathered")
return 'c';
- if (name_ == "aligned" || name_ == "align")
- return (col & 1) ? 'l' : 'r';
- if (name_ == "alignedat")
- return (col & 1) ? 'l' : 'r';
+ if (name_ == "lgathered")
+ return 'l';
+ if (name_ == "rgathered")
+ return 'r';
+ if (name_ == "split"
+ || name_ == "aligned"
+ || name_ == "align"
+ || name_ == "alignedat")
+ return colAlign(hullAlign, col);
return 'l';
}
+char InsetMathSplit::displayColAlign(idx_type idx) const
+{
+ if (name_ == "gathered")
+ return 'c';
+ if (name_ == "lgathered")
+ return 'l';
+ if (name_ == "rgathered")
+ return 'r';
+ if (name_ == "split"
+ || name_ == "aligned"
+ || name_ == "align"
+ || name_ == "alignedat")
+ return colAlign(hullAlign, col(idx));
+ return InsetMathGrid::displayColAlign(idx);
+}
+
+
void InsetMathSplit::draw(PainterInfo & pi, int x, int y) const
{
InsetMathGrid::draw(pi, x, y);
@@ -86,6 +107,10 @@ bool InsetMathSplit::getStatus(Cursor & cur, FuncRequest const & cmd,
flag.setEnabled(false);
return true;
}
+ if (s == "align-left" || s == "align-center" || s == "align-right") {
+ flag.setEnabled(false);
+ return true;
+ }
break;
}
default:
diff --git a/src/mathed/InsetMathSplit.h b/src/mathed/InsetMathSplit.h
index b0ff437..1b2aa23 100644
--- a/src/mathed/InsetMathSplit.h
+++ b/src/mathed/InsetMathSplit.h
@@ -43,6 +43,8 @@ public:
///
char defaultColAlign(col_type);
///
+ char displayColAlign(idx_type idx) const;
+ ///
InsetCode lyxCode() const { return MATH_SPLIT_CODE; }
private:
--
2.1.4
>From 0d8a7bcf01f15ad37baa464fa90e25769774fa7c Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Sun, 20 Dec 2015 20:56:34 +0000
Subject: [PATCH 2/2] Fix the display of column spacing in AMS environments
AMS align environment should have some spacing between odd and even columns.
Add a new virtual method displayColSpace() to InsetMathGrid, InsetMathHull and
InsetMathSplit.
---
src/mathed/InsetMathGrid.cpp | 40 ++++++++++++++++++++++++++++++++++++----
src/mathed/InsetMathGrid.h | 8 ++++++++
src/mathed/InsetMathHull.cpp | 7 +++++++
src/mathed/InsetMathHull.h | 2 ++
src/mathed/InsetMathSplit.cpp | 11 +++++++++++
src/mathed/InsetMathSplit.h | 2 ++
6 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/src/mathed/InsetMathGrid.cpp b/src/mathed/InsetMathGrid.cpp
index fca5722..2ea35a4 100644
--- a/src/mathed/InsetMathGrid.cpp
+++ b/src/mathed/InsetMathGrid.cpp
@@ -486,7 +486,7 @@ void InsetMathGrid::metrics(MetricsInfo & mi, Dimension & dim) const
colinfo_[col].offset_ =
colinfo_[col - 1].offset_ +
colinfo_[col - 1].width_ +
- colinfo_[col - 1].skip_ +
+ displayColSpace(col - 1) +
colsep() +
colinfo_[col].lines_ * vlinesep();
}
@@ -508,7 +508,7 @@ void InsetMathGrid::metrics(MetricsInfo & mi, Dimension & dim) const
int const nextoffset =
colinfo_[first].offset_ +
wid +
- colinfo_[last].skip_ +
+ displayColSpace(last) +
colsep() +
colinfo_[last+1].lines_ * vlinesep();
int const dx = nextoffset - colinfo_[last+1].offset_;
@@ -741,7 +741,7 @@ void InsetMathGrid::metricsT(TextMetricsInfo const & mi, Dimension & dim) const
colinfo_[col].offset_ =
colinfo_[col - 1].offset_ +
colinfo_[col - 1].width_ +
- colinfo_[col - 1].skip_ +
+ displayColSpace(col - 1) +
1 ; //colsep() +
//colinfo_[col].lines_ * vlinesep();
}
@@ -953,7 +953,7 @@ int InsetMathGrid::cellWidth(idx_type idx) const
col_type c2 = c1 + ncellcols(idx);
return colinfo_[c2].offset_
- colinfo_[c1].offset_
- - colinfo_[c2].skip_
+ - displayColSpace(c2)
- colsep()
- colinfo_[c2].lines_ * vlinesep();
}
@@ -1378,6 +1378,11 @@ char InsetMathGrid::displayColAlign(idx_type idx) const
}
+int InsetMathGrid::displayColSpace(col_type col) const
+{
+ return colinfo_[col].skip_;
+}
+
void InsetMathGrid::doDispatch(Cursor & cur, FuncRequest & cmd)
{
//lyxerr << "*** InsetMathGrid: request: " << cmd << endl;
@@ -1862,5 +1867,32 @@ char InsetMathGrid::colAlign(HullType type, col_type col)
}
+//static
+int InsetMathGrid::colSpace(HullType type, col_type col)
+{
+ int alignInterSpace;
+ switch (type) {
+ case hullEqnArray:
+ return 5;
+
+ case hullAlign:
+ alignInterSpace = 20;
+ break;
+ case hullAlignAt:
+ alignInterSpace = 0;
+ break;
+ case hullXAlignAt:
+ alignInterSpace = 40;
+ break;
+ case hullXXAlignAt:
+ case hullFlAlign:
+ alignInterSpace = 60;
+ break;
+ default:
+ return 0;
+ }
+ return (col % 2) ? alignInterSpace : 0;
+}
+
} // namespace lyx
diff --git a/src/mathed/InsetMathGrid.h b/src/mathed/InsetMathGrid.h
index 709f492..7faf938 100644
--- a/src/mathed/InsetMathGrid.h
+++ b/src/mathed/InsetMathGrid.h
@@ -261,8 +261,16 @@ protected:
/// Column alignment for display of cell \p idx.
/// Must not be written to file!
virtual char displayColAlign(idx_type idx) const;
+ /// Column spacing for display of column \p col.
+ /// Must not be written to file!
+ virtual int displayColSpace(col_type col) const;
+
+ // The following two functions are used in InsetMathHull and
+ // InsetMathSplit.
/// The value of a fixed col align for a certain hull type
static char colAlign(HullType type, col_type col);
+ /// The value of a fixed col spacing for a certain hull type
+ static int colSpace(HullType type, col_type col);
/// row info.
/// rowinfo_[nrows()] is a dummy row used only for hlines.
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index b859999..95b6545 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -382,6 +382,13 @@ char InsetMathHull::displayColAlign(idx_type idx) const
}
+int InsetMathHull::displayColSpace(col_type col) const
+{
+ return colSpace(type_, col);
+}
+
+
+//FIXME: This has probably no effect and can be removed.
int InsetMathHull::defaultColSpace(col_type col)
{
if (type_ == hullAlign || type_ == hullAlignAt)
diff --git a/src/mathed/InsetMathHull.h b/src/mathed/InsetMathHull.h
index 9598ad4..1d82ed8 100644
--- a/src/mathed/InsetMathHull.h
+++ b/src/mathed/InsetMathHull.h
@@ -111,6 +111,8 @@ public:
///
int defaultColSpace(col_type col);
///
+ int displayColSpace(col_type col) const;
+ ///
char defaultColAlign(col_type col);
///
char displayColAlign(idx_type idx) const;
diff --git a/src/mathed/InsetMathSplit.cpp b/src/mathed/InsetMathSplit.cpp
index 28a339f..ff3eaaa 100644
--- a/src/mathed/InsetMathSplit.cpp
+++ b/src/mathed/InsetMathSplit.cpp
@@ -83,6 +83,17 @@ char InsetMathSplit::displayColAlign(idx_type idx) const
}
+int InsetMathSplit::displayColSpace(col_type col) const
+{
+ if (name_ == "split" || name_ == "aligned" || name_ == "align")
+ return colSpace(hullAlign, col);
+ if (name_ == "alignedat")
+ return colSpace(hullAlignAt, col);
+ return 0;
+}
+
+
+
void InsetMathSplit::draw(PainterInfo & pi, int x, int y) const
{
InsetMathGrid::draw(pi, x, y);
diff --git a/src/mathed/InsetMathSplit.h b/src/mathed/InsetMathSplit.h
index 1b2aa23..1226534 100644
--- a/src/mathed/InsetMathSplit.h
+++ b/src/mathed/InsetMathSplit.h
@@ -41,6 +41,8 @@ public:
///
int defaultColSpace(col_type) { return 0; }
///
+ int displayColSpace(col_type col) const;
+ ///
char defaultColAlign(col_type);
///
char displayColAlign(idx_type idx) const;
--
2.1.4
