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 at the same time. This one affects a main feature of lyx (all the
more now that align has become the default over eqnarray). (In fact this
issue popped up again no earlier than last month here:
<https://tex.stackexchange.com/questions/284665/how-to-align-the-lyx-aligned-math-environment>.)
I believe that all these bugs that developers "simply got used to"
should not be dismissed as secondary. Bugs that feature prominently
but never get solved (because developers "got used to it") not only
affect the learning curve, but also the credibility of lyx (for
instance, in the context of trying to convince co-authors to write
an article with it for the first time).
Guillaume
