Le 08/12/2012 08:09, Scott Kostyshak a écrit :
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?
Yes, but then it would be clearer to rename selIsMultiCell to
multiCellSelection. Otherwise the code would look like we already know
that there is a selection.
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.
It you do that, you need to update the menus/toolbars and also the
scripts that update configuration files (prefs2prefs.py).
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?
Yes, it is 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.
Maybe isHiddenByMultiRow?
(1) What is the purpose of this status.clear()?
Is it a shortcut to indicate that the function is enabled without any
extra setting. I am not sure it is better that setEnabled(true) here.
(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 strongly prefer to avoid early returns in these functions, since this
allows to place common code at the end of the switch.
JMarc
