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
