Martin Vermeer <[EMAIL PROTECTED]> writes:
> Attached is a first attempt to streamline the handling of the
> various presentation modes of collapsable insets.
I think this is something that is very worth doing and the structure
of the patch is sound.
Some comments on the code follow.
JMarc
> + virtual Decoration decoration() const { return Conglomerate; }
> +
> + ///
These things could _maybe_ be moved to insetlayout.
> + case InlinedD:
> + return InlinedG;
> + break;
I do not like these InlinedD/InlinedG. Isn't it possible to use
Decoration::Inlined and Geometry::Inlined?
> + if (decoration() == InlinedD || decoration() == Conglomerate) {
> InsetText::metrics(mi, dim);
> } else {
> dim = dimensionCollapsed();
> - if (status() == Open) {
> + if (geometry() == OpenG || geometry() == OpenInlined) {
> InsetText::metrics(mi, textdim_);
> // This expression should not contain mi.base.texwidth
> openinlined_ = !hasFixedWidth()
I think in general it is better to use a switch, so that one sees
immediately where each case is handled:
switch(decoration()) {
case Classic:
...
case InlinedD;
case Conglomerate;
...
}
It is a good idea to avoid using a default: case for these cases where
the enum takes a few values. One adavantage is that, if you add
another value to the enum, the compiler will point to you all the
places that need to be updated.
> @@ -196,7 +212,7 @@
> void InsetCollapsable::draw(PainterInfo & pi, int x, int y) const
> {
> const int xx = x + TEXT_TO_INSET_OFFSET;
> - if (status() == Inlined) {
> + if (decoration() == InlinedD) {
> InsetText::draw(pi, xx, y);
> } else {
> Dimension dimc = dimensionCollapsed();
Should be a switch too. There are several others below.
> + if (geometry() == OpenInlined)
> + x += dimensionCollapsed().wid;
> + if (geometry() == OpenG)
> + y += dimensionCollapsed().des + textdim_.asc;
> + if (geometry() != CollapsedG)
> InsetText::drawSelection(pi, x, y);
Besides the fact that a switch is better, it also a good idea to use
"else if" in these exclusive cases, if only to help the reader to
follow the code.
