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?
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.
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 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.
- My same comment as above but for isPartOfMultiColumn.
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?
Thanks,
Scott
From 3cd8fb3d15347869b3cbf6949348746b97381ac6 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Fri, 7 Dec 2012 00:33:39 -0500
Subject: [PATCH 1/9] Remove an unnecessary condition in InsetTabular
cur.selIsMultiCell implies cur.selection so if we are conditioning
on cur.selIsMultiCell we do not need to condition on cur.selection.
---
src/insets/InsetTabular.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index 6bdf72b..bfc678d 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -4061,7 +4061,7 @@ void InsetTabular::doDispatch(Cursor & cur, FuncRequest &
cmd)
case LFUN_DOWN_SELECT:
case LFUN_DOWN:
- if (!(cur.selection() && cur.selIsMultiCell()))
+ if (!(cur.selIsMultiCell()))
cell(cur.idx())->dispatch(cur, cmd);
cur.dispatched(); // override the cell's decision
@@ -4100,7 +4100,7 @@ void InsetTabular::doDispatch(Cursor & cur, FuncRequest &
cmd)
case LFUN_UP_SELECT:
case LFUN_UP:
- if (!(cur.selection() && cur.selIsMultiCell()))
+ if (!(cur.selIsMultiCell()))
cell(cur.idx())->dispatch(cur, cmd);
cur.dispatched(); // override the cell's decision
if (sl == cur.top()) {
--
1.7.9.5
From faae90ea2d3e18a218e865bdfdbba6f82ce0b45d Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Fri, 7 Dec 2012 00:48:47 -0500
Subject: [PATCH 2/9] Remove an extra space
---
src/insets/InsetTabular.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index bfc678d..c379903 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -4755,7 +4755,7 @@ bool InsetTabular::getStatus(Cursor & cur, FuncRequest
const & cmd,
getSelection(cur, rs, re, cs, ce);
if (paste_tabular && paste_tabular->ncols() ==
ce - cs + 1
&& paste_tabular->nrows() == re - rs
+ 1)
- status.setEnabled(true);
+ status.setEnabled(true);
else {
status.setEnabled(false);
status.message(_("Selection size should
match clipboard content."));
--
1.7.9.5
From 5dc05260a2689aafca42df5309ae1ebb93f7efb3 Mon Sep 17 00:00:00 2001
From: Scott Kostyshak <skost...@lyx.org>
Date: Fri, 7 Dec 2012 01:07:11 -0500
Subject: [PATCH 3/9] In InsetTabular, rename copy-row to duplicate-row
Also rename copy-column to duplicate-column.
Rename the LFUNs and functions accordingly.
---
src/insets/InsetTabular.cpp | 20 ++++++++++----------
src/insets/InsetTabular.h | 8 ++++----
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/insets/InsetTabular.cpp b/src/insets/InsetTabular.cpp
index c379903..4fe84f4 100644
--- a/src/insets/InsetTabular.cpp
+++ b/src/insets/InsetTabular.cpp
@@ -112,8 +112,8 @@ TabularFeature tabularFeature[] =
{ Tabular::APPEND_COLUMN, "append-column", false },
{ Tabular::DELETE_ROW, "delete-row", false },
{ Tabular::DELETE_COLUMN, "delete-column", false },
- { Tabular::COPY_ROW, "copy-row", false },
- { Tabular::COPY_COLUMN, "copy-column", false },
+ { Tabular::DUPLICATE_ROW, "duplicate-row", false },
+ { Tabular::DUPLICATE_COLUMN, "duplicate-column", false },
{ Tabular::SET_LINE_TOP, "set-line-top", true },
{ Tabular::SET_LINE_BOTTOM, "set-line-bottom", true },
{ Tabular::SET_LINE_LEFT, "set-line-left", true },
@@ -721,7 +721,7 @@ void Tabular::deleteRow(row_type const row)
}
-void Tabular::copyRow(row_type const row)
+void Tabular::duplicateRow(row_type const row)
{
insertRow(row, true);
}
@@ -789,7 +789,7 @@ void Tabular::deleteColumn(col_type const col)
}
-void Tabular::copyColumn(col_type const col)
+void Tabular::duplicateColumn(col_type const col)
{
insertColumn(col, true);
}
@@ -4395,8 +4395,8 @@ bool InsetTabular::getStatus(Cursor & cur, FuncRequest
const & cmd,
case Tabular::APPEND_COLUMN:
case Tabular::DELETE_ROW:
case Tabular::DELETE_COLUMN:
- case Tabular::COPY_ROW:
- case Tabular::COPY_COLUMN:
+ case Tabular::DUPLICATE_ROW:
+ case Tabular::DUPLICATE_COLUMN:
case Tabular::SET_TOP_SPACE:
case Tabular::SET_BOTTOM_SPACE:
case Tabular::SET_INTERLINE_SPACE:
@@ -5331,12 +5331,12 @@ void InsetTabular::tabularFeatures(Cursor & cur,
cur.setSelection(false);
break;
- case Tabular::COPY_ROW:
- tabular.copyRow(row);
+ case Tabular::DUPLICATE_ROW:
+ tabular.duplicateRow(row);
break;
- case Tabular::COPY_COLUMN:
- tabular.copyColumn(column);
+ case Tabular::DUPLICATE_COLUMN:
+ tabular.duplicateColumn(column);
cur.idx() = tabular.cellIndex(row, column);
break;
diff --git a/src/insets/InsetTabular.h b/src/insets/InsetTabular.h
index 80958cc..dd79a8f 100644
--- a/src/insets/InsetTabular.h
+++ b/src/insets/InsetTabular.h
@@ -132,9 +132,9 @@ public:
///
DELETE_COLUMN,
///
- COPY_ROW,
+ DUPLICATE_ROW,
///
- COPY_COLUMN,
+ DUPLICATE_COLUMN,
///
SET_LINE_TOP,
///
@@ -449,7 +449,7 @@ public:
///
void deleteRow(row_type row);
///
- void copyRow(row_type row);
+ void duplicateRow(row_type row);
///
void insertRow(row_type row, bool copy);
///
@@ -457,7 +457,7 @@ public:
///
void deleteColumn(col_type column);
///
- void copyColumn(col_type column);
+ void duplicateColumn(col_type column);
///
void insertColumn(col_type column, bool copy);
///
--
1.7.9.5
