Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 02/02/2016 23:23, Uwe Stöhr a écrit :

Am 02.02.2016 um 22:02 schrieb Guillaume Munch:


I am sceptical, since the other patches which made your software crash
were based against master. You can try again with the same diff since
InsetMathGrid.cpp with hash 12e871a... is now in master.


I tried your patch again and now it applied without an errors or
warnings.


I am surprised that it did not crash again! I think that you can still
open a bug report for your git client.


Moreover http://www.lyx.org/trac/ticket/9944 does now not appear.


That's reassuring. I suspected indeed that something else was
responsible for such a bug.



I can now also see what you changed. You patch works fine for me so far.
I played some time with the AMS features in math.lyx.

So from my point of view I give a +1.
I cannot judge the technical details behind your patch, I can only tell
that it works.



Thanks for the review. Jean-Marc seemed also confident with such a patch
a while ago, but at this stage I would need somebody who would review
the discussion with Georg about this patch and say that they agree with
my points regarding the technical details. Otherwise this is going to
wait for 2.2.1.

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 25/01/2016 00:32, Uwe Stöhr a écrit :

Am 25.01.2016 um 00:28 schrieb Guillaume Munch:

 > Georg's commit indeed fixed the alignment, but not the width of the

column, which sometimes makes it hard to see that the first line is
left-aligned and the last one right-aligned. So you are saying that the
fix of multline was not sufficient to correctly reflect the meaning of
multline. Since #1497 was open before Georg's patch which already
improved the situation, it might be good to add a comment to the ticket
explaining why the bug is not fixed yet and what more is expected.


I am confused now. Isn't it obvious that the alignment within LyX is not
correct? Attached is the screenshot of what I see in LyX. Compare this
with the attached PDF output.



Your example illustrates what I described.




See also the attached LyX file to see what is expected (of course
without the \hfills).


My patches add a fixed gap between pairs of columns, depending on the
AMS environment. You would like this gap to behave like horizontal
springs. My changes are all I really need to work, whereas you would
like to be more faithful to the pdf output (instant preview fulfils that
purpose for me).


As I understood it the WYSIWYM concept is to give the user the feedback
of how it would appear in the output - without helpers like instant
preview.

However, even with instant preview the alignment is not correct: The
"A=Bbb" must be directly below the word "desired"


My patches introduce feedback that is currently inexistent. Being 
further "photorealistic" in the way you ask requires to dig deeper into 
the code, so I am going to ask to open new enhancement requests once my 
code is committed. My changes certainly do not preclude further 
enhancements.

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 25/01/2016 23:49, Uwe Stöhr a écrit :

Am 25.01.2016 um 07:21 schrieb Stephan Witt:


I cannot apply your patch. I get the 2 attached errors and the
TortoiseGit crashes.


It tells me that it cannot find the revision 12e871a for
InsetMathGrid.cpp. And indeed this revsion does not exists.

Guillaume, can you please send a patch file with a valid revision?

[...]


I seems that your patches make problems for TortoiseGit (I use the
latest version 1.8.16.0).
This is strange since I never had this before and the patches of the
other developers apply without any problems.


I cannot see any strange or problematic contents.


The nonexistent revision is the problem.



I am sceptical, since the other patches which made your software crash
were based against master. You can try again with the same diff since
InsetMathGrid.cpp with hash 12e871a... is now in master.

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 02.02.2016 um 22:02 schrieb Guillaume Munch:


I am sceptical, since the other patches which made your software crash
were based against master. You can try again with the same diff since
InsetMathGrid.cpp with hash 12e871a... is now in master.


I tried your patch again and now it applied without an errors or 
warnings. Moreover http://www.lyx.org/trac/ticket/9944 does now not appear.


I can now also see what you changed. You patch works fine for me so far. 
I played some time with the AMS features in math.lyx.


So from my point of view I give a +1.
I cannot judge the technical details behind your patch, I can only tell 
that it works.


regards Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 25.01.2016 um 07:21 schrieb Stephan Witt:


I cannot apply your patch. I get the 2 attached errors and the TortoiseGit 
crashes.


It tells me that it cannot find the revision 12e871a for 
InsetMathGrid.cpp. And indeed this revsion does not exists.


Guillaume, can you please send a patch file with a valid revision?

(That TortoiseGit then crashes if one does not click OK in the error 
windows for a while is a bug.)



I was curious and tried to apply the patch file „align.diff“ from Guillaume.

With the Unix-utility patch I get 2 messages with hunk offset but it applies.
I have no problem with SourceTree too. (Did I mentioned SourceTree already? :)


Yes you did and I gave it now a try. This way I also found the program 
SmartGit which is also quite nice. Nevertheless I was not yet able to 
set up SourceTree so that I can checkout from LyX's git repository - 
some SSH key issues. I think I will find a solution in the web when I 
find time.



I seems that your patches make problems for TortoiseGit (I use the latest 
version 1.8.16.0).
This is strange since I never had this before and the patches of the other 
developers apply without any problems.


I cannot see any strange or problematic contents.


The nonexistent revision is the problem.

thanks and regards
Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 25.01.2016 um 00:28 schrieb Guillaume Munch:

> Georg's commit indeed fixed the alignment, but not the width of the

column, which sometimes makes it hard to see that the first line is
left-aligned and the last one right-aligned. So you are saying that the
fix of multline was not sufficient to correctly reflect the meaning of
multline. Since #1497 was open before Georg's patch which already
improved the situation, it might be good to add a comment to the ticket
explaining why the bug is not fixed yet and what more is expected.


I am confused now. Isn't it obvious that the alignment within LyX is not 
correct? Attached is the screenshot of what I see in LyX. Compare this 
with the attached PDF output.


See also the attached LyX file to see what is expected (of course 
without the \hfills).



My patches add a fixed gap between pairs of columns, depending on the
AMS environment. You would like this gap to behave like horizontal
springs. My changes are all I really need to work, whereas you would
like to be more faithful to the pdf output (instant preview fulfils that
purpose for me).


As I understood it the WYSIWYM concept is to give the user the feedback 
of how it would appear in the output - without helpers like instant preview.


However, even with instant preview the alignment is not correct: The 
"A=Bbb" must be directly below the word "desired"


regards Uwe
#LyX 2.2 created this file. For more info see http://www.lyx.org/
\lyxformat 504
\begin_document
\begin_header
\save_transient_properties true
\origin /systemlyxdir/../
\textclass article
\use_default_options true
\maintain_unincluded_children false
\language english
\language_package default
\inputencoding auto
\fontencoding global
\font_roman "default" "default"
\font_sans "default" "default"
\font_typewriter "default" "default"
\font_math "auto" "auto"
\font_default_family default
\use_non_tex_fonts false
\font_sc false
\font_osf false
\font_sf_scale 100 100
\font_tt_scale 100 100
\graphics default
\default_output_format default
\output_sync 0
\bibtex_command default
\index_command default
\paperfontsize default
\use_hyperref false
\papersize default
\use_geometry false
\use_package amsmath 1
\use_package amssymb 1
\use_package cancel 1
\use_package esint 1
\use_package mathdots 1
\use_package mathtools 1
\use_package mhchem 1
\use_package stackrel 1
\use_package stmaryrd 1
\use_package undertilde 1
\cite_engine basic
\cite_engine_type default
\biblio_style plain
\use_bibtopic false
\use_indices false
\paperorientation portrait
\suppress_date false
\justification true
\use_refstyle 1
\index Index
\shortcut idx
\color #008000
\end_index
\secnumdepth 3
\tocdepth 3
\paragraph_separation indent
\paragraph_indentation default
\quotes_language english
\papercolumns 1
\papersides 1
\paperpagestyle default
\tracking_changes false
\output_changes false
\html_math_output 0
\html_css_as_file 0
\html_be_strict false
\end_header

\begin_body

\begin_layout Standard
\begin_inset Formula 
\begin{multline*}
A=Bbb\\
C=De\\
E=Fee
\end{multline*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{flalign*}
A & =B & dos & sd\\
C & =D & as & sd\\
E & =F & asf & sd
\end{flalign*}

\end_inset


\begin_inset Formula 
\begin{align*}
a & s & s & s
\end{align*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{alignat*}{2}
s\hspace{1cm} & s & s & s
\end{alignat*}

\end_inset


\end_layout

\begin_layout Standard
desired result:
\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{multline*}
A=Bbb\hfill\hfill\hfill\hfill\hfill\hfill\hfill\hfill\hfill\hfill\hfill\\
C=De\\
E=Fee
\end{multline*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{flalign*}
A & =B\hfill\hfill\hfill\hfill\hfill & \hfill\hfill\hfill\hfill\hfill dos & sd\\
C & =D & as & sd\\
E & =F & asf & sd
\end{flalign*}

\end_inset


\begin_inset Formula 
\begin{align*}
a & s\hfill\hfill & \hfill\hfill s & s
\end{align*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{alignat*}{2}
s\hspace{1cm} & s & s & s
\end{alignat*}

\end_inset


\end_layout

\end_body
\end_document


multline-test.21.pdf
Description: Adobe PDF document

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 24.01.2016 um 23:29 schrieb Guillaume Munch:


Here are the patches as a single diff. Can you please confirm that this
triggers your issue?


I cannot apply your patch. I get the 2 attached errors and the 
TortoiseGit crashes.


I seems that your patches make problems for TortoiseGit (I use the 
latest version 1.8.16.0). This is strange since I never had this before 
and the patches of the other developers apply without any problems.


regards Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Stephan Witt]

Am 25.01.2016 um 01:37 schrieb Uwe Stöhr :
> 
> Am 24.01.2016 um 23:29 schrieb Guillaume Munch:
> 
>> Here are the patches as a single diff. Can you please confirm that this
>> triggers your issue?
> 
> I cannot apply your patch. I get the 2 attached errors and the TortoiseGit 
> crashes.

I was curious and tried to apply the patch file „align.diff“ from Guillaume.

With the Unix-utility patch I get 2 messages with hunk offset but it applies.
I have no problem with SourceTree too. (Did I mentioned SourceTree already? :) 

> I seems that your patches make problems for TortoiseGit (I use the latest 
> version 1.8.16.0).
> This is strange since I never had this before and the patches of the other 
> developers apply without any problems.

I cannot see any strange or problematic contents.

Stephan

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 22.01.2016 um 02:07 schrieb Uwe Stöhr:


I tried this but cannot see a problem in LyX 2.1.4 compared to LyX 2.2
with your patches applied.


I still had your patch applied and this caused a crash:
http://www.lyx.org/trac/ticket/9944

Can you please have a look and provide a new patch? (if possible only 
one patch file)


thanks and regards
Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 22.01.2016 um 20:30 schrieb Guillaume Munch:


Can you please tell me what issue you are referring to precisely in
#1497 then?


Open the testfile I attached to
http://www.lyx.org/trac/ticket/1497

Compare the appearance in lyx with that in the PDF output:

- for multline (first formula in the testfile) the first line starts in 
the output at the leftmost position, the last one at the rightmost. But 
in LyX not.


- for flalign (second formula in the testfile) the first 2 columns are 
in the output at the leftmost position, the last 2 columns at the 
rightmost. But in LyX not.


- for align (third formula in the testfile) the first 2 columns are in 
the output at a position in the middle between the middle of the page 
and the laft border, the last 2 columns are at the middle-right 
position. But in LyX not.


regards Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 24/01/2016 11:15, Uwe Stöhr a écrit :

Am 22.01.2016 um 02:07 schrieb Uwe Stöhr:


I tried this but cannot see a problem in LyX 2.1.4 compared to LyX 2.2
with your patches applied.


I still had your patch applied and this caused a crash:
http://www.lyx.org/trac/ticket/9944

Can you please have a look and provide a new patch? (if possible only
one patch file)




I cannot reproduce.

I placed the cursor at the bottom of the manual and scrolled back up to 
the top with the scrollbar. No crash happened. Did I miss anything?


Here are the patches as a single diff. Can you please confirm that this 
triggers your issue?



Guillaume
diff --git a/src/mathed/InsetMathGrid.cpp b/src/mathed/InsetMathGrid.cpp
index 12e871a..9161074 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;
@@ -1827,4 +1832,56 @@ 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';
+	}
+}
+
+
+//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 bd3066d..7faf938 100644
--- a/src/mathed/InsetMathGrid.h
+++ b/src/mathed/InsetMathGrid.h
@@ -258,10 +258,19 @@ 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;
+	/// 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 94e9ec4..82c6f98 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -349,33 +349,46 @@ 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 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 24/01/2016 12:04, Uwe Stöhr a écrit :

Am 22.01.2016 um 20:30 schrieb Guillaume Munch:


Can you please tell me what issue you are referring to precisely in
#1497 then?


Open the testfile I attached to
http://www.lyx.org/trac/ticket/1497

Compare the appearance in lyx with that in the PDF output:

- for multline (first formula in the testfile) the first line starts in
the output at the leftmost position, the last one at the rightmost. But
in LyX not.



Georg's commit indeed fixed the alignment, but not the width of the
column, which sometimes makes it hard to see that the first line is
left-aligned and the last one right-aligned. So you are saying that the
fix of multline was not sufficient to correctly reflect the meaning of
multline. Since #1497 was open before Georg's patch which already
improved the situation, it might be good to add a comment to the ticket
explaining why the bug is not fixed yet and what more is expected.




- for flalign (second formula in the testfile) the first 2 columns are
in the output at the leftmost position, the last 2 columns at the
rightmost. But in LyX not.

- for align (third formula in the testfile) the first 2 columns are in
the output at a position in the middle between the middle of the page
and the laft border, the last 2 columns are at the middle-right
position. But in LyX not.



My patches add a fixed gap between pairs of columns, depending on the
AMS environment. You would like this gap to behave like horizontal
springs. My changes are all I really need to work, whereas you would
like to be more faithful to the pdf output (instant preview fulfils that
purpose for me).

In addition, as far as I understand Georg tried to have such variable
gaps in the patch at http://www.lyx.org/trac/ticket/1861. The patch has
been unfinished for seven years, and what I propose instead is a simple
fix available now. The further improvements you would like to have seem
to require more work. This is why I will try to get my patches in 2.2.1
as is.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 21/01/2016 20:07, Uwe Stöhr a écrit :

Am 21.01.2016 um 07:06 schrieb Guillaume Munch:


The alignment is set correctly on opening. Wrong alignment appears when
modifying columns. To see the bug, try to see how the behaviour changes
when adding or deleting columns repeatedly after the first one.


I tried this but cannot see a problem in LyX 2.1.4 compared to LyX 2.2
with your patches applied. Could you give me a recipe?

Besides this, my bug report
http://www.lyx.org/trac/ticket/1497
is about the alignment within LyX and this is for me not changed with
your patch. Therefore I don't think that you fixed bug #1497 too.



Ok. Can you please tell me what issue you are referring to precisely in
#1497 then?

At first the bug is about wrong alignment in the "multline" environment.
But this has been fixed some time ago by Georg (the first line is
aligned to the left, the last line to the right).

Then you wrote "The wrong alignment of multiline equations inside LyX is
still in LyX 2.1.3", and with "multiline" I understood the other
multi-line math environments because to me multline is fixed.

In particular the above recipe is meant for align, aligned, etc.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 21/01/2016 20:07, Uwe Stöhr a écrit :

Am 21.01.2016 um 07:06 schrieb Guillaume Munch:


The alignment is set correctly on opening. Wrong alignment appears when
modifying columns. To see the bug, try to see how the behaviour changes
when adding or deleting columns repeatedly after the first one.


I tried this but cannot see a problem in LyX 2.1.4 compared to LyX 2.2
with your patches applied. Could you give me a recipe?

Besides this, my bug report
http://www.lyx.org/trac/ticket/1497
is about the alignment within LyX and this is for me not changed with
your patch. Therefore I don't think that you fixed bug #1497 too.



Can you please tell me what issue you are referring to precisely in
#1497 then?

At first the bug is about wrong alignment in the "multline" environment.
But this has been fixed some time ago by Georg (the first line is
aligned to the left, the last line to the right).

Then you wrote "The wrong alignment of multiline equations inside LyX is
still in LyX 2.1.3", and with "multiline" I understood the other
multi-line math environments because to me multline is fixed.

In particular the above recipe is meant for align, aligned, etc.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 21.01.2016 um 07:06 schrieb Guillaume Munch:


The alignment is set correctly on opening. Wrong alignment appears when
modifying columns. To see the bug, try to see how the behaviour changes
when adding or deleting columns repeatedly after the first one.


I tried this but cannot see a problem in LyX 2.1.4 compared to LyX 2.2 
with your patches applied. Could you give me a recipe?


Besides this, my bug report
http://www.lyx.org/trac/ticket/1497
is about the alignment within LyX and this is for me not changed with 
your patch. Therefore I don't think that you fixed bug #1497 too.


thanks and regards
Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 19/01/2016 20:10, Uwe Stöhr a écrit :

Am 19.01.2016 um 23:45 schrieb Guillaume Munch:


Note that I do not know if it applies cleanly now.


Hmm, seems the bug in my git client is to "apply patch serial". This
means to apply 2 or more patches at once. When I apply them one after
another no problem occurs.

I did this (applied patch 1 and then patch 2) and compiled LyX without
problems.

Result: I used the attached LyX file to test and cannot see a difference
with your patch compared to LyX 2.1.4 in the LyX screen.

If you view the PDF of the file you see what I expected as alignment.
Did I make a mistake?

thanks and regards
Uwe



The alignment is set correctly on opening. Wrong alignment appears when 
modifying columns. To see the bug, try to see how the behaviour changes 
when adding or deleting columns repeatedly after the first one.


Best regards
Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 11.01.2016 um 03:21 schrieb Guillaume Munch:


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.


I wanted to apply the patches at once and this way destroyed my complete 
git folder. Seems to be a bug in TortoiseGit. After the application it 
tells me that nothing was changed, also the Git log is empty. But all 
line endings are destroyed and the editor of MSVC cannot read the files 
anymore.


However, I think I fixed it, re-applied the patches and tried to compile 
but get:


  ..\..\..\src\mathed\InsetMathHull.cpp(1257): error C2039: 
'isMutable': Is no element of 'lyx::InsetMathHull' 
[D:\LyXGit\Master\compile-2010\src\mathed\mathed.vcxproj]


regards Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 19.01.2016 um 22:52 schrieb Uwe Stöhr:


However, I think I fixed it


No, my Git is completely broken. It tells me that I have never committed 
anything the last hour but I made several commits that played ping pong.


It also telly me that my tree is completely clean and that there are no 
changes. But when I try to compile LyX I get more than 4000 errors. 
Whenever I want to open a mathed source file MSVC refuses to open the 
files because of line ending errors it cannot resolve.


I deleted my git folder and started from scratch. But as soon as I apply 
your patches, I get again a broken git tree and it even tried to commit 
the applied changed immediately. I have never seen this before.


Can you therefore please help me out in testing if Git master is OK now 
(I committed from my second PC) and send one patch with the changes you 
want to have in?


sorry again, thanks and regards
Uwe

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 19/01/2016 16:52, Uwe Stöhr a écrit :

Am 11.01.2016 um 03:21 schrieb Guillaume Munch:


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.


I wanted to apply the patches at once and this way destroyed my complete
git folder. Seems to be a bug in TortoiseGit. After the application it
tells me that nothing was changed, also the Git log is empty. But all
line endings are destroyed and the editor of MSVC cannot read the files
anymore.



I was not quite expecting my patch to be unsafe in this way! Please tell 
us if you find the cause.





However, I think I fixed it, re-applied the patches and tried to compile
but get:

   ..\..\..\src\mathed\InsetMathHull.cpp(1257): error C2039:
'isMutable': Is no element of 'lyx::InsetMathHull'
[D:\LyXGit\Master\compile-2010\src\mathed\mathed.vcxproj]




You are right, this is something I just forgot to remove during the 
rebase (and belongs to the third patch not proposed here). Try the 
attached (if you fixed your other bug!).


Note that I do not know if it applies cleanly now. I tried to rebase 
against newest master but your manipulation error now inteferes. I will 
have to rebase by hand.
>From fd5691e6489c34db3120ba24ce03cd496aacb134 Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Sun, 13 Dec 2015 03:32:32 +
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  | 26 --
 src/mathed/InsetMathSplit.cpp | 37 +++--
 src/mathed/InsetMathSplit.h   |  2 ++
 5 files changed, 77 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..78137de 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;
 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Uwe Stöhr]

Am 19.01.2016 um 23:45 schrieb Guillaume Munch:


Note that I do not know if it applies cleanly now.


Hmm, seems the bug in my git client is to "apply patch serial". This 
means to apply 2 or more patches at once. When I apply them one after 
another no problem occurs.


I did this (applied patch 1 and then patch 2) and compiled LyX without 
problems.


Result: I used the attached LyX file to test and cannot see a difference 
with your patch compared to LyX 2.1.4 in the LyX screen.


If you view the PDF of the file you see what I expected as alignment. 
Did I make a mistake?


thanks and regards
Uwe
#LyX 2.1 created this file. For more info see http://www.lyx.org/
\lyxformat 474
\begin_document
\begin_header
\textclass article
\use_default_options true
\maintain_unincluded_children false
\language english
\language_package default
\inputencoding auto
\fontencoding global
\font_roman default
\font_sans default
\font_typewriter default
\font_math auto
\font_default_family default
\use_non_tex_fonts false
\font_sc false
\font_osf false
\font_sf_scale 100
\font_tt_scale 100
\graphics default
\default_output_format default
\output_sync 0
\bibtex_command default
\index_command default
\paperfontsize default
\use_hyperref false
\papersize default
\use_geometry false
\use_package amsmath 1
\use_package amssymb 1
\use_package cancel 1
\use_package esint 1
\use_package mathdots 1
\use_package mathtools 1
\use_package mhchem 1
\use_package stackrel 1
\use_package stmaryrd 1
\use_package undertilde 1
\cite_engine basic
\cite_engine_type default
\biblio_style plain
\use_bibtopic false
\use_indices false
\paperorientation portrait
\suppress_date false
\justification true
\use_refstyle 1
\index Index
\shortcut idx
\color #008000
\end_index
\secnumdepth 3
\tocdepth 3
\paragraph_separation indent
\paragraph_indentation default
\quotes_language english
\papercolumns 1
\papersides 1
\paperpagestyle default
\tracking_changes false
\output_changes false
\html_math_output 0
\html_css_as_file 0
\html_be_strict false
\end_header

\begin_body

\begin_layout Standard
\begin_inset Formula 
\begin{multline*}
A=Bbb\\
C=De\\
E=Fee
\end{multline*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{flalign*}
A & =B & dos & sd\\
C & =D & as & sd\\
E & =F & asf & sd
\end{flalign*}

\end_inset


\begin_inset Formula 
\begin{align*}
a & s & s & s
\end{align*}

\end_inset


\end_layout

\begin_layout Standard
\begin_inset Formula 
\begin{alignat*}{2}
s\hspace{1cm} & s & s & s
\end{alignat*}

\end_inset


\end_layout

\end_body
\end_document

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Guillaume Munch wrote:

> However, if you do not have the time, this is a sufficient reason by
> itself. I am not forcing you to read these patches, and you do not need
> an excuse.

time is unfortunately really an issue for me currently.

> Maybe one more thing we are in disagreement about is your implication
> that this is a secondary issue just because you (and other people)
> "simply got used to the wrong display".

Oh, I do not imply that this is secondary. We have many important bugs in 
trac that will not be fixed for 2.2.0, even some with patches. I only 
question the urgency of a fix for a bug that nobody has been working on for 
seven years. We simply have different points of view: You look more from a 
developer perspective "there is an annoying problem, I have a fix and I am 
sure it is safe => put it in". I look more from a project management 
perspective "we want to relase soon, we do not want to introduce regressions 
=> we cannot fix all bugs (not even all important ones) right now".


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 11/01/2016 21:41, Georg Baum a écrit :

Guillaume Munch wrote:


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.


OK, I see. My argument for removing defaultColAlign() was that we do not
want to have dead code, but I overlooked that it is still used in the base
class, so the only thing which could be removed is the "virtual" keyword
(but I don't have a strong opinion on that).


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.


I believe that I understood the intention of your patch. However, I found
surprising behaviour in mathed in the past, and the motivation for my
comments was to ensure that you do not trap into this pitfall as well.


(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.)


There are some hidden parts of mathed which have no user interface. I would
not bet that these settings are not written under any circumstances (but I
agree that a bug in the usual cases would have been found earlier, so we can
assume they work).


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 am sorry, you are right, I was not careful anymore at the end of the
message.


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ß".


Well, in total it is supposed to make fun (at least for me, otherwise I
would stop contributing).


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.


I started to review the patch, but stopped when I found a difference between
the old and new behaviour of InsetMathHull::defaultColAlign() for
hullRegexp, since I don't have so much time at the moment. Again, this
combination is most likely not used, but "most likely" is not enough IMHO
for a beta.

Why is it so important to fix this bug before 2.2.0 when it is known for so
many years already? BTW, it did annoy me a lot when I wrote my thesis
(therefore my attempt to fix it many vears ago), but since I did not find a
proper solution at that time I simply got used to the wrong display (which
was not too difficult).

If you were proposing to fix the bug at the beginning of the 2.3 development
cycle then we could skip most parts of this discussion and you could simply
submit it. As I understand it, we do all agree that the proposed change is
good, the only differences are in judging the risk.





As you have noticed, this hull is constrained to a single cell. It
gets right-aligned because the commit that added hullRegexp after
hullAlign in the HullType enum did not change anything to
defaultColAlign() which reads:
if (type_ >= hullAlign)
return "rl"[col & 1];
This hull is only ever used in the Advanced Search & Replace panel. It
is not used for output and not meant to appear in a lyx file (in fact if
you try to save it in a file using copy/paste, then the parsing on
opening is broken, in a way that makes me confident that it has never
been used in a lyx document). So it is not about the alignment being
"most likely" not used. It is about the fact that this hull serves a
limited purpose that obviously has nothing to do with this
right-alignment (and that can be tested in a straightforward manner).

However, if you do not have the time, this is a sufficient reason by
itself. I am not forcing you to read these patches, and you do not need
an excuse.

I am confident in the two patches and I will wait and see if somebody
else is ready to vouch for it.



Also, I find that the fact that the issue matters and that the patch is
straightforward and risk-free have been good enough reasons to solve it
at this stage.

Maybe one more thing we are in disagreement about is your implication
that this is a secondary issue just because you (and other people)
"simply got used to the wrong display". Currently, new users not only
have to learn lyx ways but also have to get accustomed to its many
quirks 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Guillaume Munch wrote:

> 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.

OK, I see. My argument for removing defaultColAlign() was that we do not 
want to have dead code, but I overlooked that it is still used in the base 
class, so the only thing which could be removed is the "virtual" keyword 
(but I don't have a strong opinion on that).

> 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.

I believe that I understood the intention of your patch. However, I found 
surprising behaviour in mathed in the past, and the motivation for my 
comments was to ensure that you do not trap into this pitfall as well.
 
> (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.)

There are some hidden parts of mathed which have no user interface. I would 
not bet that these settings are not written under any circumstances (but I 
agree that a bug in the usual cases would have been found earlier, so we can 
assume they work).

> 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 am sorry, you are right, I was not careful anymore at the end of the 
message.

> 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ß".

Well, in total it is supposed to make fun (at least for me, otherwise I 
would stop contributing).

> 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.

I started to review the patch, but stopped when I found a difference between 
the old and new behaviour of InsetMathHull::defaultColAlign() for 
hullRegexp, since I don't have so much time at the moment. Again, this 
combination is most likely not used, but "most likely" is not enough IMHO 
for a beta.

Why is it so important to fix this bug before 2.2.0 when it is known for so 
many years already? BTW, it did annoy me a lot when I wrote my thesis 
(therefore my attempt to fix it many vears ago), but since I did not find a 
proper solution at that time I simply got used to the wrong display (which 
was not too difficult).

If you were proposing to fix the bug at the beginning of the 2.3 development 
cycle then we could skip most parts of this discussion and you could simply 
submit it. As I understand it, we do all agree that the proposed change is 
good, the only differences are in judging the risk.


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Jean-Marc Lasgouttes wrote:

> What is BTW the reason why you skip recordUndo in these cases?

My understanding is that these changes do not modify the document if 
store_user_prefs is false, and therefore nothing can be undone.


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 11/01/2016 20:30, Georg Baum a écrit :

Jean-Marc Lasgouttes wrote:


What is BTW the reason why you skip recordUndo in these cases?


My understanding is that these changes do not modify the document if
store_user_prefs is false, and therefore nothing can be undone.




Yes. BTW this corresponds to tested behaviour because there was no 
recordUndo until the recent patch by Richard.

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 10/01/2016 23:56, Guillaume Munch a écrit :

Some questions on the second patch:
1/ can OUTPUT_CHANGES be toggled for a read-only document if user prefs
are not stored?


No. Currently one cannot enable "output changes" on a
read-only document (on 2.1.5dev). If you think that this is a bug, then
it is separate from what is being achieved with the patch. I do not
think that store_user_prefs should be have anything to do with the
possibility of enabling output_changes on a read-only document.


Well, I thought about that because you do not trigger undo in this case. 
This is because the document is not actually modified. Thus it could 
work in read only. There is somevalue in doing that for output change or 
for branches, for example.


What is BTW the reason why you skip recordUndo in these cases?

JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Guillaume Munch wrote:

> Le 10/01/2016 19:08, Georg Baum a écrit :
>>
>> You don't need to do anything in tex2lyx besides writing the new
>> parameter with a default value as you did in the patch.
> 
> I mean it for the tests, as I have no familiarity with the latter.

This is easy: How to update the tests is described in Development.lyx, 
section 3.2.2 (depends on the build system and whether MSVC is used or not).

Since you only added a header line in your format change, it is easy to 
verify that the updated references are correct: The diff for eeach .lyx file 
should show the new format number and the added header line and nothing 
else.


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

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 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Guillaume Munch wrote:

> I agree that the current situation regarding \output_change and
> \tracking_change is bad and should be fixed. I gave what I think is the
> proper solution, taking into account the debate that was on the list.
> The patch is very simple (see the earlier message:
> http://article.gmane.org/gmane.editors.lyx.devel/159493 and:
> http://www.lyx.org/trac/ticket/9841). Now what we would need for such a
> change is if developers who took part in the discussion looked and say:
> yes, this is the proper solution. I do not see this happening right now.

I guess because of the mailing list outage the mail has been forgotten.

I looked at the patches in http://www.lyx.org/trac/ticket/9841, and I think 
it is a good implementation of the consensus that has been reached earlier. 
You have a +1 from me, but because this has been discussed with some strong 
opinions I suggest that there should be more reviews.

> Then, what is a proper solution? You want a file format increase +
> lyx2lyx + tex2lyx changes? I am more familiar with format increase now
> so it's easier for me, although: 1) please not while I have another
> format change pending because these things do not rebase very well, and
> 2) I will need help for tex2lyx. Another solution is to revert as I have
> suggested.

You don't need to do anything in tex2lyx besides writing the new parameter 
with a default value as you did in the patch.


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 08/01/16 23:05, Guillaume Munch a écrit :

I agree that the current situation regarding \output_change and
\tracking_change is bad and should be fixed. I gave what I think is the
proper solution, taking into account the debate that was on the list.
The patch is very simple (see the earlier message:
http://article.gmane.org/gmane.editors.lyx.devel/159493 and:
http://www.lyx.org/trac/ticket/9841). Now what we would need for such a
change is if developers who took part in the discussion looked and say:
yes, this is the proper solution. I do not see this happening right now.
Then, what is a proper solution? You want a file format increase +
lyx2lyx + tex2lyx changes? I am more familiar with format increase now
so it's easier for me, although: 1) please not while I have another
format change pending because these things do not rebase very well, and
2) I will need help for tex2lyx. Another solution is to revert as I have
suggested.


Some questions on the second patch:
1/ can OUTPUT_CHANGES be toggled for a read-only document if user prefs 
are not stored?


2/ when prefs are not stored, they are replaced by "false". Will this 
always make sense to users? This is just a question that popped up in my 
head, since we say "do not store" but something is actually stored.

It might just be a matter of wording of the option, actually.

JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 10/01/2016 19:08, Georg Baum a écrit :
>

I looked at the patches in http://www.lyx.org/trac/ticket/9841, and I think
it is a good implementation of the consensus that has been reached earlier.
You have a +1 from me, but because this has been discussed with some strong
opinions I suggest that there should be more reviews.


Ok thanks, I will wait for more "+1".

To make it clear, do we agree that the meaning of 
"\store_user_preferences" is meant to grow over time if we stumble upon 
similar scenarios? This means that for people have chosen to set 
\store_user_preferences to false, the storage of certain preferences 
might be moved to a per-user-per-document preference between releases.




You don't need to do anything in tex2lyx besides writing the new parameter
with a default value as you did in the patch.


I mean it for the tests, as I have no familiarity with the latter.

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 10/01/2016 21:11, Jean-Marc Lasgouttes a écrit :

Le 08/01/16 23:05, Guillaume Munch a écrit :

I agree that the current situation regarding \output_change and
\tracking_change is bad and should be fixed. I gave what I think is the
proper solution, taking into account the debate that was on the list.
The patch is very simple (see the earlier message:
http://article.gmane.org/gmane.editors.lyx.devel/159493 and:
http://www.lyx.org/trac/ticket/9841). Now what we would need for such a
change is if developers who took part in the discussion looked and say:
yes, this is the proper solution. I do not see this happening right now.
Then, what is a proper solution? You want a file format increase +
lyx2lyx + tex2lyx changes? I am more familiar with format increase now
so it's easier for me, although: 1) please not while I have another
format change pending because these things do not rebase very well, and
2) I will need help for tex2lyx. Another solution is to revert as I have
suggested.


Some questions on the second patch:
1/ can OUTPUT_CHANGES be toggled for a read-only document if user prefs
are not stored?


No. Currently one cannot enable "output changes" on a
read-only document (on 2.1.5dev). If you think that this is a bug, then
it is separate from what is being achieved with the patch. I do not
think that store_user_prefs should be have anything to do with the
possibility of enabling output_changes on a read-only document.



2/ when prefs are not stored, they are replaced by "false". Will this
always make sense to users? This is just a question that popped up in my
head, since we say "do not store" but something is actually stored.
It might just be a matter of wording of the option, actually.



For the two settings under consideration this makes sense, however the
long-term idea is to store as a per-user-per-document setting, local to
the machine (like the cursor location on opening). Hence the phrasing
"do not store *to file*".

I had a look at the implementation of per-user-per-document settings and
it has to be enhanced before being used in this manner, so for the
moment we still read the value from the lyx file.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 06/01/2016 20:51, Georg Baum a écrit :


BTW, a proper solution for 9841 is much more important than these alignment
issues IMHO. The current situation is worse than before dc016de34ea.




I agree that the current situation regarding \output_change and 
\tracking_change is bad and should be fixed. I gave what I think is the 
proper solution, taking into account the debate that was on the list. 
The patch is very simple (see the earlier message: 
http://article.gmane.org/gmane.editors.lyx.devel/159493 and: 
http://www.lyx.org/trac/ticket/9841). Now what we would need for such a 
change is if developers who took part in the discussion looked and say: 
yes, this is the proper solution. I do not see this happening right now. 
Then, what is a proper solution? You want a file format increase + 
lyx2lyx + tex2lyx changes? I am more familiar with format increase now 
so it's easier for me, although: 1) please not while I have another 
format change pending because these things do not rebase very well, and 
2) I will need help for tex2lyx. Another solution is to revert as I have 
suggested.



Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

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.
 
> 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.

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.


I apologize for being such a nitpicker and "Spaßbremse", 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.

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.

BTW, a proper solution for 9841 is much more important than these alignment 
issues IMHO. The current situation is worse than before dc016de34ea.


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Georg Baum]

Jean-Marc Lasgouttes wrote:

> Le 01/01/16 18:57, Guillaume Munch a écrit :
>> Dear list,
>>
>> Here's the latest version of the patch after discussion during the ML
>> shortage here: http://www.lyx.org/trac/ticket/9908. As I explained
>> there, this only affects the display. Jean-Marc, was your comment an
>> encouragement to push?
> 
> Yes , sort of. I cannot tell that it really works, but if it does not,
> it would not be too dangerous :)

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).


Georg

Re: [patch] Display the correct horizontal alignment in AMS environments

[Richard Heck]

On 01/03/2016 08:30 AM, Jean-Marc Lasgouttes wrote:
> Le 03/01/2016 12:04, Georg Baum a écrit :
>>> Yes , sort of. I cannot tell that it really works, but if it does not,
>>> it would not be too dangerous :)
>>
>> 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).
>
> You are probably right. Note that some parts of this patch are just
> spacing changes and other parts are early returns inside switch cases.
> It would be nice to avoid/separate these changes from the rest to see
> better what the patch does.

Yes, this is always helpful. There are ways to use git to do this

Richard

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 03/01/2016 12:04, Georg Baum a écrit :

Yes , sort of. I cannot tell that it really works, but if it does not,
it would not be too dangerous :)


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).


You are probably right. Note that some parts of this patch are just 
spacing changes and other parts are early returns inside switch cases. 
It would be nice to avoid/separate these changes from the rest to see 
better what the patch does.


Anyway, since this is not a format change, there is no harm with waiting 
a bit with this change.


JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 03/01/2016 11:04, Georg Baum a écrit :

Jean-Marc Lasgouttes wrote:


Le 01/01/16 18:57, Guillaume Munch a écrit :

Dear list,

Here's the latest version of the patch after discussion during the ML
shortage here: http://www.lyx.org/trac/ticket/9908. As I explained
there, this only affects the display. Jean-Marc, was your comment an
encouragement to push?


Yes , sort of. I cannot tell that it really works, but if it does not,
it would not be too dangerous :)


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?

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.

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.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 03/01/2016 13:30, Jean-Marc Lasgouttes a écrit :

Le 03/01/2016 12:04, Georg Baum a écrit :

Yes , sort of. I cannot tell that it really works, but if it does not,
it would not be too dangerous :)


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).


You are probably right. Note that some parts of this patch are just
spacing changes and other parts are early returns inside switch cases.
It would be nice to avoid/separate these changes from the rest to see
better what the patch does.


I thought that the separation in three patches was already enough to see
what each does... Which one are you discussing exactly? I fail to see
what you would like to see separated/avoided, can you give me an example?




Anyway, since this is not a format change, there is no harm with waiting
a bit with this change.




Agreed. It should be in before beta, though.


Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 03/01/16 19:27, Guillaume Munch a écrit :

I thought that the separation in three patches was already enough to see
what each does... Which one are you discussing exactly? I fail to see
what you would like to see separated/avoided, can you give me an example?


You are right of course. Sorry about that.

JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 01/01/16 18:57, Guillaume Munch a écrit :

Dear list,

Here's the latest version of the patch after discussion during the ML
shortage here: http://www.lyx.org/trac/ticket/9908. As I explained
there, this only affects the display. Jean-Marc, was your comment an
encouragement to push?


Yes , sort of. I cannot tell that it really works, but if it does not, 
it would not be too dangerous :)


JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Dear list,

Here's the latest version of the patch after discussion during the ML
shortage here: http://www.lyx.org/trac/ticket/9908. As I explained
there, this only affects the display. Jean-Marc, was your comment an
encouragement to push?


Guillaume
>From 1361409246f89b043bc215183a171d469dccf691 Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Mon, 14 Dec 2015 01:54:27 +
Subject: [PATCH 1/3] Sanitize InsetMathHull and add a check for mutability in
 LFUN_MATH_MUTATE

The transformation is purely mechanical (apart for the addition of
isMutable()). Remove in particular all comparisons < and >= involving HullType.

Add a guard to make sure that mutate() only operates on types it has been
designed for. Then I figured I could use this new knowledge to give feedback
when math-mutate is not implemented via getStatus(). (To test this, insert a
regexp in Advanced Search & Replace and try to change it into a standard
equation via the contextual menu.)
---
 src/mathed/InsetMath.h   |   3 +-
 src/mathed/InsetMathHull.cpp | 357 +++
 src/mathed/InsetMathHull.h   |   2 +
 3 files changed, 231 insertions(+), 131 deletions(-)

diff --git a/src/mathed/InsetMath.h b/src/mathed/InsetMath.h
index bd72863..deb6915 100644
--- a/src/mathed/InsetMath.h
+++ b/src/mathed/InsetMath.h
@@ -34,7 +34,8 @@ enum HullType {
 	hullFlAlign,
 	hullMultline,
 	hullGather,
-	hullRegexp
+	hullRegexp,
+	hullUnknown
 };
 
 HullType hullType(docstring const & name);
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index 097a344..1fa9fbd 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -79,16 +79,18 @@ namespace {
 	int getCols(HullType type)
 	{
 		switch (type) {
-			case hullEqnArray:
-return 3;
-			case hullAlign:
-			case hullFlAlign:
-			case hullAlignAt:
-			case hullXAlignAt:
-			case hullXXAlignAt:
-return 2;
-			default:
-return 1;
+		case hullEqnArray:
+			return 3;
+
+		case hullAlign:
+		case hullFlAlign:
+		case hullAlignAt:
+		case hullXAlignAt:
+		case hullXXAlignAt:
+			return 2;
+
+		default:
+			return 1;
 		}
 	}
 
@@ -128,28 +130,29 @@ HullType hullType(docstring const & s)
 	if (s == "flalign")   return hullFlAlign;
 	if (s == "regexp")return hullRegexp;
 	lyxerr << "unknown hull type '" << to_utf8(s) << "'" << endl;
-	return HullType(-1);
+	return hullUnknown;
 }
 
 
 docstring hullName(HullType type)
 {
 	switch (type) {
-		case hullNone:   return from_ascii("none");
-		case hullSimple: return from_ascii("simple");
-		case hullEquation:   return from_ascii("equation");
-		case hullEqnArray:   return from_ascii("eqnarray");
-		case hullAlign:  return from_ascii("align");
-		case hullAlignAt:return from_ascii("alignat");
-		case hullXAlignAt:   return from_ascii("xalignat");
-		case hullXXAlignAt:  return from_ascii("xxalignat");
-		case hullMultline:   return from_ascii("multline");
-		case hullGather: return from_ascii("gather");
-		case hullFlAlign:return from_ascii("flalign");
-		case hullRegexp: return from_ascii("regexp");
-		default:
-			lyxerr << "unknown hull type '" << type << "'" << endl;
-			return from_ascii("none");
+	case hullNone:   return from_ascii("none");
+	case hullSimple: return from_ascii("simple");
+	case hullEquation:   return from_ascii("equation");
+	case hullEqnArray:   return from_ascii("eqnarray");
+	case hullAlign:  return from_ascii("align");
+	case hullAlignAt:return from_ascii("alignat");
+	case hullXAlignAt:   return from_ascii("xalignat");
+	case hullXXAlignAt:  return from_ascii("xxalignat");
+	case hullMultline:   return from_ascii("multline");
+	case hullGather: return from_ascii("gather");
+	case hullFlAlign:return from_ascii("flalign");
+	case hullRegexp: return from_ascii("regexp");
+	case hullUnknown:
+	default:
+		lyxerr << "unknown hull type" << endl;
+		return from_ascii("unknown");
 	}
 }
 
@@ -326,10 +329,15 @@ Inset * InsetMathHull::editXY(Cursor & cur, int x, int y)
 
 InsetMath::mode_type InsetMathHull::currentMode() const
 {
-	if (type_ == hullNone)
+	switch (type_) {
+	case hullNone:
+	case hullUnknown:
 		return UNDECIDED_MODE;
+
 	// definitely math mode ...
-	return MATH_MODE;
+	default:
+		return MATH_MODE;
+	}
 }
 
 
@@ -351,15 +359,24 @@ bool InsetMathHull::idxLast(Cursor & cur) const
 
 char InsetMathHull::defaultColAlign(col_type col)
 {
-	if (type_ == hullEqnArray)
+	switch (type_) {
+	case hullEqnArray:
 		return "rcl"[col];
-	if (type_ == hullMultline)
+
+	case hullMultline:
+	case hullGather:
 		return 'c';
-	if (type_ == hullGather)
-		return 'c';
-	if (type_ >= hullAlign)
+
+	case hullAlign:
+	case hullFlAlign:
+	case hullAlignAt:
+	case hullXAlignAt:
+	case hullXXAlignAt:
 		return "rl"[col & 1];
-	return 'c';
+
+	default:
+		return 'c';
+	}
 }
 
 
@@ -378,45 +395,48 @@ char InsetMathHull::displayColAlign(idx_type idx) const
 
 int 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 17/12/2015 16:50, Guillaume Munch a écrit :

Le 14/12/2015 12:27, Jean-Marc Lasgouttes a écrit :

Le 14/12/2015 12:05, Guillaume Munch a écrit :

You're right, see now second patch attached. It's a fairly simple
change, only adding some information to Georg's displayColAlign(), in
addition to some mechanical refactoring and disabling of horizontal
alignment buttons which were wrongly enabled. See the new commit log.


Thanks for the patches. A couple remarks:
- in switches it would be better to avoid using default:. This way, when
adding a new hull type, we'll get a warning for each un-handled case and
we'll have to think about the right value.


I agree, and thanks for the information. I never know what I am allowed
to expect from c++ compilers, so I tend to imitate the style I find in
the LyX sources...


- if defaultColAlign and defaultColSpace are broken in some way, the
least we shell do is add a FIXME in the source. Deciding what to do with
them would be even better.


Yes: are these data used in output formats different from LaTeX (e.g.
LyXHTML)? Then we should add a FIXME. Otherwise they are useless and we
can remove them.



I concluded that defaultCol* were now superfluous (in addition to
broken), and there already are FIXMEs regarding the alignment in html,
so I removed them in the attached.
>From ddc5488d0a78b1a0a6fc7cced1bc621229b3f1da Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Mon, 14 Dec 2015 01:54:27 +
Subject: [PATCH 1/3] Sanitize InsetMathHull and add a check for mutability in
 LFUN_MATH_MUTATE

The transformation is purely mechanical (apart for the addition of
isMutable()). Remove in particular all comparisons < and >= involving HullType.

Add a guard to make sure that mutate() only operates on types it has been
designed for. Then I figured I could use this new knowledge to give feedback
when math-mutate is not implemented via getStatus(). (To test this, insert a
regexp in Advanced Search & Replace and try to change it into a standard
equation via the contextual menu.)
---
 src/mathed/InsetMath.h   |   3 +-
 src/mathed/InsetMathHull.cpp | 357 +++
 src/mathed/InsetMathHull.h   |   2 +
 3 files changed, 231 insertions(+), 131 deletions(-)

diff --git a/src/mathed/InsetMath.h b/src/mathed/InsetMath.h
index bd72863..deb6915 100644
--- a/src/mathed/InsetMath.h
+++ b/src/mathed/InsetMath.h
@@ -34,7 +34,8 @@ enum HullType {
 	hullFlAlign,
 	hullMultline,
 	hullGather,
-	hullRegexp
+	hullRegexp,
+	hullUnknown
 };
 
 HullType hullType(docstring const & name);
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index 097a344..1fa9fbd 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -79,16 +79,18 @@ namespace {
 	int getCols(HullType type)
 	{
 		switch (type) {
-			case hullEqnArray:
-return 3;
-			case hullAlign:
-			case hullFlAlign:
-			case hullAlignAt:
-			case hullXAlignAt:
-			case hullXXAlignAt:
-return 2;
-			default:
-return 1;
+		case hullEqnArray:
+			return 3;
+
+		case hullAlign:
+		case hullFlAlign:
+		case hullAlignAt:
+		case hullXAlignAt:
+		case hullXXAlignAt:
+			return 2;
+
+		default:
+			return 1;
 		}
 	}
 
@@ -128,28 +130,29 @@ HullType hullType(docstring const & s)
 	if (s == "flalign")   return hullFlAlign;
 	if (s == "regexp")return hullRegexp;
 	lyxerr << "unknown hull type '" << to_utf8(s) << "'" << endl;
-	return HullType(-1);
+	return hullUnknown;
 }
 
 
 docstring hullName(HullType type)
 {
 	switch (type) {
-		case hullNone:   return from_ascii("none");
-		case hullSimple: return from_ascii("simple");
-		case hullEquation:   return from_ascii("equation");
-		case hullEqnArray:   return from_ascii("eqnarray");
-		case hullAlign:  return from_ascii("align");
-		case hullAlignAt:return from_ascii("alignat");
-		case hullXAlignAt:   return from_ascii("xalignat");
-		case hullXXAlignAt:  return from_ascii("xxalignat");
-		case hullMultline:   return from_ascii("multline");
-		case hullGather: return from_ascii("gather");
-		case hullFlAlign:return from_ascii("flalign");
-		case hullRegexp: return from_ascii("regexp");
-		default:
-			lyxerr << "unknown hull type '" << type << "'" << endl;
-			return from_ascii("none");
+	case hullNone:   return from_ascii("none");
+	case hullSimple: return from_ascii("simple");
+	case hullEquation:   return from_ascii("equation");
+	case hullEqnArray:   return from_ascii("eqnarray");
+	case hullAlign:  return from_ascii("align");
+	case hullAlignAt:return from_ascii("alignat");
+	case hullXAlignAt:   return from_ascii("xalignat");
+	case hullXXAlignAt:  return from_ascii("xxalignat");
+	case hullMultline:   return from_ascii("multline");
+	case hullGather: return from_ascii("gather");
+	case hullFlAlign:return from_ascii("flalign");
+	case hullRegexp: return from_ascii("regexp");
+	case hullUnknown:
+	default:
+		lyxerr << "unknown hull 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 14/12/2015 12:27, Jean-Marc Lasgouttes a écrit :

Le 14/12/2015 12:05, Guillaume Munch a écrit :

You're right, see now second patch attached. It's a fairly simple
change, only adding some information to Georg's displayColAlign(), in
addition to some mechanical refactoring and disabling of horizontal
alignment buttons which were wrongly enabled. See the new commit log.


Thanks for the patches. A couple remarks:
- in switches it would be better to avoid using default:. This way, when
adding a new hull type, we'll get a warning for each un-handled case and
we'll have to think about the right value.


I agree, and thanks for the information. I never know what I am allowed
to expect from c++ compilers, so I tend to imitate the style I find in
the LyX sources...


- if defaultColAlign and defaultColSpace are broken in some way, the
least we shell do is add a FIXME in the source. Deciding what to do with
them would be even better.


Yes: are these data used in output formats different from LaTeX (e.g. 
LyXHTML)? Then we should add a FIXME. Otherwise they are useless and we 
can remove them.



Guillaume

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 14/12/2015 12:05, Guillaume Munch a écrit :
> You're right, see now second patch attached. It's a fairly simple
> change, only adding some information to Georg's displayColAlign(), in
> addition to some mechanical refactoring and disabling of horizontal
> alignment buttons which were wrongly enabled. See the new commit log.

Thanks for the patches. A couple remarks:
- in switches it would be better to avoid using default:. This way, when
adding a new hull type, we'll get a warning for each un-handled case and
we'll have to think about the right value.
- if defaultColAlign and defaultColSpace are broken in some way, the
least we shell do is add a FIXME in the source. Deciding what to do with
them would be even better.

Unfortunately I do not have time until end of week for testing.

JMarc

Re: [patch] Display the correct horizontal alignment in AMS environments

[Guillaume Munch]

Le 13/12/2015 11:41, Jean-Marc Lasgouttes a écrit :

Le 13/12/2015 04:37, Guillaume Munch a écrit :

Dear list

This has been annoying me for a while. OK to push?


It would deserve a commit log with explanations of what this fixes. The
patch does many things it is difficult for me to say whether this is
correct.


You're right, see now second patch attached. It's a fairly simple
change, only adding some information to Georg's displayColAlign(), in
addition to some mechanical refactoring and disabling of horizontal
alignment buttons which were wrongly enabled. See the new commit log.

Moreover I saw that Georg has a related patch at
 (!!) so maybe he has an opinion
about this. Georg?


Anyway this could also be something that we keep for 2.2.x.


I am now done with any "big" patch for 2.2., so this leaves
enough time for discussion: more than a week for LFUN_TABLE_MODIFY in
another thread (it is subject to the feature freeze) and three weeks for
these UI bugfixes. For these, I know this looks big, but the actual
changes are small, the rest is mainly refactoring and cleaning (as you
prompted me to do).



FWIW, I also tend to be annoyed by bad on screen alignment, and the
change of ordering of enums seems to be a good catch. However, it would
be a good idea to get rid of the test on >= hullAlign and replace it
with a helper function. This will make the code more robust IMO.


This was a very good suggestion because the situation was even less 
clean than I imagined. I did that in the first patch attached. Please

have a look.

I also attach a third patch to apply a similar solution to fix the
spacing of the columns in AMS environments. They are all independent 
from one another.


>From 075989c35201ecb05fdb086ff66dee379f83019b Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Mon, 14 Dec 2015 01:54:27 +
Subject: [PATCH 1/3] Sanitize InsetMathHull and add a check for mutability in
 LFUN_MATH_MUTATE

The transformation is purely mechanical (apart for the addition of
isMutable()). Remove in particular all comparisons < and >= involving HullType.

Add a guard to make sure that mutate() only operates on types it has been
designed for. Then I figured I could use this new knowledge to give feedback
when math-mutate is not implemented via getStatus(). (To test this, insert a
regexp in Advanced Search & Replace and try to change it into a standard
equation via the contextual menu.)
---
 src/mathed/InsetMath.h   |   3 +-
 src/mathed/InsetMathHull.cpp | 346 +++
 src/mathed/InsetMathHull.h   |   2 +
 3 files changed, 223 insertions(+), 128 deletions(-)

diff --git a/src/mathed/InsetMath.h b/src/mathed/InsetMath.h
index bd72863..deb6915 100644
--- a/src/mathed/InsetMath.h
+++ b/src/mathed/InsetMath.h
@@ -34,7 +34,8 @@ enum HullType {
 	hullFlAlign,
 	hullMultline,
 	hullGather,
-	hullRegexp
+	hullRegexp,
+	hullUnknown
 };
 
 HullType hullType(docstring const & name);
diff --git a/src/mathed/InsetMathHull.cpp b/src/mathed/InsetMathHull.cpp
index 097a344..68318a2 100644
--- a/src/mathed/InsetMathHull.cpp
+++ b/src/mathed/InsetMathHull.cpp
@@ -79,16 +79,18 @@ namespace {
 	int getCols(HullType type)
 	{
 		switch (type) {
-			case hullEqnArray:
-return 3;
-			case hullAlign:
-			case hullFlAlign:
-			case hullAlignAt:
-			case hullXAlignAt:
-			case hullXXAlignAt:
-return 2;
-			default:
-return 1;
+		case hullEqnArray:
+			return 3;
+
+		case hullAlign:
+		case hullFlAlign:
+		case hullAlignAt:
+		case hullXAlignAt:
+		case hullXXAlignAt:
+			return 2;
+
+		default:
+			return 1;
 		}
 	}
 
@@ -128,28 +130,29 @@ HullType hullType(docstring const & s)
 	if (s == "flalign")   return hullFlAlign;
 	if (s == "regexp")return hullRegexp;
 	lyxerr << "unknown hull type '" << to_utf8(s) << "'" << endl;
-	return HullType(-1);
+	return hullUnknown;
 }
 
 
 docstring hullName(HullType type)
 {
 	switch (type) {
-		case hullNone:   return from_ascii("none");
-		case hullSimple: return from_ascii("simple");
-		case hullEquation:   return from_ascii("equation");
-		case hullEqnArray:   return from_ascii("eqnarray");
-		case hullAlign:  return from_ascii("align");
-		case hullAlignAt:return from_ascii("alignat");
-		case hullXAlignAt:   return from_ascii("xalignat");
-		case hullXXAlignAt:  return from_ascii("xxalignat");
-		case hullMultline:   return from_ascii("multline");
-		case hullGather: return from_ascii("gather");
-		case hullFlAlign:return from_ascii("flalign");
-		case hullRegexp: return from_ascii("regexp");
-		default:
-			lyxerr << "unknown hull type '" << type << "'" << endl;
-			return from_ascii("none");
+	case hullNone:   return from_ascii("none");
+	case hullSimple: return from_ascii("simple");
+	case hullEquation:   return from_ascii("equation");
+	case hullEqnArray:   return from_ascii("eqnarray");
+	case hullAlign:  return from_ascii("align");
+	case 

Re: [patch] Display the correct horizontal alignment in AMS environments

[Jean-Marc Lasgouttes]

Le 13/12/2015 04:37, Guillaume Munch a écrit :

Dear list

This has been annoying me for a while. OK to push?


It would deserve a commit log with explanations of what this fixes. The 
patch does many things it is difficult for me to say whether this is 
correct. Anyway this could also be something that we keep for 2.2.x.


FWIW, I also tend to be annoyed by bad on screen alignment, and the 
change of ordering of enums seems to be a good catch. However, it would 
be a good idea to get rid of the test on >= hullAlign and replace it 
with a helper function. This will make the code more robust IMO.


JMarc