Dear list,
We need to reach a conclusion on this to open the way for beta. It seems
we have come a long way on this series of patches so it would be nice to
know whether people who have participated in the discussion and asked
for modifications let us know whether they intend to let me go ahead on
this or not (hoping a move to behind Gare de Lyon went smoothly).
For the two aspects raised by Jean-Marc, I have found that it comes down
to the assumption that after the patch, "inset-modify tabular" is only
meant for tabulars and not math grids. Indeed, the only way prefs2prefs
leaves a "inset-modify tabular" command behind is if before the patch it
started with "from_dialog", and therefore did not work with math grids
before. All other "inset-modify tabular" commands are issued by the
tabular dialog and meant for tabulars as well.
We have taken our time for this series of patches (including the
Christmas disruption) and it has gone through several iterations
including implementing new conversion policies, and also I have been
testing it in master since the beginning of this month without seeing
any particular issue (though I know the interest of having tests made by
other people than me). So I feel pretty confident about it.
Let's go ahead with solving #9794 and releasing beta.
Guillaume
Le 11/01/2016 12:54, Guillaume Munch a écrit :
Le 11/01/2016 14:23, Jean-Marc Lasgouttes a écrit :
Le 08/01/2016 23:22, Guillaume Munch a écrit :
Thank you, waiting for Jean-Marc's opinion then.
A few comments on the patches. I was not able to apply them using "git
am" I am not sure how they were produced.
Strange. I rebased in the attached. If there are still issues please
give me the error message.
0001:
* fix the log title (.sh, not .py)
0002:
* be careful to update the commit hash when you actually commit.
* Actually, I am not sure what this commit does here.
This is something I had to do in an earlier patch but did not know,
wanted to get confirmation. But I did not want to open a new thread just
for that.
0003:
* I am not sure what the 'from-dialog' parameter does in the
GuiTabular dialog, so I am not really able to comment on this part.
It was used to bypass the status check of the command, because the
latter only checked the first command and therefore was incorrect for
what was coming from the dialog. I.e.:
* no keyword: broken status check
* from-dialog keyword: no status check (dispatched by the dialog)
Since we no longer use inset-modify for the interface, it makes more
sense to exchange the general and the special case, so that we now have:
* no keyword: no status check
* for-dialog: status check a single command (only passed internally by
the dialog to getStatus)
* I'd prefer to rename getActionStatus to getFeatureStatus (or
something with feature in the name). Action is something else (in
frontend).
* The parts that remove INSET_MODIFY handling in maths look OK at
first sight, but I cannot vouch for their general innocuousness.
UI commands are now tabular-feature. If Math receives inset-modify
tabular, then it comes from the tabular dialog, and therefore is not
meant for the math inset but the enclosing tabular. You can see that the
tabular dialog is a buggy if the cursor is located in math, and if you
click ok t says "unknown feature from-dialog".
Removing the handling of inset-modify tabular implies that inset-modify
is now dispatched by InsetMathNest, which disables it entirely (two
exceptions: InsetMathSpace and InsetMathRef). This corresponds to
current behaviour.
(I could go further and remove INSET_MODIFY from InsetMathNest. Then,
any inset-modify command would be dispatched to the enclosing text
inset. Maybe I'll do that after 2.2.)
InsetMathCases::doDispatch(): I kept the existing behaviour but do not
understand it fully. What is the effect of setting cur.undispatched()
before dispatching to the parent class? Is the setting not overridden by
the parent class?
0004: not my area of expertise
Then I rely on Georg for this one.
000[5678]: OK
0009:
* maybe avoid the "default:" clause in InsetMathHull::isTable() and
use all the enum values.
Ok.
All in all, I like the patch, but I cannot check that it does everything
correctly.
Attached takes all comments into account. I hope the additional
explanations help.
Guillaume
