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.
