Op 22-12-2012 7:05, Scott Kostyshak schreef:
On Fri, Dec 21, 2012 at 10:07 AM, Richard Heck <rgh...@lyx.org> wrote:
On 12/21/2012 03:11 AM, Scott Kostyshak wrote:
Wrong patch.
Sorry about that. Correct patch attached.
Thanks, Scott
diff --git a/lib/bind/cua.bind b/lib/bind/cua.bind
index 4d6c286..44bfcf6 100644
--- a/lib/bind/cua.bind
+++ b/lib/bind/cua.bind
@@ -134,6 +134,8 @@ Format 1
\bind "M-Up" "paragraph-move-up"
\bind "M-Down" "paragraph-move-down"
Why not e.g., "command-alternatives paragraph-move-up; inset-modify
tabular move-row-up" ?
In this way, you can leave paragraph-move-up/down untouched.
diff --git a/src/LyXAction.cpp b/src/LyXAction.cpp
index 68ce15d..5289a31 100644
--- a/src/LyXAction.cpp
+++ b/src/LyXAction.cpp
@@ -1931,6 +1931,7 @@ void LyXAction::init()
/*!
* \var lyx::FuncCode lyx::LFUN_PARAGRAPH_MOVE_DOWN
* \li Action: Moves the current paragraph downwards in the document.
+ If in a table, move the current row downwards.
* \li Syntax: paragraph-move-down
* \li Origin: Edwin, 8 Apr 2006
* \endvar
This special case would not be necessary.
@@ -769,6 +774,125 @@ void Tabular::insertRow(row_type const row, bool
copy)
}
+void Tabular::moveColumn(col_type col, Tabular::ColDirection const
direction)
+{
+ if (direction == Tabular::LEFT)
+ col = col -1;
+
+ swapColumns(col, col + 1);
+ updateIndexes();
+
+ // Note that the names refer to the positions after the swap.
+ map< pair<row_type, col_type>, pair<bool, bool> > cMap;
+ map< pair<row_type, col_type>, pair<bool, bool> > cplus1Map;
Style: local variables are written as c_map.
Why not something like col_lines and next_col_lines ? This is somewhat
clearer to me as I like the variable name to describe the contents
rather than the type of the variable.
+
+ // Could potentially do everything in one loop, but would have to be
+ // careful with multi-row cells. Currently the first loop reads the
+ // borders and the second loop writes them.
+ for (row_type r = 0; r < nrows(); ++r) {
+
+ // Cells part of a multi-row will write to the same idx.
+ {
+ idx_type const i = cellIndex(r, col);
+ cMap[make_pair(r, col)] =
+ make_pair(leftLine(i), rightLine(i));
+ }
+ {
+ idx_type const j = cellIndex(r, col + 1);
+ cplus1Map[make_pair(r, col + 1)] =
+ make_pair(leftLine(j), rightLine(j));
+ }
I thought you would get rid of these (unnecessary) brackets ?
+ }
+
+ for (row_type r = 0; r < nrows(); ++r) {
+ idx_type const i = cellIndex(r, col);
+ idx_type const j = cellIndex(r, col + 1);
+
+ pair<bool, bool> const j_lines =
+ cplus1Map[make_pair(r, col + 1)];
+ setLeftLine(i, j_lines.first);
+ setRightLine(i, j_lines.second);
+
+ pair<bool, bool> const i_lines =
+ cMap[make_pair(r, col)];
+ setLeftLine(j, i_lines.first);
+ setRightLine(j, i_lines.second);
+
+ if (buffer().params().trackChanges) {
+ cellInfo(i).inset->setChange(Change(Change::INSERTED));
+ cellInfo(j).inset->setChange(Change(Change::INSERTED));
+ }
+ }
+}
This looks like a lot of code just to make sure that the lines are set
correctly. I did not look good enough to see whether I see a better
solution.
+
+
+void Tabular::swapColumns(col_type const & c1, col_type const & c2)
No "const &" here.
+{
+ for (row_type r = 0; r < nrows(); ++r) {
+ std::swap(cell_info[r][c1], cell_info[r][c2]);
+ }
+}
+
+
+void Tabular::moveRow(row_type row, Tabular::RowDirection const
direction)
No "const" here.
+{
+ if (direction == Tabular::UP)
+ row = row - 1;
+
+ swapRows(row, row + 1);
+
+ updateIndexes();
+
+ // Note that the names refer to the positions after the swap.
+ map< pair<row_type, col_type>, pair<bool, bool> > rMap;
+ map< pair<row_type, col_type>, pair<bool, bool> > rplus1Map;
+
+ // Could potentially do everything in one loop, but would have to be
+ // careful with multi-column cells. Currently the first loop
reads the
+ // borders and the second loop writes them.
+ for (col_type c = 0; c < ncols(); ++c) {
+
+ // Cells part of a multi-column will write to the same idx.
+ {
+ idx_type const i = cellIndex(row, c);
+ rMap[make_pair(row, c)] =
+ make_pair(topLine(i), bottomLine(i));
+ }
+ {
+ idx_type const j = cellIndex(row + 1, c);
+ rplus1Map[make_pair(row + 1, c)] =
+ make_pair(topLine(j), bottomLine(j));
+ }
+ }
+
+ for (col_type c = 0; c < ncols(); ++c) {
+ idx_type const i = cellIndex(row, c);
+ idx_type const j = cellIndex(row + 1, c);
+
+ pair<bool, bool> const j_lines =
+ rplus1Map[make_pair(row + 1, c)];
+ setTopLine(i, j_lines.first);
+ setBottomLine(i, j_lines.second);
+
+ pair<bool, bool> const i_lines =
+ rMap[make_pair(row, c)];
+ setTopLine(j, i_lines.first);
+ setBottomLine(j, i_lines.second);
+
+ if (buffer().params().trackChanges) {
+ cellInfo(i).inset->setChange(Change(Change::INSERTED));
+ cellInfo(j).inset->setChange(Change(Change::INSERTED));
+ }
+ }
+}
Yet again a lot of similar code :(.
+
+
+void Tabular::swapRows(row_type const & r1, row_type const & r2)
No "const &".
+{
+ std::swap(cell_info[r1], cell_info[r2]);
+}
+
+
void Tabular::deleteColumn(col_type const col)
{
// Not allowed to delete last column
@@ -1603,6 +1727,16 @@ bool Tabular::isMultiColumn(idx_type cell) const
}
+bool Tabular::hasMultiColumn(col_type c) const
+{
+ for (row_type r = 0; r < nrows(); ++r) {
+ if(isMultiColumn(cellIndex(r, c)))
+ return(true);
+ }
+ return(false);
+}
Style: if(..) -> if (..)
Style: return(true) -> return true.
+
+
Tabular::CellData & Tabular::cellInfo(idx_type cell) const
{
return cell_info[cellRow(cell)][cellColumn(cell)];
@@ -1643,6 +1777,14 @@ bool Tabular::isMultiRow(idx_type cell) const
|| cellInfo(cell).multirow == CELL_PART_OF_MULTIROW);
}
+bool Tabular::hasMultiRow(row_type r) const
+{
+ for (col_type c = 0; c < ncols(); ++c) {
+ if(isMultiRow(cellIndex(r, c)))
+ return(true);
+ }
+ return(false);
+}
Same as above.
Tabular::idx_type Tabular::setMultiRow(idx_type cell, idx_type number,
bool const bottom_border)
@@ -4174,6 +4316,14 @@ void InsetTabular::doDispatch(Cursor & cur,
FuncRequest & cmd)
// break;
// }
+ case LFUN_PARAGRAPH_MOVE_DOWN:
+ tabularFeatures(cur, Tabular::MOVE_ROW_DOWN);
+ break;
+
+ case LFUN_PARAGRAPH_MOVE_UP:
+ tabularFeatures(cur, Tabular::MOVE_ROW_UP);
+ break;
+
case LFUN_LAYOUT_TABULAR:
cur.bv().showDialog("tabular");
break;
As written above, I would get rid of this.
@@ -4410,6 +4560,84 @@ bool InsetTabular::getStatus(Cursor & cur,
FuncRequest const & cmd,
status.setEnabled(!tabular.rotate && !tabular.is_long_tabular
&& tabular.tabular_valignment ==
Tabular::LYX_VALIGN_MIDDLE);
break;
+ case Tabular::MOVE_COLUMN_RIGHT:
+ if (cur.selection()) {
+ status.message(_("Selections not supported."));
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.ncols() == tabular.cellColumn(cur.idx()) + 1) {
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.hasMultiColumn(tabular.cellColumn(cur.idx())) ||
+ tabular.hasMultiColumn(tabular.cellColumn(cur.idx()) + 1)) {
+ status.message(_("Multi-column in current or"
+ " destination column."));
+ status.setEnabled(false);
+ break;
+ }
+ status.setEnabled(true);
+ break;
+ case Tabular::MOVE_COLUMN_LEFT:
+ if (cur.selection()) {
+ status.message(_("Selections not supported."));
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.cellColumn(cur.idx()) == 0) {
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.hasMultiColumn(tabular.cellColumn(cur.idx())) ||
+ tabular.hasMultiColumn(tabular.cellColumn(cur.idx()) - 1)) {
+ status.message(_("Multi-column in current or"
+ " destination column."));
+ status.setEnabled(false);
+ break;
+ }
+ status.setEnabled(true);
+ break;
+
+ case Tabular::MOVE_ROW_DOWN:
+ if (cur.selection()) {
+ status.message(_("Selections not supported."));
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.nrows() == tabular.cellRow(cur.idx()) + 1) {
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.hasMultiRow(tabular.cellRow(cur.idx())) ||
+ tabular.hasMultiRow(tabular.cellRow(cur.idx()) + 1)) {
+ status.message(_("Multi-row in current or"
+ " destination row."));
+ status.setEnabled(false);
+ break;
+ }
+ status.setEnabled(true);
+ break;
+
+ case Tabular::MOVE_ROW_UP:
+ if (cur.selection()) {
+ status.message(_("Selections not supported."));
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.cellRow(cur.idx()) == 0) {
+ status.setEnabled(false);
+ break;
+ }
+ if (tabular.hasMultiRow(tabular.cellRow(cur.idx())) ||
+ tabular.hasMultiRow(tabular.cellRow(cur.idx()) - 1)) {
+ status.message(_("Multi-row in current or"
+ " destination row."));
+ status.setEnabled(false);
+ break;
+ }
+ status.setEnabled(true);
+ break;
This looks like 4 times the same piece of code. There must be a
possibility to shorten this code.
case Tabular::SET_DECIMAL_POINT:
status.setEnabled(
@@ -4731,6 +4959,12 @@ bool InsetTabular::getStatus(Cursor & cur,
FuncRequest const & cmd,
return cell(cur.idx())->getStatus(cur, cmd, status);
}
+ case LFUN_PARAGRAPH_MOVE_DOWN:
+ return getStatus(cur, FuncRequest(LFUN_INSET_MODIFY, "tabular
move-row-down"), status);
+
+ case LFUN_PARAGRAPH_MOVE_UP:
+ return getStatus(cur, FuncRequest(LFUN_INSET_MODIFY, "tabular
move-row-up"), status);
+
I would get rid of these (as written above).
// disable in non-fixed-width cells
case LFUN_PARAGRAPH_BREAK:
// multirow does not allow paragraph breaks
@@ -5361,6 +5595,26 @@ void InsetTabular::tabularFeatures(Cursor & cur,
cur.idx() = tabular.cellIndex(row, column);
break;
+ case Tabular::MOVE_COLUMN_RIGHT:
+ tabular.moveColumn(column, Tabular::RIGHT);
+ cur.idx() = tabular.cellIndex(row, column + 1);
+ break;
+
+ case Tabular::MOVE_COLUMN_LEFT:
+ tabular.moveColumn(column, Tabular::LEFT);
+ cur.idx() = tabular.cellIndex(row, column - 1);
+ break;
+
+ case Tabular::MOVE_ROW_DOWN:
+ tabular.moveRow(row, Tabular::DOWN);
+ cur.idx() = tabular.cellIndex(row + 1, column);
+ break;
+
+ case Tabular::MOVE_ROW_UP:
+ tabular.moveRow(row, Tabular::UP);
+ cur.idx() = tabular.cellIndex(row - 1, column);
+ break;
+
case Tabular::SET_LINE_TOP:
case Tabular::TOGGLE_LINE_TOP: {
bool lineSet = (feature == Tabular::SET_LINE_TOP)
@@ -5436,7 +5690,7 @@ void InsetTabular::tabularFeatures(Cursor & cur,
tabular.rightLine(cur.idx()));
break;
}
- // we have a selection so this means we just add all this
+ // we have a selection so this means we just add all these
// cells to form a multicolumn cell
idx_type const s_start = cur.selBegin().idx();
row_type const col_start = tabular.cellColumn(s_start);
diff --git a/src/insets/InsetTabular.h b/src/insets/InsetTabular.h
index 80958cc..5dc0b3e 100644
--- a/src/insets/InsetTabular.h
+++ b/src/insets/InsetTabular.h
@@ -10,6 +10,7 @@
* \author J眉rgen Vigna
* \author Edwin Leuven
* \author Uwe St枚hr
+ * \author Scott Kostyshak
*
* Full author contact details are available in file CREDITS.
*/
@@ -136,6 +137,14 @@ public:
///
COPY_COLUMN,
///
+ MOVE_COLUMN_RIGHT,
+ ///
+ MOVE_COLUMN_LEFT,
+ ///
+ MOVE_ROW_DOWN,
+ ///
+ MOVE_ROW_UP,
+ ///
SET_LINE_TOP,
///
SET_LINE_BOTTOM,
@@ -331,6 +340,16 @@ public:
CAPTION_ANY
};
+ enum RowDirection {
+ UP = 0,
+ DOWN
+ };
+
+ enum ColDirection {
+ RIGHT = 0,
+ LEFT
+ };
+
The "=0"s are unneeded. Unless one uses the values of the enum items
explicitly.
class ltType {
public:
// constructor
@@ -453,6 +472,14 @@ public:
///
void insertRow(row_type row, bool copy);
///
+ void moveColumn(col_type col, Tabular::ColDirection direction);
+ ///
+ void swapColumns(col_type const & c1, col_type const & c2);
+ ///
+ void moveRow(row_type row, Tabular::RowDirection direction);
+ ///
+ void swapRows(row_type const & r1, row_type const & r2);
+ ///
It is unneeded to pass the c1, c2, r1, and r2 as "const &".
I guess "Tabular::" is not needed.
void appendColumn(col_type column);
///
void deleteColumn(col_type column);
@@ -483,6 +510,8 @@ public:
///
bool isMultiColumn(idx_type cell) const;
///
+ bool hasMultiColumn(col_type cell) const;
+ ///
idx_type setMultiColumn(idx_type cell, idx_type number,
bool const right_border);
///
@@ -494,6 +523,8 @@ public:
///
bool isMultiRow(idx_type cell) const;
///
+ bool hasMultiRow(row_type r) const;
+ ///
idx_type setMultiRow(idx_type cell, idx_type number,
bool const bottom_border);
///
--
1.7.9.5
