Thank you for your detailed reply. I tested with Lyx Documentation and
as you said it crashed. I will solve that problem.
Regarding to whitespace and comments, yes I also understand value of
meaningful comments and proper formatted code as I was also stranger
to this code some times ago and those comments has helped me a lot to
understand code. So I will definitely use proper meaningful comments
in next patches.
It was my misunderstanding to divide polygon into two rectangles.
Actually I have to divide it into three rectangles to fill three boxes
generated in polygon.
But now I have another idea, that is to use QBrush to fill polygon
instead of dividing it into rectangles and in fact I got better
results with QBrush. So I will include it into next patch.
On 3/14/14, Richard Heck <rgh...@lyx.org> wrote:
> On 03/14/2014 12:02 PM, Mitul Modi wrote:
>> Sorry for that mistake,
>> here is the revised patch.
>
> No problem.
>
> The best way to test this kind of thing is with the LyX documentation
> that you will find under the Help menu. E.g., try it with the User Guide.
>
> If I do that, I get a crash. Oddly, if I don't run under the debugger, I
> get it right away. If I do, then I only get it when attempting to open
> and close, say, the first footnote. I think there is something wrong in
> the metrics calculation but don't have time to investigate right now.
>
> That said, this looks like an extremely promising start.
>
> A few other comments. Please have a look at the stylistic info under
> development/coding. We try not to be too pedantic about that sort of
> thing, but uniformity matters. So, for example:
>
>> diff --git a/src/frontends/Painter.h b/src/frontends/Painter.h
>> index d219b1b..7f2f4bd 100644
>> --- a/src/frontends/Painter.h
>> +++ b/src/frontends/Painter.h
>> @@ -92,6 +92,9 @@ public:
>> virtual void rectangle(int x, int y, int w, int h, Color,
>> line_style = line_solid, float line_width = thin_line) = 0;
>>
>> + /// draw Polygon for three box implementetion of insets
>> + virtual void polygon(int const * xp, int const * yp, int np, Color
>> col,line_style=line_solid,float linewidth=thin_line)=0;
>
> That should be broken neatly across two lines, and spacing should be
> uniform. So, e.g.:
>
>> + virtual void polygon(int const * xp, int const * yp, int np,
>> + Color col, line_style = line_solid,
>> + float linewidth = thin_line) = 0;
>
> Also, watch out for whitespace errors. I.e., trailing whitespace on
> various lines, or lines that have nothing but tabs on them. Most editors
> will help catch that.
>
> Admittedly, the LyX code is not commented as well as it could be, but we
> are trying to do better, and it really helps us read people's patches if
> they're well commented. Especially, e.g., here:
>
>> +void GuiPainter::polygon(int const * xp, int const * yp, int np, Color
>> col,line_style ls,float lw)
>> +{
>> + if (!isDrawingEnabled())
>> + return;
>> +
>> + // double the size if needed
>> + static QVector<QPoint> points(32);
>> + if (np > points.size())
>> + points.resize(2 * np);
>> +
>> + bool antialias = false;
>> + for (int i = 0; i < np; ++i) {
>> + points[i].setX(xp[i]);
>> + points[i].setY(yp[i]);
>> + if (i != 0)
>> + antialias |= xp[i-1] != xp[i] && yp[i-1] != yp[i];
>> + }
>
> What exactly is that test doing, and how is it working?
>
>> diff --git a/src/insets/InsetText.cpp b/src/insets/InsetText.cpp
>> index 5c0c113..1d57b66 100644
>> --- a/src/insets/InsetText.cpp
>> +++ b/src/insets/InsetText.cpp
>> @@ -211,8 +211,66 @@ void InsetText::metrics(MetricsInfo & mi, Dimension &
>> dim) const
>> void InsetText::draw(PainterInfo & pi, int x, int y) const
>> {
>> ...
>> + int x1[8],y1[8];
>>
>> if (drawFrame_ || pi.full_repaint) {
>> +
>> + //Calculate points of polygon of inset
>> + x1[0]=xframe-TEXT_TO_INSET_OFFSET; y1[0]=yframe-h0
>> +2*TEXT_TO_INSET_OFFSET;
>> + x1[1]=xframe-TEXT_TO_INSET_OFFSET;
>> y1[1]=yframe+2*TEXT_TO_INSET_OFFSET;
>> + x1[2]=xframe-TEXT_TO_INSET_OFFSET;
>> y1[2]=yframe+2*TEXT_TO_INSET_OFFSET;
>> + x1[3]=xframe-TEXT_TO_INSET_OFFSET;
>> y1[3]=yframe+h-h0+2*TEXT_TO_INSET_OFFSET;
>> + x1[4]=xframe+w2+2*TEXT_TO_INSET_OFFSET;
>> y1[4]=yframe+h-h0+2*TEXT_TO_INSET_OFFSET;
>> + x1[5]=xframe+w2+2*TEXT_TO_INSET_OFFSET;
>> y1[5]=yframe+h-2*h0+2*TEXT_TO_INSET_OFFSET;
>> + x1[6]=xframe+w1+2*TEXT_TO_INSET_OFFSET;
>> y1[6]=yframe+h-2*h0+2*TEXT_TO_INSET_OFFSET;
>> + x1[7]=xframe+w1+2*TEXT_TO_INSET_OFFSET;
>> y1[7]=yframe-h0+2*TEXT_TO_INSET_OFFSET;
>
> What are all those values, and how are they being calculated?
> (Stylistically, put the y's on their own lines.) It would help,
> presumably, if these had more descriptive names. Then you have to
> declare a lot more int's, and you will have to pass more arguments to
> polygon(), but it may pay off in terms of readability. Or maybe if it
> were well enough commented, that would be OK.
>
> I also find myself wondering what happens in the simple case of an inset
> that does fit on one line, maybe because it only contains a few characters.
>
>> + if (pi.full_repaint){
>> + //polygon is divided into two rectangles to fill color
>> + pi.pain.fillRectangle(xframe-TEXT_TO_INSET_OFFSET,
>> + yframe-h0+2*TEXT_TO_INSET_OFFSET,
>> + w1+2*TEXT_TO_INSET_OFFSET,
>> + h-h0,
>> + pi.backgroundColor(this));
>> + pi.pain.fillRectangle(xframe-TEXT_TO_INSET_OFFSET,
>> + yframe+2*TEXT_TO_INSET_OFFSET,
>> + w2+2*TEXT_TO_INSET_OFFSET,
>> + h-h0,
>> + pi.backgroundColor(this));
>> +
>> + }
>
> Here, too, I'm wondering exactly what these two rectangles are.
>
> Richard
>
>
--
Regards
Mitul Modi
B.E. Computer Engineering,
Birla Vishvakarma Mahavidhyalaya,
Vallabh Vidhyanagar - 388 120
India
