Angus Leeming wrote:
>
> This is looking good Rob. Any patch that has more lines starting with "-"
> thatn with "+" is probably doing the right thing ;-) In your case you have
> 1731 '-'s and 673 '+'s. And you reckon functionality is unchanged? Excellent.
Lots of stuff concerning size_kind/type has gone completely.
Also, the readFigInset has been removed totally, since lyx2lyx takes
care of that.
> I'm worried however by this
>
> +Backward compatibility from 1.2.0 to 1.3.0 is mostly alright.
> +Compatibility problems arrise when (lyx)size_kind is original_size
> +and also values for (lyx)scale and/or (lyx)widht/height are given.
I may think of this a little more, but I believe some backward compatibility
loss is unavoidable, with all these old keywords disappearing. But keep in
mind: only odd input will break, e.g. choose "Original size", but also have
a value there for Scale; The Scale value will then be taken, instead of
scale=100%. I can't see a solution for this, since the scale comes AFTER
the Original_size keyword in the previous Graphics inset.
Could lyx2lyx help out here?
> + fl_addto_choice(file_->choice_width, + (_("scale%%|") +
> choice_Length_All).c_str());
>
> What's that initial '+' doing ?
Nothing; typo. Sorry. I'll fix that with when I submit patch next time.
> + fl_set_input_filter(file_->input_lyxscale, fl_unsigned_int_filter);
> ...
> + igp.lyxscale = strToInt(getString(file_->input_lyxscale));
>
> This seems inconsistent. If you allow the user to input a float, you should
> store a float.
Both, lyxscale and scale are integers. lyxscale's input field is set to
integer input only by
fl_set_input_filter(file_->input_lyxscale, fl_unsigned_int_filter);
Indeed, the width input allowed always a float, but I will change that in next
patch: when scale% unit is selected for width, allow only integer input, otherwise
float input. That solved the problem, I guess.
>
> + igp.scale = -1;
> + igp.width = getLyXLengthFromWidgets(file_->input_width,
> + file_->choice_width);
>
> I believe that Herbert wanted to store both (althout the reasoning escapes me
> for now). You could achieve this by saving igp.scale = -igp.scale;
There's no use of that in the present dialog. There's only one input field,
that stores either width (in any unit) or scale (in scale% unit). There is no
place for both, so storing them both would be confusing and unnecessary.
> + bool const enable = fl_get_choice(file_->choice_width) != 1 &&
> + !trim(getString(file_->input_width)).empty() &&
> + !trim(getString(file_->input_height)).empty();
>
> xforms_helpers.C's getString already trim()s its output.
Thanks, removed the trims.
>
>>Do we need to push up the Format Id for the graphics inset with this patch?
>
>
> I think that we can get rid of the Format Id, since the information is
> inherent in lyxformat. I believe that Baruch created it as a temporary
> variable when he was first creating the InsetGraphics.
Shall I remove that too, then. But before I do that, we need to be really sure
that the Format output (per graphics inset) can go. I don't know anything
about this.
>>A remote issue: insetgraphicsParams.C uses "grfx::Params pars" in
>>grfx::Params InsetGraphicsParams::as_grfxParams().
>>grfx::Params contains fields that do not exist anymore: width, height and
>>keepLyXAspectRatio. These are all replaced by LyX scale, and aspect ratio
>>is always kept. So these three fields in grfx::Params could go, but I
>>haven't done anything in this area.
>
> Do so. Let's have a self consistent patch.
Okay. Will send a more "self consistent patch" as soon as I have implemented what
I mentioned above.
Could you please answer a few of the questions above to help me further.
Thanks.
Rob.
