Re: [patch] Solving selection painting bugs
On Saturday 25 October 2008 10:51:05 Pavel Sanda wrote: > yes, i've tested it. > pavel OK. -- José Abílio
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote: >> I am not against that but I would like to hear other opinions from >> developers >> working on this area. >> > I agree with Pavel. The patch looks good at first glance. No much time to > do anything more. If Pavel tested it I think it should go in. yes, i've tested it. pavel
Re: [patch] Solving selection painting bugs
On 25/10/2008 10:55, José Matos wrote: On Friday 24 October 2008 18:05:21 Pavel Sanda wrote: imho better to put it in before 1.6.0. I am not against that but I would like to hear other opinions from developers working on this area. I agree with Pavel. The patch looks good at first glance. No much time to do anything more. If Pavel tested it I think it should go in. Abdel.
Re: [patch] Solving selection painting bugs
On Friday 24 October 2008 18:05:21 Pavel Sanda wrote: > imho better to put it in before 1.6.0. I am not against that but I would like to hear other opinions from developers working on this area. > pavel -- José Abílio
Re: [patch] Solving selection painting bugs
José Matos wrote: > On Thursday 23 October 2008 20:20:13 Vincent van Ravesteijn wrote: > > Jose, I hope I'm not too late for 1.6 :S... Otherwise I'll have to make > > it smell like a major bug fix.. > > For rc4 certainly it is late. :-) > > For it to be applied to 1.6.0 I would like to hear from others. In the worst imho better to put it in before 1.6.0. pavel
Re: [patch] Solving selection painting bugs
On Thursday 23 October 2008 20:20:13 Vincent van Ravesteijn wrote: > Jose, I hope I'm not too late for 1.6 :S... Otherwise I'll have to make > it smell like a major bug fix.. For rc4 certainly it is late. :-) For it to be applied to 1.6.0 I would like to hear from others. In the worst case scenario this will certainly be considered for 1.6.1 not that I am advocating but just considering all scenarios. I don't want to discourage you by delaying last minute changes but experience has shown me the hard way how these changes can be disruptive even if the patch is (apparently) harmless. Please keep up the good work. :-) > Vincent -- José Abílio
Re: [patch] Solving selection painting bugs
New patch.
Summary:
A PainterInfo::backgroundColor function is introduced that determines
the background color of a certain Inset. This color is depended on the
selection state, the color of its parent and its own color. The painting
is now also correct for the special cases like Math and Tabular.
Major changes since last patch:
- I moved the Inset::backgroundColor(PainterInfo & pain, int x, int y)
function to the PainterInfo-class. (If you really want, I can also
supply a patch with the old Inset::backgroundColor function).
(MetricsInfo)
- Solved the problem wrt background painting of Math insets. Now their
background is retained during editing. (InsetMathHull/RowPainter)
- Tabular Cell Insets are now correctly drawn. (InsetTabular)
Good luck with analyzing this patch.. I'll be around for any questions.
Jose, I hope I'm not too late for 1.6 :S... Otherwise I'll have to make
it smell like a major bug fix..
Vincent
Index: src/insets/InsetTabular.h
===
--- src/insets/InsetTabular.h (revision 26996)
+++ src/insets/InsetTabular.h (working copy)
@@ -834,6 +834,8 @@
/// descending into the insets
docstring asString(idx_type stidx, idx_type enidx, bool intoInsets = true);
+ /// Returns whether the cell in the specified row and column is selected.
+ bool isCellSelected(Cursor & cur, row_type row, col_type col) const;
//
// Public structures and variables
///
Index: src/insets/InsetLayout.cpp
===
--- src/insets/InsetLayout.cpp (revision 26996)
+++ src/insets/InsetLayout.cpp (working copy)
@@ -119,7 +119,7 @@
FontInfo font = inherit_font;
labelfont_ = inherit_font;
- bgcolor_ = Color_background;
+ bgcolor_ = Color_none;
bool getout = false;
// whether we've read the CustomPars or ForcePlain tag
// for issuing a warning in case MultiPars comes later
Index: src/insets/Inset.cpp
===
--- src/insets/Inset.cpp (revision 26996)
+++ src/insets/Inset.cpp (working copy)
@@ -430,7 +430,7 @@
ColorCode Inset::backgroundColor() const
{
- return Color_background;
+ return Color_none;
}
Index: src/insets/InsetTabular.cpp
===
--- src/insets/InsetTabular.cpp (revision 26996)
+++ src/insets/InsetTabular.cpp (working copy)
@@ -2996,11 +2996,37 @@
dim.wid = tabular.width() + 2 * ADD_TO_TABULAR_WIDTH;
}
+bool InsetTabular::isCellSelected(Cursor & cur, row_type row, col_type col)
+ const
+{
+ if (&cur.inset() == this && cur.selection()) {
+ if (cur.selIsMultiCell()) {
+ row_type rs, re;
+ col_type cs, ce;
+ getSelection(cur, rs, re, cs, ce);
+
+ if (col >= cs && col <= ce && row >= rs && row <= re)
+return true;
+ } else
+ if (col == tabular.cellColumn(cur.idx())
+&& row == tabular.cellRow(cur.idx())) {
+ CursorSlice const & beg = cur.selBegin();
+ CursorSlice const & end = cur.selEnd();
+ if (end.lastpos() > 0 && end.pos() == end.lastpos()
+ && beg.pos() == 0)
+return true;
+ }
+ }
+ return false;
+}
+
+
void InsetTabular::draw(PainterInfo & pi, int x, int y) const
{
//lyxerr << "InsetTabular::draw: " << x << " " << y << endl;
BufferView * bv = pi.base.bv;
+ Cursor & cur = pi.base.bv->cursor();
// FIXME: As the full backrgound is painted in drawSelection(),
// we have no choice but to do a full repaint for the Text cells.
@@ -3012,6 +3038,7 @@
x += ADD_TO_TABULAR_WIDTH;
bool const original_drawing_state = pi.pain.isDrawingEnabled();
+ bool const original_selection_state = pi.selected;
idx_type idx = 0;
first_visible_cell = Tabular::npos;
@@ -3026,6 +3053,7 @@
if (first_visible_cell == Tabular::npos)
first_visible_cell = idx;
+ pi.selected |= isCellSelected(cur, i, j);
int const cx = nx + tabular.getBeginningOfTextInCell(idx);
// Cache the Inset position.
bv->coordCache().insets().add(cell(idx).get(), cx, y);
@@ -3043,6 +3071,7 @@
}
nx += tabular.columnWidth(idx);
++idx;
+ pi.selected = original_selection_state;
}
if (i + 1 < tabular.row_info.size())
@@ -3065,7 +3094,7 @@
int const w = tabular.width();
int const h = tabular.height();
int yy = y - tabular.rowAscent(0);
- pi.pain.fillRectangle(x, yy, w, h, backgroundColor());
+ pi.pain.fillRectangle(x, yy, w, h, pi.backgroundColor(this));
if (!cur.selection())
return;
@@ -3076,9 +3105,6 @@
if (cur.selIsMultiCell()) {
- row_type rs, re;
- col_type cs, ce;
- getSelection(cur, rs, re, cs, ce);
y -= tabular.rowAscent(0);
for (row_type j = 0; j < tabular.row_info.size(); ++j) {
int const a = tabular.rowAscent(j);
@@ -3091,7 +3117,7 @@
idx_type const cell =
tabular.cellIndex(j, i);
int const w = tabular.columnWidth(cell);
-if (i >= cs && i <= ce && j >= rs && j <= re)
+if (isCellSelected(cur, j, i))
pi.pain.fillRectangle
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: > Thus, if the selection is outside the current Inset, we don't touch > pi_.selected_. OK. JMarc
Re: [patch] Solving selection painting bugs
On Tue, Sep 23, 2008 at 02:08:48PM +0200, Vincent van Ravesteijn - TNW wrote: > Jmarc wrote: > > First thing I have to say is that I like this patch. > > +1 ;-) > > > - math insets previews will keep their background, even when no > > specific background was really wanted. What is weird is that they > > become transparent when they are edited or selected. > > This is hardcoded and commented in the code, so this probably has a > valid reason, which I don't understand, to be honest. I'll try what > happens when I remove this (something I learned from the list a while > ago). > > Src/mathed/InsetMathHull.cpp: > 388 // background of mathed under focus is not painted because > 389 // selection at the top level of nested inset is difficult to > handle. > 390 if (!editing(pi.base.bv)) > 391pi.pain.fillRectangle(.., Color_mathbg); Could be an old "solution" I would not be surprised if removing it would help. Andre'
RE: [patch] Solving selection painting bugs
>>> - when selecting several cells of a tabular inset, the contents of >>> the cells is not painted in blue. >> >> This is one level deeper into Tabulars, I'll have a look into that. > >Not necessarily, it might just be this one (where the two sides of >the cursor may point to a different cells): > >+ if (cur.text() == &text_ && cur.anchor().text() == &text_) >+ pi_.selected_ = row_.sel_beg <= pos && row_.sel_end > pos; > >What is this test intended to do? > >JMarc > It is the counterpart of the test in [EMAIL PROTECTED]: 1984 bool selection = cur.selection() 1985 // This is our text. 1986&& cur.text() == text_ 1987// if the anchor is outside, this is not our selection 1988&& cur.anchor().text() == text_ 1989&& pit >= sel_beg.pit() && pit <= sel_end.pit(); The consequence of this piece of code is that if we are in an Inset, row_.sel_beg and row_.sel_end are both -1 even when the Inset as a whole is selected. Then if we want to draw a nested Inset, we do not want to evaluate pi_.selected_ (it'll be always false) but we want to inherit the selection status as it is. Thus, if the selection is outside the current Inset, we don't touch pi_.selected_. Vincent
Re: [patch] Solving selection painting bugs
Jean-Marc Lasgouttes wrote: "Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: I treated it the same as Color_none and thus it inherits the color of its 'parent'. Just because I don't want to bother the innocent user with it. I think the goal _was_ to bother the innocent user with it (this will probably output bad latex too!). The LaTeX will be OK, because unknown insets just get output as text. But yes, the user is supposed to be bothered, because an unknown layout is an error. rh
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: >> - when selecting several cells of a tabular inset, the contents >> of the cells is not painted in blue. > > This is one level deeper into Tabulars, I'll have a look into that. Not necessarily, it might just be this one (where the two sides of the cursor may point to a different cells): + if (cur.text() == &text_ && cur.anchor().text() == &text_) + pi_.selected_ = row_.sel_beg <= pos && row_.sel_end > pos; What is this test intended to do? JMarc
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: > I treated it the same as Color_none and thus it inherits the color of > its 'parent'. Just because I don't want to bother the innocent user with > it. I think the goal _was_ to bother the innocent user with it (this will probably output bad latex too!). JMarc
RE: [patch] Solving selection painting bugs
>"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: >> That's because InsetLayout is initialized with Color_error. >> >> src/insets/InsetLayout.cpp: >> 36bgcolor_(Color_error), >> >> It's a safety measure for drawing an Inset for which no Layout file is >> read or, more specifically, for an Inset for which InsetLayout::read >> isn't called. I don't know whether this can happen though. > >But in this case we do want to display in Color_error color IMO. > I treated it the same as Color_none and thus it inherits the color of its 'parent'. Just because I don't want to bother the innocent user with it. > JMarc Vincent
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: > That's because InsetLayout is initialized with Color_error. > > src/insets/InsetLayout.cpp: > 36bgcolor_(Color_error), > > It's a safety measure for drawing an Inset for which no Layout file is > read or, more specifically, for an Inset for which InsetLayout::read > isn't called. I don't know whether this can happen though. But in this case we do want to display in Color_error color IMO. JMarc
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: Jmarc wrote: + if (color_bg != Color_error && color_bg != Color_none) What is this special treatment of Color_error for? That's because InsetLayout is initialized with Color_error. src/insets/InsetLayout.cpp: 36bgcolor_(Color_error), It's a safety measure for drawing an Inset for which no Layout file is read or, more specifically, for an Inset for which InsetLayout::read isn't called. I don't know whether this can happen though. Yes, it can happen. If you change document classes or modules, for example, you might lose the layout for an inset. Richard
RE: [patch] Solving selection painting bugs
Jmarc wrote: > First thing I have to say is that I like this patch. +1 ;-) > - math insets previews will keep their background, even when no > specific background was really wanted. What is weird is that they > become transparent when they are edited or selected. This is hardcoded and commented in the code, so this probably has a valid reason, which I don't understand, to be honest. I'll try what happens when I remove this (something I learned from the list a while ago). Src/mathed/InsetMathHull.cpp: 388 // background of mathed under focus is not painted because 389 // selection at the top level of nested inset is difficult to handle. 390 if (!editing(pi.base.bv)) 391pi.pain.fillRectangle(.., Color_mathbg); > - when selecting several cells of a tabular inset, the contents > of the cells is not painted in blue. This is one level deeper into Tabulars, I'll have a look into that. > > + if (color_bg != Color_error && color_bg != Color_none) > > What is this special treatment of Color_error for? That's because InsetLayout is initialized with Color_error. src/insets/InsetLayout.cpp: 36bgcolor_(Color_error), It's a safety measure for drawing an Inset for which no Layout file is read or, more specifically, for an Inset for which InsetLayout::read isn't called. I don't know whether this can happen though. Vincent
Re: [patch] Solving selection painting bugs
First thing I have to say is that I like this patch. Vincent van Ravesteijn <[EMAIL PROTECTED]> writes: > I don't know which other problems you exactly meant, but the following > will solve some : > > - Insets without an own color inherit the color from the containing > Inset (e.g. footnotes, tables, captions), > - Selected Insets will be painted with Color_selection incl. math Insets, That seems to cover what I had in mind. I'll comment on this patch and not on the next one, because it is a sane first step. We can discuss later whether we want to do color blending and how (but I am not sure this is 1.6 scope). > Each Inset has either an own custom color or it has the Color_none > (clear as in ColorCode.h, but we might change this to Color_inherit). > If there is no color specified in its layout or it isn't an > InsetCollapsable, it will have the Color_none as default (changed in > InsetLayout.cpp & Inset.cpp). Good. The only two remaining problems I saw are - math insets previews will keep their background, even when no specific background was really wanted. What is weird is that they become transparent when they are edited or selected. - when selecting several cells of a tabular inset, the contents of the cells is not painted in blue. > Then, given a certain PainterInfo with the selection status and the > color of the containing Inset (or of anyone 'above' if the containing > Inset has Color_none), the method Inset::realizeBackgroundColor > (Inset.cpp) will realize the background color. I'd prefer to keep the backgroundColor(PainterInfo) name, since the 'realize' prefix (used in fonts), tend to imply that we modify the underlying object. Actually, since the method is not intended to become virtual, we could have a method PainterIndo::realizeBackgroundColor(Inset const &) that changes the value of PainterInfo::background_color. This is especially true since the methods uses the variable selected_, which ought to be private. Some additional comments below. JMarc > + bool pi_selected = pi_.selected_; > + Cursor & cur = pi_.base.bv->cursor(); constify. > + bool pi_selected = pi_.selected_; > + Cursor & cur = pi_.base.bv->cursor(); Here too. > /// > + bool selected_; Document the variable. > + if (color_bg != Color_error && color_bg != Color_none) What is this special treatment of Color_error for?
Re: [patch] Solving selection painting bugs
Jean-Marc Lasgouttes wrote:
You could even make a new backgroundColor(PainterInfo const &) method,
that would also allow you to pass the color of the containing inset
(case of a footnote in a yellow LyX note) and thus fix other problems...
JMarc
I don't know which other problems you exactly meant, but the following
will solve some :
- Insets without an own color inherit the color from the containing
Inset (e.g. footnotes, tables, captions),
- Selected Insets will be painted with Color_selection incl. math Insets,
Each Inset has either an own custom color or it has the Color_none
(clear as in ColorCode.h, but we might change this to Color_inherit). If
there is no color specified in its layout or it isn't an
InsetCollapsable, it will have the Color_none as default (changed in
InsetLayout.cpp & Inset.cpp).
In case of nested Insets, the color of the Inset is then passed with
PainterInfo to any child insets. This is now done in InsetText in stead
of InsetCollapsable.
Then, given a certain PainterInfo with the selection status and the
color of the containing Inset (or of anyone 'above' if the containing
Inset has Color_none), the method Inset::realizeBackgroundColor
(Inset.cpp) will realize the background color.
Vincent
Index: src/insets/InsetCollapsable.cpp
===
--- src/insets/InsetCollapsable.cpp (revision 26450)
+++ src/insets/InsetCollapsable.cpp (working copy)
@@ -264,8 +264,6 @@
LASSERT(layout_, /**/);
autoOpen_ = pi.base.bv->cursor().isInside(this);
- ColorCode const old_color = pi.background_color;
- pi.background_color = backgroundColor();
FontInfo tmpfont = pi.base.font;
pi.base.font = layout_->font();
@@ -375,7 +373,6 @@
}
break;
}
- pi.background_color = old_color;
pi.base.font = tmpfont;
}
Index: src/insets/InsetText.cpp
===
--- src/insets/InsetText.cpp(revision 26450)
+++ src/insets/InsetText.cpp(working copy)
@@ -199,11 +199,18 @@
int const h = tm.height() + 2 * TEXT_TO_INSET_OFFSET;
int const xframe = x + TEXT_TO_INSET_OFFSET / 2;
if (pi.full_repaint)
- pi.pain.fillRectangle(xframe, yframe, w, h,
backgroundColor());
+ pi.pain.fillRectangle(xframe, yframe, w, h,
+ realizeBackgroundColor(pi));
+
if (drawFrame_)
pi.pain.rectangle(xframe, yframe, w, h, frameColor());
}
+ ColorCode const old_color = pi.background_color;
+ pi.background_color = realizeBackgroundColor(pi, false);
+
tm.draw(pi, x + TEXT_TO_INSET_OFFSET, y);
+
+ pi.background_color = old_color;
}
Index: src/rowpainter.cpp
===
--- src/rowpainter.cpp (revision 26450)
+++ src/rowpainter.cpp (working copy)
@@ -665,7 +665,13 @@
if (x_ > pi_.base.bv->workWidth())
continue;
x_ = pi_.base.bv->coordCache().getInsets().x(inset);
+
+ bool pi_selected = pi_.selected_;
+ Cursor & cur = pi_.base.bv->cursor();
+ if (cur.text() == &text_ && cur.anchor().text() == &text_)
+ pi_.selected_ = row_.sel_beg <= pos && row_.sel_end >
pos;
paintInset(inset, pos);
+ pi_.selected_ = pi_selected;
}
}
@@ -794,7 +800,13 @@
} else if (inset) {
// If outer row has changed, nested insets are repaint
completely.
pi_.base.bv->coordCache().insets().add(inset, int(x_),
yo_);
+
+ bool pi_selected = pi_.selected_;
+ Cursor & cur = pi_.base.bv->cursor();
+ if (cur.text() == &text_ && cur.anchor().text() ==
&text_)
+ pi_.selected_ = row_.sel_beg <= pos &&
row_.sel_end > pos;
paintInset(inset, pos);
+ pi_.selected_ = pi_selected;
++vpos;
} else {
Index: src/MetricsInfo.h
===
--- src/MetricsInfo.h (revision 26450)
+++ src/MetricsInfo.h (working copy)
@@ -105,6 +105,8 @@
/// Whether the parent is deleted (change tracking)
bool erased_;
///
+ bool selected_;
+ ///
bool full_repaint;
///
ColorCode background_color;
Index: src/insets/InsetLayout.cpp
===
--- src/insets/InsetLayout.cpp (revision 26450)
+++ src/insets/InsetLayout.cpp (working copy)
@@ -110,7 +110,7 @@
FontInfo font = inherit_font;
labelfont_
Re: [patch] Solving selection painting bugs
On Fri, Sep 19, 2008 at 09:53:40AM +0200, Abdelrazak Younes wrote: > Vincent van Ravesteijn wrote: >> Abdelrazak Younes wrote: >>> >>> We also need to fix the selection painting within Inset. In your >>> example, the inset should have a blue background, regardless off end of >>> paragraph. That's also to be fixed for mathed. I mean, if you're looking >>> for something else to fix ;-) >>> >>> Abdel. >>> >> >> Tu veux tester ? > > I really think we should switch to French on this list :-) Mais vous n'y pensez pas, mon ami... Andre', moving the bookmark of like http://dict.leo.org/frde more to the top...
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote: Abdelrazak Younes wrote: We also need to fix the selection painting within Inset. In your example, the inset should have a blue background, regardless off end of paragraph. That's also to be fixed for mathed. I mean, if you're looking for something else to fix ;-) Abdel. Tu veux tester ? I really think we should switch to French on this list :-) Wanna give it a try ? Looks good. Abdel.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn <[EMAIL PROTECTED]> writes: > - pi.background_color = backgroundColor(); > + if (pi.selected_) > + pi.background_color = Color_selection; > + else > + pi.background_color = backgroundColor(); You could even make a new backgroundColor(PainterInfo const &) method, that would also allow you to pass the color of the containing inset (case of a footnote in a yellow LyX note) and thus fix other problems... JMarc
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote:
Vincent van Ravesteijn wrote:
>
> Abdelrazak Younes wrote:
> >
> > We also need to fix the selection painting within Inset. In your
> > example, the inset should have a blue background, regardless off
end of
> > paragraph. That's also to be fixed for mathed. I mean, if you're
looking
> > for something else to fix ;-)
> >
> > Abdel.
> >
>
> Tu veux tester ?
>
> Wanna give it a try ?
>
> Vincent
>
It can even (much) simpler and better... it now also works for nested
insets.
(sorry for the spam)
Vincent
And this one does the trick for math... bed time.
Vincent
Index: src/mathed/InsetMathHull.cpp
===
--- src/mathed/InsetMathHull.cpp(revision 26450)
+++ src/mathed/InsetMathHull.cpp(working copy)
@@ -387,9 +387,10 @@
// background of mathed under focus is not painted because
// selection at the top level of nested inset is difficult to handle.
+ ColorCode color_bg = pi.selected_ ? Color_selection : Color_mathbg;
if (!editing(pi.base.bv))
pi.pain.fillRectangle(x + 1, y - dim.asc + 1, dim.wid - 2,
- dim.asc + dim.des - 1, Color_mathbg);
+ dim.asc + dim.des - 1, color_bg);
if (use_preview_) {
// one pixel gap in front
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote:
Abdelrazak Younes wrote:
>
> We also need to fix the selection painting within Inset. In your
> example, the inset should have a blue background, regardless off end of
> paragraph. That's also to be fixed for mathed. I mean, if you're looking
> for something else to fix ;-)
>
> Abdel.
>
Tu veux tester ?
Wanna give it a try ?
Vincent
It can even (much) simpler and better... it now also works for nested
insets.
(sorry for the spam)
Vincent
Index: src/insets/InsetCollapsable.cpp
===
--- src/insets/InsetCollapsable.cpp (revision 26450)
+++ src/insets/InsetCollapsable.cpp (working copy)
@@ -265,7 +265,10 @@
autoOpen_ = pi.base.bv->cursor().isInside(this);
ColorCode const old_color = pi.background_color;
- pi.background_color = backgroundColor();
+ if (pi.selected_)
+ pi.background_color = Color_selection;
+ else
+ pi.background_color = backgroundColor();
FontInfo tmpfont = pi.base.font;
pi.base.font = layout_->font();
Index: src/insets/InsetText.cpp
===
--- src/insets/InsetText.cpp(revision 26450)
+++ src/insets/InsetText.cpp(working copy)
@@ -198,8 +198,12 @@
int const yframe = y - TEXT_TO_INSET_OFFSET - tm.ascent();
int const h = tm.height() + 2 * TEXT_TO_INSET_OFFSET;
int const xframe = x + TEXT_TO_INSET_OFFSET / 2;
- if (pi.full_repaint)
- pi.pain.fillRectangle(xframe, yframe, w, h,
backgroundColor());
+ if (pi.full_repaint) {
+ if (pi.selected_)
+ pi.pain.fillRectangle(xframe, yframe, w, h,
Color_selection);
+ else
+ pi.pain.fillRectangle(xframe, yframe, w, h,
backgroundColor());
+ }
if (drawFrame_)
pi.pain.rectangle(xframe, yframe, w, h, frameColor());
}
Index: src/rowpainter.cpp
===
--- src/rowpainter.cpp (revision 26450)
+++ src/rowpainter.cpp (working copy)
@@ -665,7 +665,13 @@
if (x_ > pi_.base.bv->workWidth())
continue;
x_ = pi_.base.bv->coordCache().getInsets().x(inset);
+
+ bool pi_selected = pi_.selected_;
+ Cursor & cur = pi_.base.bv->cursor();
+ if (cur.text() == &text_ && cur.anchor().text() == &text_)
+ pi_.selected_ = row_.sel_beg <= pos && row_.sel_end >
pos;
paintInset(inset, pos);
+ pi_.selected_ = pi_selected;
}
}
@@ -794,7 +800,13 @@
} else if (inset) {
// If outer row has changed, nested insets are repaint
completely.
pi_.base.bv->coordCache().insets().add(inset, int(x_),
yo_);
+
+ bool pi_selected = pi_.selected_;
+ Cursor & cur = pi_.base.bv->cursor();
+ if (cur.text() == &text_ && cur.anchor().text() ==
&text_)
+ pi_.selected_ = row_.sel_beg <= pos &&
row_.sel_end > pos;
paintInset(inset, pos);
+ pi_.selected_ = pi_selected;
++vpos;
} else {
Index: src/MetricsInfo.h
===
--- src/MetricsInfo.h (revision 26450)
+++ src/MetricsInfo.h (working copy)
@@ -105,6 +105,8 @@
/// Whether the parent is deleted (change tracking)
bool erased_;
///
+ bool selected_;
+ ///
bool full_repaint;
///
ColorCode background_color;
RE: [patch] Solving selection painting bugs
>> Well, that's a pity. Look at Captions, Ragged-Line-Breaks, >> LyxMenuSeparators (in the User's guide). They are all painted crappy >> (when moving your mouse around them), because they have no color >> specified and they do not inherit their parent's color. This means >> that the area cannot be cleared prior to redrawing the Inset. > >Can't we convey this information through PainterInfo? > >JMarc > Oui, How stupid, just didn't look for it in PainterInfo, although I must have seen that it is set in InsetCollapsable. Now, the search for all Insets that suffer from this problem. I also see that there is information passed about whether the parent is deleted (this information isn't always used either if the Inset is open). Then, we might add a selection member to it to pass information about whether we have to paint in a selected state... solving the other problem. Well, that's for tomorrow. Vincent
Re: [patch] Solving selection painting bugs
> Well, that's a pity. Look at Captions, Ragged-Line-Breaks, > LyxMenuSeparators (in the User's guide). They are all painted crappy > (when moving your mouse around them), because they have no color > specified and they do not inherit their parent's color. This means > that > the area cannot be cleared prior to redrawing the Inset. Can't we convey this information through PainterInfo? JMarc
RE: [patch] Solving selection painting bugs
>>> We also need to fix the selection painting within Inset. In your >>> example, the inset should have a blue background, regardless off end >>> of paragraph. That's also to be fixed for mathed. >>> >> >> We really need transparent colors then. Then compute a selection color >> for each Inset based on their background color and the selection color. >> > > That could be difficult. I'd say it's OK if we loose the initial > background color... at least for now... Or just one color, different from the main selection color, for all selected insets, then the screen doesn't look intimidating blue. >>> I mean, if you're looking for something else to fix ;-) >>> >> >> I don't want to sound rude, but I will probably not soon be 'looking' >> for something to fix... ;-) >> > >Hum, not sure I understand... I mean, while using LyX for the last three years or so, I wrote down things that I would like to fix, and the list is still growing :( .. >> PS. Do you know that at every mouse click, how innocent it might be, >> there is a full metrics update ? If yes, can you tell me why this is. >> It looks a bit a waste of CPU cycles... (i.e. all checksums we are >> calculating disappear). >> >Should not be. I made sure that was not the case a number of times. >Maybe not true anymore... and I of course agree with you here. > I lost track in figuring out where exactly the update is set. Vincent
RE: [patch] Solving selection painting bugs
>>> We really need transparent colors then. Then compute a selection >>> color for each Inset based on their background color and the >>> selection color. >> That could be difficult. I'd say it's OK if we loose the initial >> background color... at least for now... > >There was a time where insets could have a backgroundColor() property, >which could be 'none', in which case they would inherit the parent's >color. It did >not fit someone's vision of parent insets, so it got >nuked. Well, that's a pity. Look at Captions, Ragged-Line-Breaks, LyxMenuSeparators (in the User's guide). They are all painted crappy (when moving your mouse around them), because they have no color specified and they do not inherit their parent's color. This means that the area cannot be cleared prior to redrawing the Inset. So I couldn't find a simple way to fix the painting, other than assuming that their background is the same as the main buffer's background (which is not always the case of course). Suggestions ? > JMarc Vincent
Re: [patch] Solving selection painting bugs
We really need transparent colors then. Then compute a selection color for each Inset based on their background color and the selection color. That could be difficult. I'd say it's OK if we loose the initial background color... at least for now... There was a time where insets could have a backgroundColor() property, which could be 'none', in which case they would inherit the parent's color. It did not fit someone's vision of parent insets, so it got nuked. JMarc
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: Anyone thinks this has potential (see attachment) ? I do :-) In that case, I added an extra boolean to the Row class saying whether there is an end of paragraph end or a normal end of line. Now, we can experiment with different kinds of painting. Maybe the Apple HIG says something about it ;-) I can send you a link if you want to look at it. :-) /Konrad
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: We also need to fix the selection painting within Inset. and *of* insets themselves. f.e. if you change the color of selected text to white in preferences you will see that say the numbers of sections stay black (whereas we would then like them white). it would be nice if this type of stuff is fixed indeed. last time i tried to understand this type of stuff it seemed that insets are ignorant of their selection state. Yes, that is. Right. i remember starting to passing around a selection parameter, but i had to touch *many* files. this was too daunting for me so i aborted the operation... Same overhere, had a look at it, and stranded... But it doesn't seem that important for the near future, unless I hear objections... then... No, you're right, not that important, that was a sticky remarck in my head, forget about it. Abdel.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: Anyone thinks this has potential (see attachment) ? I do :-) In that case, I added an extra boolean to the Row class saying whether there is an end of paragraph end or a normal end of line. Now, we can experiment with different kinds of painting. Maybe the Apple HIG says something about it ;-) OK. We also need to fix the selection painting within Inset. In your example, the inset should have a blue background, regardless off end of paragraph. That's also to be fixed for mathed. We really need transparent colors then. Then compute a selection color for each Inset based on their background color and the selection color. That could be difficult. I'd say it's OK if we loose the initial background color... at least for now... I mean, if you're looking for something else to fix ;-) I don't want to sound rude, but I will probably not soon be 'looking' for something to fix... ;-) Hum, not sure I understand... Abdel. Vincent PS. Do you know that at every mouse click, how innocent it might be, there is a full metrics update ? If yes, can you tell me why this is. It looks a bit a waste of CPU cycles... (i.e. all checksums we are calculating disappear). Should not be. I made sure that was not the case a number of times. Maybe not true anymore... and I of course agree with you here. Abdel.
RE: [patch] Solving selection painting bugs
> > We also need to fix the selection painting within Inset. > > and *of* insets themselves. > > f.e. if you change the color of selected text to white in > preferences you will see that say the numbers of sections > stay black (whereas we would then like them white). it > would be nice if this type of stuff is fixed indeed. > > last time i tried to understand this type of stuff it seemed > that insets are ignorant of their selection state. Yes, that is. > i remember starting to passing around a selection parameter, > but i had to touch *many* files. this was too daunting for > me so i aborted the operation... Same overhere, had a look at it, and stranded... But it doesn't seem that important for the near future, unless I hear objections... then... > ed. Vincent
RE: [patch] Solving selection painting bugs
> > Anyone thinks this has potential (see attachment) ? > > > > I do :-) > In that case, I added an extra boolean to the Row class saying whether there is an end of paragraph end or a normal end of line. Now, we can experiment with different kinds of painting. Maybe the Apple HIG says something about it ;-) > We also need to fix the selection painting within Inset. In > your example, the inset should have a blue background, regardless > off end of paragraph. That's also to be fixed for mathed. We really need transparent colors then. Then compute a selection color for each Inset based on their background color and the selection color. > I mean, if you're looking for something else to fix ;-) I don't want to sound rude, but I will probably not soon be 'looking' for something to fix... ;-) > Abdel. Vincent PS. Do you know that at every mouse click, how innocent it might be, there is a full metrics update ? If yes, can you tell me why this is. It looks a bit a waste of CPU cycles... (i.e. all checksums we are calculating disappear).
RE: [patch] Solving selection painting bugs
> We also need to fix the selection painting within Inset. and *of* insets themselves. f.e. if you change the color of selected text to white in preferences you will see that say the numbers of sections stay black (whereas we would then like them white). it would be nice if this type of stuff is fixed indeed. last time i tried to understand this type of stuff it seemed that insets are ignorant of their selection state. i remember starting to passing around a selection parameter, but i had to touch *many* files. this was too daunting for me so i aborted the operation... ed.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: About the new selection painting with your patch: I realize now that we are back to 1.5 style. The goal in the previous painting was to be able to differentiate betwen row break and new paragraph within a selection. Anyone thinks this has potential (see attachment) ? I do :-) Or has other ideas about painting the selection, especially with respect to end of paragraphs and other margins ? We also need to fix the selection painting within Inset. In your example, the inset should have a blue background, regardless off end of paragraph. That's also to be fixed for mathed. I mean, if you're looking for something else to fix ;-) Abdel.
RE: [patch] Solving selection painting bugs
> About the new selection painting with your patch: I realize now that > we are back to 1.5 style. The goal in the previous painting was to be > able to differentiate betwen row break and new paragraph within a > selection. > Anyone thinks this has potential (see attachment) ? Or has other ideas about painting the selection, especially with respect to end of paragraphs and other margins ? Vincent <>
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote: >> Please verify that I didn't overlooked something. >> > Just some nitpicks in name of consistency... its in. pavel
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote:
Please verify that I didn't overlooked something.
Just some nitpicks in name of consistency...
Vincent
Index: src/Row.cpp
===
--- src/Row.cpp (revision 26401)
+++ src/Row.cpp (working copy)
@@ -65,11 +65,11 @@
}
-bool Row::isMarginSelected(bool margin_begin, DocIterator const & beg,
-DocIterator
const & end) const
+bool Row::isMarginSelected(bool left_margin, DocIterator const & beg,
+ DocIterator const & end) const
{
- pos_type const sel_pos = margin_begin ? sel_beg : sel_end;
- pos_type const margin_pos = margin_begin ? pos_ : end_;
+ pos_type const sel_pos = left_margin ? sel_beg : sel_end;
+ pos_type const margin_pos = left_margin ? pos_ : end_;
// Is the chosen margin selected ?
if (sel_pos == margin_pos) {
@@ -94,7 +94,7 @@
void Row::setSelectionAndMargins(DocIterator const & beg,
-DocIterator
const & end) const
+ DocIterator const & end) const
{
setSelection(beg.pos(), end.pos());
Index: src/Row.h
===
--- src/Row.h (revision 26401)
+++ src/Row.h (working copy)
@@ -49,8 +49,8 @@
void setSelection(pos_type sel_beg, pos_type sel_end) const;
///
bool selection() const;
- /// Set the selection begin and end and whether the margin begin and end
- /// are selected.
+ /// Set the selection begin and end and whether the left and/or right
+ /// margins are selected.
void setSelectionAndMargins(DocIterator const & beg,
DocIterator const & end) const;
@@ -100,7 +100,7 @@
* \param beg
* \param end
*/
- bool isMarginSelected(bool margin_begin, DocIterator const & beg,
+ bool isMarginSelected(bool left_margin, DocIterator const & beg,
DocIterator const & end) const;
/// has the Row appearance changed since last drawing?
RE: [patch] Solving selection painting bugs
> About the new selection painting with your patch: I realize now that > we are back to 1.5 style. The goal in the previous painting was to be > able to differentiate betwen row break and new paragraph within a > selection. > The most important improvements are: - some real bugs are removed, - the margins are always drawn immediately, - the selection is always drawn from the anchor point, even if it is a 'boundary-point'. > I realize now that this is not easy to do consistently so going backward > is perhaps preferable. I think, it is not so difficult to adapt the new code to the style you want. All within paragraph are managed by Row, whereas the between paragraph margins are managed by drawParagraph. Options: - the between paragraphs end margin could be painted by a block with a width of a space. This is done by more applications: TextPad, MSVC, Outlook ... - the difference could be made by painting in different colors.. I'm also thinking to do this for the normal painting. I don't like the absurd justification in front of a multi-line inset. We might also just break the line, but paint the margin differently such that you know that there is no paragraph break. Vincent
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote:
I'll make this change and commit.
Thanks,
Abdel.
You might also consider Ed's comment about the naming in the Row class..
Maybe left_margin_sel and right_margin_sel are better names, but then
the naming of the functions should also change..
I did that. I got rid of the access function as those members are public
anyway.
There are also a number of style issue (not all yours) that I corrected.
Please have a look at what I committed below. In general:
- no camelBump style for local variable (drawOnBegMargin)
- no C-style pre-declaration, declare a local variable only when you
need it (ex: drawOnBegMargin)
- no local copy if the variable is used once (drawOnBegMargin is not needed)
- doxygen documentation in header, not in cpp.
- locally used method should be private (ex: isMarginSelected())
- 80 char per line maximum.
- local variables should be const when they are not meant to change.
Please verify that I didn't overlooked something.
About the new selection painting with your patch: I realize now that we
are back to 1.5 style. The goal in the previous painting was to be able
to differentiate betwen row break and new paragraph within a selection.
I realize now that this is not easy to do consistently so going backward
is perhaps preferable.
Abdel.
Modified: lyx-devel/trunk/src/ParagraphMetrics.cpp
URL:http://www.lyx.org/trac/file/lyx-devel/trunk/src/ParagraphMetrics.cpp?rev=26399
==
--- lyx-devel/trunk/src/ParagraphMetrics.cpp (original)
+++ lyx-devel/trunk/src/ParagraphMetrics.cpp Sun Sep 14 16:32:40 2008
@@ -103,9 +103,10 @@
}
Dimension const& d = row.dimension();
- char_type const b[] = { row.sel_beg, row.sel_end, d.wid, d.asc, d.des};
- // Each of the variable to process is 4 bytes: 4x5 = 20
- crc.process_bytes(b, 20);
+ char_type const b[] = { row.sel_beg, row.sel_end,
+ row.left_margin_sel, row.right_margin_sel, d.wid, d.asc, d.des};
+ // Each of the variable to process is 4 bytes: 4x7 = 28
+ crc.process_bytes(b, 28);
return crc.checksum();
}
Modified: lyx-devel/trunk/src/Row.cpp
URL:http://www.lyx.org/trac/file/lyx-devel/trunk/src/Row.cpp?rev=26399
==
--- lyx-devel/trunk/src/Row.cpp (original)
+++ lyx-devel/trunk/src/Row.cpp Sun Sep 14 16:32:40 2008
@@ -18,6 +18,8 @@
#include "Row.h"
+#include "DocIterator.h"
+
#include "support/debug.h"
@@ -26,13 +28,15 @@
Row::Row()
: separator(0), label_hfill(0), x(0),
- sel_beg(-1), sel_end(-1), changed_(false), crc_(0), pos_(0), end_(0)
+ sel_beg(-1), sel_end(-1), changed_(false), crc_(0),
+ pos_(0), end_(0), left_margin_sel(false), right_margin_sel(false)
{}
Row::Row(pos_type pos)
: separator(0), label_hfill(0), x(0),
- sel_beg(-1), sel_end(-1), changed_(false), crc_(0), pos_(pos), end_(0)
+ sel_beg(-1), sel_end(-1), changed_(false), crc_(0),
+ pos_(0), end_(0), left_margin_sel(false), right_margin_sel(false)
{}
@@ -61,6 +65,46 @@
}
+bool Row::isMarginSelected(bool margin_begin, DocIterator const& beg,
+DocIterator
const& end) const
+{
+ pos_type const sel_pos = margin_begin ? sel_beg : sel_end;
+ pos_type const margin_pos = margin_begin ? pos_ : end_;
+
+ // Is the chosen margin selected ?
+ if (sel_pos == margin_pos) {
+ if (beg.pos() == end.pos())
+ // This is a special case in which the space between
after
+ // pos i-1 and before pos i is selected, i.e. the
margins
+ // (see DocIterator::boundary_).
+ return beg.boundary()&& !end.boundary();
+ else if (end.pos() == margin_pos)
+ // If the selection ends around the margin, it is only
+ // drawn if the cursor is after the margin.
+ return !end.boundary();
+ else if (beg.pos() == margin_pos)
+ // If the selection begins around the margin, it is
+ // only drawn if the cursor is before the margin.
+ return beg.boundary();
+ else
+ return true;
+ }
+ return false;
+}
+
+
+void Row::setSelectionAndMargins(DocIterator const& beg,
+DocIterator
const& end) const
+{
+ setSelection(beg.pos(), end.pos());
+
+ if (selection()) {
+ right_margin_sel = isMarginSelected(false, beg, end);
+ left_margin_sel = isMarginSelected(true, beg, end);
+ }
+}
+
+
void Row::setSelection(pos_type beg, pos_type end) const
{
if (pos_>= beg&& pos_<= end)
@
RE: [patch] Solving selection painting bugs
> I'll make this change and commit. > > Thanks, > Abdel. You might also consider Ed's comment about the naming in the Row class.. Maybe left_margin_sel and right_margin_sel are better names, but then the naming of the functions should also change.. Vincent
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote: Abdelrazak Younes wrote: Yes, that sounds a better and cleaner solution. We basically only keep rows that are shown on screen in memory so adding two move variable is not a big problem. Abdel. Hi, I sort of cut the new code in four. It is no longer computed for and after the checksum and the code is generalized for both the begin as end margin. Besides it is mostly transferred into Row, so the drawParagraph function is now as clean as it used to be. Thanks for that patch, I like it much better than the previous one. Just one nitpick: +void Row::setEndMarginSelection(bool const end_margin) const We typically don't use const when passing POD data so that should be: +void Row::setEndMarginSelection(bool end_margin) const I'll make this change and commit. Thanks, Abdel.
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote: > Both looks good to me. Somebody please commit them (or wait until you have > commit privilege). i did this on your request. pavel
RE: [patch] Solving selection painting bugs
>>mutable bool margin_beg_sel; >>mutable bool margin_end_sel; > >if these bools indicate whether you want to draw some selection to >either the left or right, why not simply call >them: > >leftmargin_sel >rightmargin_sel > >(just ignore me if it shows that i didn't study your patch) Yes, that's also an option. In TextMetrics::drawParagraph they were called beg_margin and end_margin, so I sort of used the same terms. Now you say, it is very clear to call them that. Vincent
RE: [patch] Solving selection painting bugs
>mutable bool margin_beg_sel; >mutable bool margin_end_sel; if these bools indicate whether you want to draw some selection to either the left or right, why not simply call them: leftmargin_sel rightmargin_sel (just ignore me if it shows that i didn't study your patch)
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote:
Yes, that sounds a better and cleaner solution. We basically only keep
rows that are shown on screen in memory so adding two move variable is
not a big problem.
Abdel.
Hi,
I sort of cut the new code in four. It is no longer computed for and
after the checksum and the code is generalized for both the begin as end
margin. Besides it is mostly transferred into Row, so the drawParagraph
function is now as clean as it used to be.
The most important things are:
+///
+mutable bool margin_beg_sel;
+///
+mutable bool margin_end_sel;
Two new members added to Row.
-row.setSelection(sel_beg.pos(), sel_end.pos());
+row.setSelectionAndMargins(sel_beg_par, sel_end_par);
I added this function Row::setSelectionAndMargins(..), which now sets
the selection and immediately computes whether the margins are selected
or not.
+if (row.sel_beg == 0)
+row.setBeginMarginSelection(sel_beg.pit() < pit);
+if (row.sel_end == sel_end_par.lastpos())
+row.setEndMarginSelection(sel_end.pit() > pit);
This is needed because the Row does not know a thing about other
paragraphs. This is just old code with two if statements.
+char_type const b[] = { row.sel_beg, row.sel_end,
+row.margin_beg_sel, row.margin_end_sel, d.wid, d.asc, d.des};
+// Each of the variable to process is 4 bytes: 4x7 = 28
+crc.process_bytes(b, 28);
This is all code left for calculating the checksum. Only two terms are added
I also added Row::isMarginSelected( .. ). This function computes for
both the begin margin (first argument true) as for the end margin
whether they are selected. The essential code in this function is now
very easy to understand and consists mostly of comments.
Further, just some access functions and a Row::selection().. just for ease.
Thank you for your critics,
Vincent
Index: src/TextMetrics.cpp
===
--- src/TextMetrics.cpp (revision 26384)
+++ src/TextMetrics.cpp (working copy)
@@ -1987,15 +1987,19 @@
&& cur.anchor().text() == text_
&& pit >= sel_beg.pit() && pit <= sel_end.pit();
+ // We store the begin and end pos of the selection relative to this par
+ DocIterator sel_beg_par = cur.selectionBegin();
+ DocIterator sel_end_par = cur.selectionEnd();
+
// We care only about visible selection.
if (selection) {
if (pit != sel_beg.pit()) {
- sel_beg.pit() = pit;
- sel_beg.pos() = 0;
+ sel_beg_par.pit() = pit;
+ sel_beg_par.pos() = 0;
}
if (pit != sel_end.pit()) {
- sel_end.pit() = pit;
- sel_end.pos() = sel_end.lastpos();
+ sel_end_par.pit() = pit;
+ sel_end_par.pos() = sel_end_par.lastpos();
}
}
@@ -2012,9 +2016,18 @@
RowPainter rp(pi, *text_, pit, row, bidi, x, y);
if (selection)
- row.setSelection(sel_beg.pos(), sel_end.pos());
+ row.setSelectionAndMargins(sel_beg_par, sel_end_par);
else
row.setSelection(-1, -1);
+
+ // The row knows nothing about the paragraph, so we have to
check
+ // whether this row is the first or last and update the margins.
+ if (row.selection()) {
+ if (row.sel_beg == 0)
+ row.setBeginMarginSelection(sel_beg.pit() <
pit);
+ if (row.sel_end == sel_end_par.lastpos())
+ row.setEndMarginSelection(sel_end.pit() > pit);
+ }
// Row signature; has row changed since last paint?
row.setCrc(pm.computeRowSignature(row, bparams));
@@ -2036,34 +2049,18 @@
pi.pain.fillRectangle(x, y - row.ascent(),
width(), row.height(), pi.background_color);
}
+
+ if (row.selection())
+ drawRowSelection(pi, x, row, cur, pit);
- bool row_selection = row.sel_beg != -1 && row.sel_end != -1;
- if (row_selection) {
- DocIterator beg = bv_->cursor().selectionBegin();
- DocIterator end = bv_->cursor().selectionEnd();
- // FIXME (not here): pit is not updated when extending
- // a selection to a new row with cursor right/left
- bool const beg_margin = beg.pit() < pit;
- bool const end_margin = end.pit() > pit;
- beg.pit() = pit;
- beg.pos() = row.sel_beg;
-
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote:
Vincent van Ravesteijn - TNW wrote:
As a general comment on the patch, I didn't review it in detail but
I feel that this is a lot of additional code.
The only new code is in the logic whether to draw the margins or not.
And this was just the problem that I had with how LyX drawn the margins.
Sometimes it does, sometimes with a delay until the next full repaint,
sometimes not at all.
I don't like the new arguments to 'computeRowSignature()'; maybe the
reason is that the Row object doesn't keep track of the boundary; if
you add this member in addition to sel_beg and sel_end I guess you
have everything needed and this will result in lesser code. But that's
just a guess.
I understand. Maybe I oversaw the most obvious solution: Move the
end_margin and beg_margin code before the checksum computation.
Yes, that sounds a better and cleaner solution. We basically only keep
rows that are shown on screen in memory so adding two move variable is
not a big problem.
Now,
basically both computeRowSignature and drawParagraph are doing the same
thing. Then, I maybe only have to pass "row, bparams, bool beg_margin,
and bool end_margin". Can you live with this ? Or indeed, add beg_margin
and end_margin to Row, (I intentionally don't want to use the term
boundary).
The later yes.
I would understand if you feel demotivated by my repetitive comments.
I do not want you to commit code you don't like.
Thanks. I am happy that someone else besides me put some interests
into painting.
PS. Can you still copy-paste insets ? I can't since revision, well
approximately 26360.
No, you're right!
My last commit seems to fix this.
Abdel.
Author: younes
Date: Sat Sep 13 17:32:26 2008
New Revision: 26383
URL:http://www.lyx.org/trac/changeset/26383
Log:
Fix silly bug spotted by Vincent in r26372
Modified:
lyx-devel/trunk/src/CutAndPaste.cpp
Modified: lyx-devel/trunk/src/CutAndPaste.cpp
URL:http://www.lyx.org/trac/file/lyx-devel/trunk/src/CutAndPaste.cpp?rev=26383
==
--- lyx-devel/trunk/src/CutAndPaste.cpp (original)
+++ lyx-devel/trunk/src/CutAndPaste.cpp Sat Sep 13 17:32:26 2008
@@ -195,7 +195,7 @@
for (pos_type i = 0; i< tmpbuf->size(); ++i) {
// do not track deletion of invalid insets
if (Inset * inset = tmpbuf->getInset(i))
- if
(target_inset->insetAllowed(inset->lyxCode()))
+ if
(!target_inset->insetAllowed(inset->lyxCode()))
tmpbuf->eraseChar(i--, false);
}
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: As a general comment on the patch, I didn't review it in detail but I feel that this is a lot of additional code. The only new code is in the logic whether to draw the margins or not. And this was just the problem that I had with how LyX drawn the margins. Sometimes it does, sometimes with a delay until the next full repaint, sometimes not at all. I don't like the new arguments to 'computeRowSignature()'; maybe the reason is that the Row object doesn't keep track of the boundary; if you add this member in addition to sel_beg and sel_end I guess you have everything needed and this will result in lesser code. But that's just a guess. I understand. Maybe I oversaw the most obvious solution: Move the end_margin and beg_margin code before the checksum computation. Yes, that sounds a better and cleaner solution. We basically only keep rows that are shown on screen in memory so adding two move variable is not a big problem. Now, basically both computeRowSignature and drawParagraph are doing the same thing. Then, I maybe only have to pass "row, bparams, bool beg_margin, and bool end_margin". Can you live with this ? Or indeed, add beg_margin and end_margin to Row, (I intentionally don't want to use the term boundary). The later yes. I would understand if you feel demotivated by my repetitive comments. I do not want you to commit code you don't like. Thanks. I am happy that someone else besides me put some interests into painting. PS. Can you still copy-paste insets ? I can't since revision, well approximately 26360. No, you're right! Abdel.
RE: [patch] Solving selection painting bugs
> As a general comment on the patch, I didn't review it in detail but > I feel that this is a lot of additional code. The only new code is in the logic whether to draw the margins or not. And this was just the problem that I had with how LyX drawn the margins. Sometimes it does, sometimes with a delay until the next full repaint, sometimes not at all. > I don't like the new arguments to 'computeRowSignature()'; maybe the > reason is that the Row object doesn't keep track of the boundary; if > you add this member in addition to sel_beg and sel_end I guess you > have everything needed and this will result in lesser code. But that's > just a guess. I understand. Maybe I oversaw the most obvious solution: Move the end_margin and beg_margin code before the checksum computation. Now, basically both computeRowSignature and drawParagraph are doing the same thing. Then, I maybe only have to pass "row, bparams, bool beg_margin, and bool end_margin". Can you live with this ? Or indeed, add beg_margin and end_margin to Row, (I intentionally don't want to use the term boundary). > I would understand if you feel demotivated by my repetitive comments. I do not want you to commit code you don't like. > FYI I basically rewrote this part for 1.6 and this resulted in much > lesser code than in 1.5, I wouldn't like the code to grow too much > again. That's what I was afraid of. > But I'd be OK with your patch if you promise to think about my suggestion :-) > > Abdel. Thinking right now. Vincent PS. Can you still copy-paste insets ? I can't since revision, well approximately 26360.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote:
Abdel,
I took the liberty to change the drawParagraph function in
TextMetrics.cpp a bit. This because, after you asked me to use
meaningfull names for the variables, I tried to understand the other
names in the function and I didn't succeed. I felt that variables
shouldn't change their role during the function and that the boundary
member shouldn't be used differently than how it is documented.
Besides, I improved the logic of drawing the margins further, such
that I cannot find any drawing errors left.
Here are some explanations:
+CursorSlice par_slice = cur.top();
Are you sure you want a copy and not a reference? Because you use
par_slice.lastpos() below and this points to the previous pit that you
change here:
+par_slice.pit() = pit;
+
+// We store the begin and end pos of the selection relative to
this par
+pos_type sel_beg_par_pos = -1;
+pos_type sel_end_par_pos = -1;
// We care only about visible selection.
if (selection) {
-if (pit != sel_beg.pit()) {
-sel_beg.pit() = pit;
-sel_beg.pos() = 0;
-}
-if (pit != sel_end.pit()) {
-sel_end.pit() = pit;
-sel_end.pos() = sel_end.lastpos();
-}
+sel_beg_par_pos = sel_beg.pit() == pit ? sel_beg.pos() : 0;
+sel_end_par_pos = sel_end.pit() == pit ?
+sel_end.pos() : par_slice.lastpos();
}
1. Sel_beg and sel_end were first used to indicate the current
selection, then they were adapted to represent the selection
clipped to the current paragraph (without comment). I think
it is better just to make two variables for that and to let
sel_beg and sel_end keep the same role throughout the function.
-DocIterator beg = bv_->cursor().selectionBegin();
-DocIterator end = bv_->cursor().selectionEnd();
// FIXME (not here): pit is not updated when extending
// a selection to a new row with cursor right/left
-bool const beg_margin = beg.pit() < pit;
-bool const end_margin = end.pit() > pit;
-beg.pit() = pit;
-beg.pos() = row.sel_beg;
-end.pit() = pit;
-end.pos() = row.sel_end;
-if (end.pos() == row.endpos()) {
-// selection goes till the end of the row.
-end.boundary(true);
2. The same as above, beg and end are first used as the begin and end of
the selection. Then, they are clipped to the row. Moreover, the
boundary member is misused to carry information that says nothing
about whether the cursor is after i-1 or before i.
As this is just administration I moved it into drawSelection().
// FIXME (not here): pit is not updated when extending // a selection
to a new row with cursor right/left
3. I don't know whether this FIXME is still necessary. I didn't found
any
problems with it.
Maybe not anymore.
AS a general comment on the patch, I didn't review it in detail but I
feel that this is a lot of additional code. I don't like the new
arguments to 'computeRowSignature()'; maybe the reason is that the Row
object doesn't keep track of the boundary; if you add this member in
addition to sel_beg and sel_end I guess you have everything needed and
this will result in lesser code. But that's just a guess. I would
understand if you feel demotivated by my repetitive comments. FYI I
basically rewrote this part for 1.6 and this resulted in much lesser
code than in 1.5, I wouldn't like the code to grow too much again. But
I'd be OK with your patch if you promise to think about my suggestion :-)
Abdel.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote: Vincent van Ravesteijn - TNW wrote: First, when only selecting the end_margin between 'after i-1' and 'before i', the anchor is incorrectly set to be equal to the cur. This is because comparing two CursorSlices does not take into account the boundary property (because the CursorSlice does not know this). Second, if we call cur.setSelection() in line Text3.cpp:1256 we should require some sort of repaint, because if the selection is adjusted, we may have to draw the selection again. Here are two patches solving these two minor issues. Both looks good to me. Somebody please commit them (or wait until you have commit privilege). It might be a good idea to transfer the boundary to CursorSlice, but that is maybe not for 1.6.0. Abdel.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn wrote: Abdel, I took the liberty to change the drawParagraph function in TextMetrics.cpp a bit. This because, after you asked me to use meaningfull names for the variables, I tried to understand the other names in the function and I didn't succeed. I felt that variables shouldn't change their role during the function and that the boundary member shouldn't be used differently than how it is documented. Good, you are making the next step in your LyX carrier: detect and correct bad style. In this case probably my bad style, probably inherited from a bad style that I didn't dare correcting at the time; it's good that you dare ;-) I'll try to review the patch over the week-end. Looks good at a first glance. Abdel.
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote:
First, when only selecting the end_margin between 'after i-1' and
'before i', the anchor is incorrectly set to be equal to the cur. This
is because comparing two CursorSlices does not take into account the
boundary property (because the CursorSlice does not know this).
Second, if we call cur.setSelection() in line Text3.cpp:1256 we should
require some sort of repaint, because if the selection is adjusted, we
may have to draw the selection again.
Here are two patches solving these two minor issues.
Vincent
Index: src/Text3.cpp
===
--- src/Text3.cpp (revision 26379)
+++ src/Text3.cpp (working copy)
@@ -1254,9 +1254,12 @@
// selectWord but bvcur is current
// mouse position.
cur.bv().cursor().setSelection();
- }
+ // We might have removed an empty but drawn
selection
+ // (probably a margin)
+ cur.updateFlags(Update::SinglePar |
Update::FitCursor);
+ } else
+ cur.noUpdate();
// FIXME: We could try to handle drag and drop of
selection here.
- cur.noUpdate();
return;
case mouse_button::button2:
Index: src/Cursor.cpp
===
--- src/Cursor.cpp (revision 26379)
+++ src/Cursor.cpp (working copy)
@@ -944,7 +944,14 @@
{
if (!selection())
return *this;
- DocIterator di = (anchor() < top() ? anchor_ : *this);
+
+ DocIterator di;
+ // FIXME: This is a work-around for the problem that
+ // CursorSlice doesn't keep track of the boundary.
+ if (anchor() == top())
+ di = anchor_.boundary() > boundary() ? anchor_ : *this;
+ else
+ di = anchor() < top() ? anchor_ : *this;
di.resize(depth());
return di;
}
@@ -954,7 +961,15 @@
{
if (!selection())
return *this;
- DocIterator di = (anchor() > top() ? anchor_ : *this);
+
+ DocIterator di;
+ // FIXME: This is a work-around for the problem that
+ // CursorSlice doesn't keep track of the boundary.
+ if (anchor() == top())
+ di = anchor_.boundary() < boundary() ? anchor_ : *this;
+ else
+ di = anchor() > top() ? anchor_ : *this;
+
if (di.depth() > depth()) {
di.resize(depth());
++di.pos();
Re: [patch] Solving selection painting bugs
Abdel,
I took the liberty to change the drawParagraph function in
TextMetrics.cpp a bit. This because, after you asked me to use
meaningfull names for the variables, I tried to understand the other
names in the function and I didn't succeed. I felt that variables
shouldn't change their role during the function and that the boundary
member shouldn't be used differently than how it is documented.
Besides, I improved the logic of drawing the margins further, such that
I cannot find any drawing errors left.
Here are some explanations:
+CursorSlice par_slice = cur.top();
+par_slice.pit() = pit;
+
+// We store the begin and end pos of the selection relative to this par
+pos_type sel_beg_par_pos = -1;
+pos_type sel_end_par_pos = -1;
// We care only about visible selection.
if (selection) {
-if (pit != sel_beg.pit()) {
-sel_beg.pit() = pit;
-sel_beg.pos() = 0;
-}
-if (pit != sel_end.pit()) {
-sel_end.pit() = pit;
-sel_end.pos() = sel_end.lastpos();
-}
+sel_beg_par_pos = sel_beg.pit() == pit ? sel_beg.pos() : 0;
+sel_end_par_pos = sel_end.pit() == pit ?
+sel_end.pos() : par_slice.lastpos();
}
1. Sel_beg and sel_end were first used to indicate the current
selection, then they were adapted to represent the selection
clipped to the current paragraph (without comment). I think
it is better just to make two variables for that and to let
sel_beg and sel_end keep the same role throughout the function.
-DocIterator beg = bv_->cursor().selectionBegin();
-DocIterator end = bv_->cursor().selectionEnd();
// FIXME (not here): pit is not updated when extending
// a selection to a new row with cursor right/left
-bool const beg_margin = beg.pit() < pit;
-bool const end_margin = end.pit() > pit;
-beg.pit() = pit;
-beg.pos() = row.sel_beg;
-end.pit() = pit;
-end.pos() = row.sel_end;
-if (end.pos() == row.endpos()) {
-// selection goes till the end of the row.
-end.boundary(true);
2. The same as above, beg and end are first used as the begin and end of
the selection. Then, they are clipped to the row. Moreover, the
boundary member is misused to carry information that says nothing
about whether the cursor is after i-1 or before i.
As this is just administration I moved it into drawSelection().
// FIXME (not here): pit is not updated when extending // a selection to
a new row with cursor right/left
3. I don't know whether this FIXME is still necessary. I didn't found any
problems with it.
+bool const begin_boundary = beg.pos() >= row.endpos();
4. This is necessary to prevent painting errors.
Vincent
==
Index: src/TextMetrics.cpp
===
--- src/TextMetrics.cpp (revision 26377)
+++ src/TextMetrics.cpp (working copy)
@@ -1971,16 +1971,17 @@
&& cur.anchor().text() == text_
&& pit >= sel_beg.pit() && pit <= sel_end.pit();
+ CursorSlice par_slice = cur.top();
+ par_slice.pit() = pit;
+
+ // We store the begin and end pos of the selection relative to this par
+ pos_type sel_beg_par_pos = -1;
+ pos_type sel_end_par_pos = -1;
// We care only about visible selection.
if (selection) {
- if (pit != sel_beg.pit()) {
- sel_beg.pit() = pit;
- sel_beg.pos() = 0;
- }
- if (pit != sel_end.pit()) {
- sel_end.pit() = pit;
- sel_end.pos() = sel_end.lastpos();
- }
+ sel_beg_par_pos = sel_beg.pit() == pit ? sel_beg.pos() : 0;
+ sel_end_par_pos = sel_end.pit() == pit ?
+ sel_end.pos() : par_slice.lastpos();
}
for (size_t i = 0; i != nrows; ++i) {
@@ -1995,13 +1996,11 @@
pi.pain.setDrawingEnabled(inside && original_drawing_state);
RowPainter rp(pi, *text_, pit, row, bidi, x, y);
- if (selection)
- row.setSelection(sel_beg.pos(), sel_end.pos());
- else
- row.setSelection(-1, -1);
-
+ row.setSelection(sel_beg_par_pos, sel_end_par_pos);
+
// Row signature; has row changed since last paint?
- row.setCrc(pm.computeRowSignature(row, bparams));
+ row.setCrc(pm.computeRowSignature(row, bparams,
+ sel_beg, sel_end, pit));
bool row_has_changed = row.changed();
// Don't paint the row if a full repaint has not been requested
@@ -2023,21 +2022,51 @@
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: > After solving these two minor issues (I will send a patch when I'm > finished with this stuff), I did not have problems with the bug I > mentioned before, so don't spend time on it. Thanks for the explanations. JMarc
RE: [patch] Solving selection painting bugs
> Could you restate again what the bug is? > > Jmarc The bug _was_ that when making a selection, cur.selection() is true even if the selection is still empty. In cs 26146 we removed such an empty selection when the cursor is released. But, before releasing the mouse, the 'problem' might still be there. While working to improve the drawing of the selection, I also encountered two other problems. First, when only selecting the end_margin between 'after i-1' and 'before i', the anchor is incorrectly set to be equal to the cur. This is because comparing two CursorSlices does not take into account the boundary property (because the CursorSlice does not know this). Second, if we call cur.setSelection() in line Text3.cpp:1256 we should require some sort of repaint, because if the selection is adjusted, we may have to draw the selection again. After solving these two minor issues (I will send a patch when I'm finished with this stuff), I did not have problems with the bug I mentioned before, so don't spend time on it. Vincent
Re: [patch] Solving selection painting bugs
"Vincent van Ravesteijn - TNW" <[EMAIL PROTECTED]> writes: > For the moment, forget this cur.setSelection() part. It is still not > correct probably. Although I don't encounter the problem anymore, it > might not be correct to call it in cur.setSelection(...). Maybe it > breaks down something else. > > cs 26146 did solve a same problem, but then the selection is removed > when the mouse is released. Iin this case however, the mouse isn't > released yet, so the empty selections still exist. Maybe we should add > it to line 1234 in Text3.cpp ? Could you restate again what the bug is? JMarc
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: As Richard said, it would be nice if you didn't attach your patch as mime as they do not appear inline with Thunderbird. I think the problem is that the mime-type is octet-stream instead of text. JMarc That's indeed what gmane says. I also see there that the "[patch] Cheering up Listings" and "[patch] Tabbing in Listings" were displayed correctly. Did this display correctly in Thunderbird too then ? Yes. If so, I don't understand what made it to change. Bytheway, I already installed Thunderbird, so I hope these problems will disappear. Vincent
RE: [patch] Solving selection painting bugs
>> As Richard said, it would be nice if you didn't attach your patch as >> mime as they do not appear inline with Thunderbird. > >I think the problem is that the mime-type is octet-stream instead of text. > >JMarc > That's indeed what gmane says. I also see there that the "[patch] Cheering up Listings" and "[patch] Tabbing in Listings" were displayed correctly. Did this display correctly in Thunderbird too then ? If so, I don't understand what made it to change. Bytheway, I already installed Thunderbird, so I hope these problems will disappear. Vincent
Re: [patch] Solving selection painting bugs
Abdelrazak Younes <[EMAIL PROTECTED]> writes: > As Richard said, it would be nice if you didn't attach your patch as > mime as they do not appear inline with Thunderbird. I think the problem is that the mime-type is octet-stream instead of text. JMarc
Re: [patch] Solving selection painting bugs
On Tue, Sep 09, 2008 at 11:33:08PM +0200, Abdelrazak Younes wrote:
> Vincent van Ravesteijn - TNW wrote:
>> For the moment, forget this cur.setSelection() part. It is still not
>> correct probably. Although I don't encounter the problem anymore, it
>> might not be correct to call it in cur.setSelection(...). Maybe it
>> breaks down something else.
>>
>> cs 26146 did solve a same problem, but then the selection is removed
>> when the mouse is released. Iin this case however, the mouse isn't
>> released yet, so the empty selections still exist. Maybe we should add
>> it to line 1234 in Text3.cpp ?
>>
>
> I am not sure I understand. I'll let JMarc handle this part.
>
>> Consider it work in progress..
>>
> Still some comments:
>
> is_beg_pit and is_end_pit: please rename those to something more
> meaningful related to the selection state. IIUC, maybe 'is_sel_begin'
> and 'is_sel_end'?
>
> +char_type const bb[] = { row.sel_beg == 0 && is_beg_pit ? 1 : 0 +
> +row.sel_end == par_->size() && is_end_pit ? 2 : 0 +
> +row.sel_end == row.endpos() && sel_end_pos == row.endpos() ? 4
> : 0 };
>
Incitentalluy, since this is only a single char this might as well look
like
char_type const bb = row.sel_beg == 0 && is_beg_pit ? 1 : 0
+ row.sel_end == par_->size() && is_end_pit ? 2 : 0
+ row.sel_end == row.endpos() && sel_end_pos == row.endpos() ? 4 : 0;
foo(&bb, 1);
Look already a bot less convoluted.
char_type bb = (row.sel_beg == 0 && is_beg_pit);
bb |= (row.sel_end == par_->size() && is_end_pit) << 1;
bb |= (row.sel_end == row.endpos() && sel_end_pos == row.endpos()) << 2;
Hm. Not much difference.
char_type bb = 0;
if (row.sel_beg == 0 && is_beg_pit)
bb += 1;
if (row.sel_end == par_->size() && is_end_pit)
bb += 2;
if (row.sel_end == row.endpos() && sel_end_pos == row.endpos())
bb += 4;
might make the intention mor obvious, even if it's a bit longer.
Andre'
Re: [patch] Solving selection painting bugs
On Tue, Sep 09, 2008 at 11:08:01PM +0200, Abdelrazak Younes wrote: >> Hereby a new patch, without this nice feature. > > As Richard said, it would be nice if you didn't attach your patch as > mime as they do not appear inline with Thunderbird. It looks good in mutt. Of course, the html part can (and should) go, maybe that's what confuses thunderbird. Andre'
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote:
For the moment, forget this cur.setSelection() part. It is still not
correct probably. Although I don't encounter the problem anymore, it
might not be correct to call it in cur.setSelection(...). Maybe it
breaks down something else.
cs 26146 did solve a same problem, but then the selection is removed
when the mouse is released. Iin this case however, the mouse isn't
released yet, so the empty selections still exist. Maybe we should add
it to line 1234 in Text3.cpp ?
I am not sure I understand. I'll let JMarc handle this part.
Consider it work in progress..
Still some comments:
is_beg_pit and is_end_pit: please rename those to something more
meaningful related to the selection state. IIUC, maybe 'is_sel_begin'
and 'is_sel_end'?
+char_type const bb[] = { row.sel_beg == 0 && is_beg_pit ? 1 : 0 +
+row.sel_end == par_->size() && is_end_pit ? 2 : 0 +
+row.sel_end == row.endpos() && sel_end_pos == row.endpos() ? 4
: 0 };
This is too convoluted, please split this expression into more
intelligible smaller booleans.
Abdel.
Re: [patch] Solving selection painting bugs
Abdelrazak Younes wrote: Vincent van Ravesteijn - TNW wrote: Starting your selection in an end margin often causes the selection in this end margin to be painted later. This is because when starting your selection in an end margin, you may have set a (possible empty) selection before really selecting the end margin. The problem is that the checksum (computed later) is the same for this empty selection and for the end margin selection. Therfore, we need a call to cur.setSelection() before evaluating cur.selection(). + cur.setSelection(); bool selection = cur.selection() // This is our text. && cur.text() == text_ // if the anchor is outside, this is not our selection && cur.anchor().text() == text_ && pit>= sel_beg.pit()&& pit<= sel_end.pit(); ... leading to nice assertions. Hereby a new patch, without this nice feature. As Richard said, it would be nice if you didn't attach your patch as mime as they do not appear inline with Thunderbird. In Thunderbird, I can attach it with the "attach" dialog, and then have it show up BOTH as text, inline, and in a form where it can be saved to file. I'm not sure what configures this behavior, though. rh
RE: [patch] Solving selection painting bugs
> As Richard said, it would be nice if you didn't attach > your patch as mime as they do not appear inline with > Thunderbird. I'll try to figure this out. > I'll try to review this patch. > > Abdel. For the moment, forget this cur.setSelection() part. It is still not correct probably. Although I don't encounter the problem anymore, it might not be correct to call it in cur.setSelection(...). Maybe it breaks down something else. cs 26146 did solve a same problem, but then the selection is removed when the mouse is released. Iin this case however, the mouse isn't released yet, so the empty selections still exist. Maybe we should add it to line 1234 in Text3.cpp ? Consider it work in progress.. Vincent
Re: [patch] Solving selection painting bugs
Vincent van Ravesteijn - TNW wrote: Starting your selection in an end margin often causes the selection in this end margin to be painted later. This is because when starting your selection in an end margin, you may have set a (possible empty) selection before really selecting the end margin. The problem is that the checksum (computed later) is the same for this empty selection and for the end margin selection. Therfore, we need a call to cur.setSelection() before evaluating cur.selection(). + cur.setSelection(); bool selection = cur.selection() // This is our text. && cur.text() == text_ // if the anchor is outside, this is not our selection && cur.anchor().text() == text_ && pit>= sel_beg.pit()&& pit<= sel_end.pit(); ... leading to nice assertions. Hereby a new patch, without this nice feature. As Richard said, it would be nice if you didn't attach your patch as mime as they do not appear inline with Thunderbird. I'll try to review this patch. Abdel. Vincent
