On Thu, Dec 13, 2012 at 6:45 PM, Uwe Stöhr <uwesto...@web.de> wrote:
> Am 08.12.2012 08:09, schrieb Scott Kostyshak:
>
>
>> While studying InsetTabular{.cpp,.h}, I have some suggestions and
>> questions.
>>
>> In the 0001 patch attached, am I missing something or can we remove
>> cur.selection in the conditions?
>
>
> I would leave them in. It makes sense to me that we don#t accept a "down"
> selection if only the current cell is selected.
OK, I'll leave them in. I agree that it makes sense but my suggestion
was based on the fact that selIsMultiCell already checks that there is
a selection (any selection) :
bool selIsMultiCell() const
{ return selection_ && selBegin().idx() != selEnd().idx(); }
so we are doing something like if (selection_ && selection_ && ...)
but in reality it doesn't matter and the names make more sense as they
are.
>
>
>> In 0003, can I rename "copy-column" and "copy-row" to
>> "duplicate-column" and "duplicate-row"? The features do not just copy
>> but they also insert. The both together seem more of a "duplicate"
>> functionality.
>
>
> OK, if you like to. But for me "copy row " implies that the row content will
> appear somewhere else.
Agreed, but does it imply that the same function will also paste it?
To me it doesn't.
> So it is either a cut+paste or copy+paste. In the
> first case it is not a duplication. But I don't have a string opinion about
> the naming of the LFUN. But if you do the change, please also update our
> LFUN list and the LFUN documentation LyX-file.
I think I'll leave it. It's not clear that my suggestion is better
(because of your concerns) and it's not worth the potential mistake if
I change everything else.
>
>> There are also a few things I did not change but that I find confusing:
>> Why isn't "col_type" not named "col_num" and row_type "row_num"?
>> Does the "_type" emphasize that it's a typedef?
>
>
> I don't know. This must have historical reasons.
Yes, JMarc said that it's because it's the convention used in the STL.
>
>
>> - I find it unintuitive that a cell that is at the beginning of a
>> multirow is not "part" of a multirow:
>> See, for example, Tabular::isPartOfMultiRow.
>> I guess my confusion is because of:
>> CELL_BEGIN_OF_MULTICOLUMN,
>> ///
>> CELL_PART_OF_MULTICOLUMN,
>>
>> From a programming point of view, I understand why they are treated
>> differently, but from an interface point of view, I wonder if there
>> could be better names. I can't think of any though.
>
>
> Form the technical point of view there was no other solution and prom the UI
> point of view as a user you are never confronted with this and thus won't
> notice.
I agree, but from a "user of the code" perspective I think it is
confusing. This is not a big issue for me (now that I know the code)
but I was thinking of how to make things clearer for the next person
who wants to dig into InsetTabular.{cpp,h}.
This is the only issue remaining and I attached my proposed patch in
this same email thread (1 day ago I think). I'm attaching the same
patch to this email for convenience. I don't have a strong opinion so
if you don't like it I will not insist. What do you think?
>
>
>> - My same comment as above but for isPartOfMultiColumn.
>
>
> Dito.
>
>
>> I have two questions regarding the following code, which is in
>> function <<bool InsetTabular::getStatus(Cursor & cur, FuncRequest
>> const & cmd, FuncStatus & status) const>>,
>>
>> switch (action) {
>> case Tabular::SET_PWIDTH:
>> case Tabular::SET_MPWIDTH:
>> case Tabular::SET_SPECIAL_COLUMN:
>> case Tabular::SET_SPECIAL_MULTICOLUMN:
>> case Tabular::APPEND_ROW:
>> case Tabular::APPEND_COLUMN:
>> case Tabular::DELETE_ROW:
>> case Tabular::DELETE_COLUMN:
>> case Tabular::COPY_ROW:
>> case Tabular::COPY_COLUMN:
>> case Tabular::SET_TOP_SPACE:
>> case Tabular::SET_BOTTOM_SPACE:
>> case Tabular::SET_INTERLINE_SPACE:
>> status.clear();
>> return true;
>>
>>
>> (1) What is the purpose of this status.clear()?
>>
>> (2) Why is "return true" used here instead of "break" (there is a
>> "return true" right after the cases and all the other cases use
>> "break")? Is this just inconsistent style or do they not provide
>> equivalent functionality?
>
>
> I also don't know.
Thanks for the comments, Uwe!
Scott
From 465a224967786ee26578a90f12b36503600e8203 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Wed, 12 Dec 2012 03:57:56 -0500
Subject: [PATCH] In InsetTabular change a few names
Make the following name changes:
CELL_PART_OF_MULTIROW -> CELL_HIDDEN_BY_MULTIROW
CELL_PART_OF_MULTICOLUMN -> CELL_HIDDEN_BY_MULTICOLUMN
isPartOfMultiRow -> isHiddenByMultiRow
isPartOfMultiColumn -> isHiddenByMultiColumn
These changes were introduced because it was not clear that, for
example, isPartOfMultiRow would return false for an enum value of
CELL_BEGIN_OF_MULTIROW.
---
src/insets/InsetTabular.cpp | 94 +++++++++++++++++++++----------------------
src/insets/InsetTabular.h | 8 ++--
2 files changed, 51 insertions(+), 51 deletions(-)
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 6e2bc21..60fa689 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -713,7 +713,7 @@ void Tabular::deleteRow(row_type const row)
// Care about multirow cells
if (row + 1 < nrows() &&
cell_info[row][c].multirow == CELL_BEGIN_OF_MULTIROW &&
- cell_info[row + 1][c].multirow == CELL_PART_OF_MULTIROW) {
+ cell_info[row + 1][c].multirow == CELL_HIDDEN_BY_MULTIROW) {
cell_info[row + 1][c].multirow =
CELL_BEGIN_OF_MULTIROW;
}
}
@@ -747,12 +747,12 @@ void Tabular::insertRow(row_type const row, bool copy)
if (buffer().params().trackChanges)
cell_info[row +
1][c].inset->setChange(Change(Change::INSERTED));
if (cell_info[row][c].multirow == CELL_BEGIN_OF_MULTIROW)
- cell_info[row + 1][c].multirow = CELL_PART_OF_MULTIROW;
+ cell_info[row + 1][c].multirow =
CELL_HIDDEN_BY_MULTIROW;
}
updateIndexes();
for (col_type c = 0; c < ncols(); ++c) {
- if (isPartOfMultiRow(row, c))
+ if (isHiddenByMultiRow(row, c))
continue;
// inherit line settings
idx_type const i = cellIndex(row + 1, c);
@@ -834,7 +834,7 @@ void Tabular::deleteColumn(col_type const col)
// Care about multicolumn cells
if (col + 1 < ncols() &&
cell_info[r][col].multicolumn == CELL_BEGIN_OF_MULTICOLUMN
&&
- cell_info[r][col + 1].multicolumn ==
CELL_PART_OF_MULTICOLUMN) {
+ cell_info[r][col + 1].multicolumn ==
CELL_HIDDEN_BY_MULTICOLUMN) {
cell_info[r][col + 1].multicolumn =
CELL_BEGIN_OF_MULTICOLUMN;
}
cell_info[r].erase(cell_info[r].begin() + col);
@@ -867,7 +867,7 @@ void Tabular::insertColumn(col_type const col, bool copy)
if (bp.trackChanges)
cell_info[r][col +
1].inset->setChange(Change(Change::INSERTED));
if (cell_info[r][col].multicolumn == CELL_BEGIN_OF_MULTICOLUMN)
- cell_info[r][col + 1].multicolumn =
CELL_PART_OF_MULTICOLUMN;
+ cell_info[r][col + 1].multicolumn =
CELL_HIDDEN_BY_MULTICOLUMN;
}
updateIndexes();
for (row_type r = 0; r < nrows(); ++r) {
@@ -894,10 +894,10 @@ void Tabular::updateIndexes()
// reset cell number
for (row_type row = 0; row < nrows(); ++row)
for (col_type column = 0; column < ncols(); ++column) {
- if (!isPartOfMultiColumn(row, column)
- && !isPartOfMultiRow(row, column))
+ if (!isHiddenByMultiColumn(row, column)
+ && !isHiddenByMultiRow(row, column))
++numberofcells;
- if (isPartOfMultiRow(row, column))
+ if (isHiddenByMultiRow(row, column))
cell_info[row][column].cellno = cell_info[row -
1][column].cellno;
else
cell_info[row][column].cellno = numberofcells -
1;
@@ -909,16 +909,16 @@ void Tabular::updateIndexes()
// reset column and row of cells and update their width and alignment
for (row_type row = 0; row < nrows(); ++row)
for (col_type column = 0; column < ncols(); ++column) {
- if (isPartOfMultiColumn(row, column))
+ if (isHiddenByMultiColumn(row, column))
continue;
// columnofcell needs to be called before setting width
and aligment
// multirow cells inherit the width from the column
width
- if (!isPartOfMultiRow(row, column)) {
+ if (!isHiddenByMultiRow(row, column)) {
columnofcell[i] = column;
rowofcell[i] = row;
}
setFixedWidth(row, column);
- if (isPartOfMultiRow(row, column))
+ if (isHiddenByMultiRow(row, column))
continue;
cell_info[row][column].inset->setContentAlignment(
getAlignment(cellIndex(row, column)));
@@ -931,7 +931,7 @@ Tabular::idx_type Tabular::numberOfCellsInRow(row_type
const row) const
{
idx_type result = 0;
for (col_type c = 0; c < ncols(); ++c)
- if (cell_info[row][c].multicolumn !=
Tabular::CELL_PART_OF_MULTICOLUMN)
+ if (cell_info[row][c].multicolumn !=
Tabular::CELL_HIDDEN_BY_MULTICOLUMN)
++result;
return result;
}
@@ -1394,7 +1394,7 @@ Tabular::idx_type Tabular::getFirstCellInRow(row_type
row) const
// is really invalid, i.e., it is NOT the first cell in the row. but
// i do not know what to do here. (rgh)
while (c < numcells - 1
- && cell_info[row][c].multirow == CELL_PART_OF_MULTIROW)
+ && cell_info[row][c].multirow == CELL_HIDDEN_BY_MULTIROW)
++c;
return cell_info[row][c].cellno;
}
@@ -1407,8 +1407,8 @@ Tabular::idx_type Tabular::getLastCellInRow(row_type row)
const
// problem as in the previous routine: if all the cells are part of a
// multirow or part of a multi column, then our return value is invalid.
while (c > 0
- && (cell_info[row][c].multirow == CELL_PART_OF_MULTIROW
- || cell_info[row][c].multicolumn ==
CELL_PART_OF_MULTICOLUMN))
+ && (cell_info[row][c].multirow == CELL_HIDDEN_BY_MULTIROW
+ || cell_info[row][c].multicolumn ==
CELL_HIDDEN_BY_MULTICOLUMN))
--c;
return cell_info[row][c].cellno;
}
@@ -1654,7 +1654,7 @@ void Tabular::read(Lexer & lex)
bool Tabular::isMultiColumn(idx_type cell) const
{
return (cellInfo(cell).multicolumn == CELL_BEGIN_OF_MULTICOLUMN
- || cellInfo(cell).multicolumn == CELL_PART_OF_MULTICOLUMN);
+ || cellInfo(cell).multicolumn == CELL_HIDDEN_BY_MULTICOLUMN);
}
@@ -1683,7 +1683,7 @@ Tabular::idx_type Tabular::setMultiColumn(idx_type cell,
idx_type number,
for (idx_type i = 1; i < number; ++i) {
CellData & cs1 = cellInfo(cell + i);
- cs1.multicolumn = CELL_PART_OF_MULTICOLUMN;
+ cs1.multicolumn = CELL_HIDDEN_BY_MULTICOLUMN;
cs.inset->appendParagraphs(cs1.inset->paragraphs());
cs1.inset->clear();
}
@@ -1695,7 +1695,7 @@ Tabular::idx_type Tabular::setMultiColumn(idx_type cell,
idx_type number,
bool Tabular::isMultiRow(idx_type cell) const
{
return (cellInfo(cell).multirow == CELL_BEGIN_OF_MULTIROW
- || cellInfo(cell).multirow == CELL_PART_OF_MULTIROW);
+ || cellInfo(cell).multirow == CELL_HIDDEN_BY_MULTIROW);
}
bool Tabular::hasMultiRow(row_type r) const
@@ -1734,7 +1734,7 @@ Tabular::idx_type Tabular::setMultiRow(idx_type cell,
idx_type number,
for (idx_type i = 1; i < number; ++i) {
CellData & cs1 = cell_info[row + i][col];
- cs1.multirow = CELL_PART_OF_MULTIROW;
+ cs1.multirow = CELL_HIDDEN_BY_MULTIROW;
cs.inset->appendParagraphs(cs1.inset->paragraphs());
cs1.inset->clear();
}
@@ -1748,7 +1748,7 @@ Tabular::idx_type Tabular::columnSpan(idx_type cell) const
row_type const row = cellRow(cell);
col_type const col = cellColumn(cell);
int span = 1;
- while (col + span < ncols() && isPartOfMultiColumn(row, col + span))
+ while (col + span < ncols() && isHiddenByMultiColumn(row, col + span))
++span;
return span;
@@ -1759,7 +1759,7 @@ Tabular::idx_type Tabular::rowSpan(idx_type cell) const
{
col_type const column = cellColumn(cell);
col_type row = cellRow(cell) + 1;
- while (row < nrows() && isPartOfMultiRow(row, column))
+ while (row < nrows() && isHiddenByMultiRow(row, column))
++row;
return row - cellRow(cell);
@@ -1844,7 +1844,7 @@ Tabular::idx_type Tabular::cellAbove(idx_type cell) const
col_type const col = cellColumn(cell);
row_type r = cellRow(cell) - 1;
- while (r > 0 && cell_info[r][col].multirow == CELL_PART_OF_MULTIROW)
+ while (r > 0 && cell_info[r][col].multirow == CELL_HIDDEN_BY_MULTIROW)
--r;
return cell_info[r][col].cellno;
@@ -2116,19 +2116,19 @@ int Tabular::height() const
}
-bool Tabular::isPartOfMultiColumn(row_type row, col_type column) const
+bool Tabular::isHiddenByMultiColumn(row_type row, col_type column) const
{
LASSERT(row < nrows(), /**/);
LASSERT(column < ncols(), /**/);
- return cell_info[row][column].multicolumn == CELL_PART_OF_MULTICOLUMN;
+ return cell_info[row][column].multicolumn == CELL_HIDDEN_BY_MULTICOLUMN;
}
-bool Tabular::isPartOfMultiRow(row_type row, col_type column) const
+bool Tabular::isHiddenByMultiRow(row_type row, col_type column) const
{
LASSERT(row < nrows(), /**/);
LASSERT(column < ncols(), /**/);
- return cell_info[row][column].multirow == CELL_PART_OF_MULTIROW;
+ return cell_info[row][column].multirow == CELL_HIDDEN_BY_MULTIROW;
}
@@ -2538,12 +2538,12 @@ void Tabular::TeXRow(otexstream & os, row_type row,
bool ismulticol = false;
bool ismultirow = false;
for (col_type c = 0; c < ncols(); ++c) {
- if (isPartOfMultiColumn(row, c))
+ if (isHiddenByMultiColumn(row, c))
continue;
cell = cellIndex(row, c);
- if (isPartOfMultiRow(row, c)
+ if (isHiddenByMultiRow(row, c)
&& column_info[c].alignment != LYX_ALIGN_DECIMAL) {
if (cell != getLastCellInRow(row))
os << " & ";
@@ -2598,7 +2598,7 @@ void Tabular::TeXRow(otexstream & os, row_type row,
tail.setMacrocontextPositionRecursive(dit);
tail.latex(os, newrp);
}
- } else if (!isPartOfMultiRow(row, c)) {
+ } else if (!isHiddenByMultiRow(row, c)) {
if (!runparams.nice)
os.texrow().start(par.id(), 0);
inset->latex(os, newrp);
@@ -2802,7 +2802,7 @@ int Tabular::docbookRow(odocstream & os, row_type row,
os << "<row>\n";
for (col_type c = 0; c < ncols(); ++c) {
- if (isPartOfMultiColumn(row, c))
+ if (isHiddenByMultiColumn(row, c))
continue;
os << "<entry align=\"";
@@ -2954,7 +2954,7 @@ docstring Tabular::xhtmlRow(XHTMLStream & xs, row_type
row,
xs << html::StartTag("tr");
for (col_type c = 0; c < ncols(); ++c) {
- if (isPartOfMultiColumn(row, c) || isPartOfMultiRow(row, c))
+ if (isHiddenByMultiColumn(row, c) || isHiddenByMultiRow(row, c))
continue;
stringstream attr;
@@ -3112,7 +3112,7 @@ bool Tabular::plaintextTopHLine(odocstream & os, row_type
row,
col_type column = cellColumn(i);
int len = clen[column];
while (column < ncols() - 1
- && isPartOfMultiColumn(row, ++column))
+ && isHiddenByMultiColumn(row, ++column))
len += clen[column] + 4;
os << docstring(len, ch);
if (topLine(i)) {
@@ -3159,7 +3159,7 @@ bool Tabular::plaintextBottomHLine(odocstream & os,
row_type row,
}
col_type column = cellColumn(i);
int len = clen[column];
- while (column < ncols() - 1 && isPartOfMultiColumn(row,
++column))
+ while (column < ncols() - 1 && isHiddenByMultiColumn(row,
++column))
len += clen[column] + 4;
os << docstring(len, ch);
if (bottomLine(i)) {
@@ -3197,7 +3197,7 @@ void Tabular::plaintextPrintCell(odocstream & os,
unsigned int len1 = sstr.str().length();
unsigned int len2 = clen[column];
- while (column < ncols() - 1 && isPartOfMultiColumn(row, ++column))
+ while (column < ncols() - 1 && isHiddenByMultiColumn(row, ++column))
len2 += clen[column] + 4;
len2 -= len1;
@@ -3269,7 +3269,7 @@ void Tabular::plaintext(odocstream & os,
if (!onlydata && plaintextTopHLine(os, r, clen))
os << docstring(depth * 2, ' ');
for (col_type c = 0; c < ncols(); ++c) {
- if (isPartOfMultiColumn(r, c) || isPartOfMultiRow(r,c))
+ if (isHiddenByMultiColumn(r, c) ||
isHiddenByMultiRow(r,c))
continue;
if (onlydata && c > 0)
// we don't use operator<< for single UCS4
character.
@@ -3549,8 +3549,8 @@ void InsetTabular::metrics(MetricsInfo & mi, Dimension &
dim) const
int maxasc = 0;
int maxdes = 0;
for (col_type c = 0; c < tabular.ncols(); ++c) {
- if (tabular.isPartOfMultiColumn(r, c)
- || tabular.isPartOfMultiRow(r, c))
+ if (tabular.isHiddenByMultiColumn(r, c)
+ || tabular.isHiddenByMultiRow(r, c))
// multicolumn or multirow cell, but not first
one
continue;
idx_type const cell = tabular.cellIndex(r, c);
@@ -3701,12 +3701,12 @@ void InsetTabular::draw(PainterInfo & pi, int x, int y)
const
for (row_type r = 0; r < tabular.nrows(); ++r) {
int nx = x;
for (col_type c = 0; c < tabular.ncols(); ++c) {
- if (tabular.isPartOfMultiColumn(r, c))
+ if (tabular.isHiddenByMultiColumn(r, c))
continue;
idx = tabular.cellIndex(r, c);
- if (tabular.isPartOfMultiRow(r, c)) {
+ if (tabular.isHiddenByMultiRow(r, c)) {
nx += tabular.cellWidth(idx);
continue;
}
@@ -3762,12 +3762,12 @@ void InsetTabular::drawSelection(PainterInfo & pi, int
x, int y) const
for (row_type r = 0; r < tabular.nrows(); ++r) {
int xx = x;
for (col_type c = 0; c < tabular.ncols(); ++c) {
- if (tabular.isPartOfMultiColumn(r, c))
+ if (tabular.isHiddenByMultiColumn(r, c))
continue;
idx_type const cell = tabular.cellIndex(r, c);
- if (tabular.isPartOfMultiRow(r, c)) {
+ if (tabular.isHiddenByMultiRow(r, c)) {
xx += tabular.cellWidth(cell);
continue;
}
@@ -5596,7 +5596,7 @@ void InsetTabular::tabularFeatures(Cursor & cur,
for (col_type c = sel_col_start; c <= sel_col_end; ++c) {
row_type const r = sel_row_start;
if (!tabular.isMultiColumn(tabular.cellIndex(r, c))
- || (r > sel_row_start &&
!tabular.isPartOfMultiColumn(r, c)))
+ || (r > sel_row_start &&
!tabular.isHiddenByMultiColumn(r, c)))
merge = true;
}
// If the selection contains at least one singlecol cell
@@ -5651,7 +5651,7 @@ void InsetTabular::tabularFeatures(Cursor & cur,
for (row_type r = sel_row_start; r <= sel_row_end; ++r) {
col_type const c = sel_col_start;
if (!tabular.isMultiRow(tabular.cellIndex(r, c))
- || (r > sel_row_start &&
!tabular.isPartOfMultiRow(r, c)))
+ || (r > sel_row_start &&
!tabular.isHiddenByMultiRow(r, c)))
merge = true;
}
// If the selection contains at least one singlerow cell
@@ -5990,14 +5990,14 @@ bool InsetTabular::pasteClipboard(Cursor & cur)
for (col_type c1 = 0, c2 = actcol;
c1 < paste_tabular->ncols() && c2 < tabular.ncols();
++c1, ++c2) {
- if (paste_tabular->isPartOfMultiColumn(r1, c1) &&
- tabular.isPartOfMultiColumn(r2, c2))
+ if (paste_tabular->isHiddenByMultiColumn(r1, c1) &&
+ tabular.isHiddenByMultiColumn(r2, c2))
continue;
- if (paste_tabular->isPartOfMultiColumn(r1, c1)) {
+ if (paste_tabular->isHiddenByMultiColumn(r1, c1)) {
--c2;
continue;
}
- if (tabular.isPartOfMultiColumn(r2, c2)) {
+ if (tabular.isHiddenByMultiColumn(r2, c2)) {
--c1;
continue;
}
diff --git a/src/insets/InsetTabular.h b/src/insets/InsetTabular.h
index 8affb9a..10acb51 100644
--- a/src/insets/InsetTabular.h
+++ b/src/insets/InsetTabular.h
@@ -286,11 +286,11 @@ public:
///
CELL_BEGIN_OF_MULTICOLUMN,
///
- CELL_PART_OF_MULTICOLUMN,
+ CELL_HIDDEN_BY_MULTICOLUMN,
///
CELL_BEGIN_OF_MULTIROW,
///
- CELL_PART_OF_MULTIROW
+ CELL_HIDDEN_BY_MULTIROW
};
///
@@ -499,9 +499,9 @@ public:
///
void unsetMultiColumn(idx_type cell);
///
- bool isPartOfMultiColumn(row_type row, col_type column) const;
+ bool isHiddenByMultiColumn(row_type row, col_type column) const;
///
- bool isPartOfMultiRow(row_type row, col_type column) const;
+ bool isHiddenByMultiRow(row_type row, col_type column) const;
///
bool isMultiRow(idx_type cell) const;
///
--
1.7.9.5
