Hi William, On Wednesday, 2008-07-30 17:22:43 +0100, William S Fulton wrote:
> I have been working on adding multiline formulae support and have
> attached a patch to bug #35913. It also fixes bug #83666. A quick
> summary of what it adds is the display of new lines when the result of a
> formula contains a newline character. Previously, the newline characters
> were filtered away and converted into spaces.
That's indeed a nice patch, thanks.
Some nitpick though ;-)
Method ScFormulaCell::IsMultilineResult() unconditionally obtains
a string result, which in case the result is not string returns an empty
string so would be ok, and then searches for a newline character. In
case the result was a string this would have some performance penalty,
as for every access this would had to be done again. In case the result
was not a string, obtaining a string result and search on the empty
string would be superfluous.
Instead, I propose to introduce a method ScFormulaResult::IsMultiline()
and a tristate (unknown,yes,no) flag in ScFormulaResult, execute the
logic only if the flag says unknown and the result is string, and store
the state in the flag. When at ScFormulaResult a string result or matrix
with a string at its top left is set, set the flag to unknown. For all
other results set it to no, except in the copying Assign() method of
course where it should be copied instead. Then using
class ScFormulaResult
{
...
enum Multiline
{
MULTILINE_UNKNOWN = 0,
MULTILINE_YES,
MULTILINE_NO
};
...
Multiline meMultiline : 2;
...
};
bool ScFormulaResult::IsMultiline() const
{
if (meMultiline == MULTILINE_UNKNOWN)
{
const String& rStr = GetString();
if (rStr.Len() && rStr.Search( _LF ) != STRING_NOTFOUND)
meMultiline = MULTILINE_YES;
else
meMultiline = MULTILINE_NO;
}
return meMultiline == MULTILINE_YES;
}
should catch all. Written from scratch and untested..
Method ScFormulaCell::IsMultilineResult() then should simply be
if (!IsValue())
return aResult.IsMultiline();
return false;
The IsValue() call here also assures that the cell is recalculated if
needed.
Moving the detection to ScFormulaResult has another advantage that it
can be used independently of ScFormulaCell.
> Some notable changes that the patch adds is:
> [... nice list snipped ...]
>
> Some potential issues:
> - Compatibility with Excel possibly needs discussing. We've not been
> compatible with Excel in the past and this patch does not make it 100%
> compatible, rather it improves compatibility, and improves further where
> Excel gets it wrong IMHO.
I really don't have any problem with Calc being superior to Excel at
some places ;-) But being different here may result in documents being
displayed with different row heights when imported, something that
sometimes makes users act strangely..
> I've chosen to show the newlines by default,
> whereas this is impossible in Excel unless the "Wrap text" option is
> ticked in the Format Cells dialog. To me the Excel behaviour is
> incorrect as the option turns on newline display as well as wrapping the
> contents, so it is doing something more than what it advertises - it
> mixes formatting with content display.
> The behaviour in Calc showing new
> lines is now consistent between ScFormulaCell and ScEditCell.
For interoperability I think we should follow the Excel approach and
only display multiline if the wrap option is set. Note that Excel sets
it automatically for a non-formula cell if that contains a linefeed. If
we wanted to automatically set the row height later we could introduce
an option; doing it the opposite way, display multiline now and turn it
off later, would be harder from a user experience's perspective.
> - Spreadsheets created in OpenOffice.org 2 *might* look different *if*
> the result of a formula contains a newline.
Same as above for Excel import. Having a newline contained and wrap
option set in older documents should be rare. A slight difference would
be in string width, as an "inline" newline is displayed with zero width
whereas a space was, well, a space.
> - Performance. The patch could be made more performant by searching for
> '\n' just once in the formula result and caching the result somewhere,
> see the new method ScFormulaCell::IsMultilineResult(). I'm not sure if
> this is really necessary though.
See above ;-)
> Everything else that I can see just works as you'd hope, for example
> matrix cells as well as "Shrink to fit cell size", "Wrap text
> automatically" in the Alignment tab of the Format Cells properties
> dialog. This is largely because the patch simply uses the edit engine to
> display the formula result if (and only if) the formula result contain
> multiple lines.
> I havn't yet figured out the Calc regression testing to make sure that
> nothing else is broken.
Special care must be taken of places that directly or indirectly call
ScCellFormat::GetString() because those now may have to digest a newline
that they didn't have to before. Same for ScFormulaCell::GetString() and
ScEditCell::GetString(), the latter being the most invasive change of
your patch. This has to be carefully checked and might make it necessary
to introduce another method ScEditCell::GetMultilineString() to be used
in formulas and not touch ScEditCell::GetString(). Places that still
obtain a string via ScEditUtil::GetSpaceDelimitedString() now receive
a string that is different from ScEditCell::GetString(). Check for side
effects. Printing should be tested as well.
> If it is easy for someone to run the tests with
> the patch or point me in the right direction that would be much
> appreciated.
The QA project has a bunch of tests they run on a CWS or on special
occasions. The tests reside in module qadevOOo, for Calc that would be
those in qadevOOo/tests/basic/mod/sc, the [EMAIL PROTECTED] mailing list should
be
able to provide details. Some differences in this case may be only
visually detectable though, due to different row heights and string
widths.
> I've been developing and testing under Linux and have also
> sent off my signed SCA.
Fine. Looks like the "copyright-approved" page is outdated, last
updated 2008-06-03 ...
Thanks
Eike
--
OOo/SO Calc core developer. Number formatter stricken i18n transpositionizer.
SunSign 0x87F8D412 : 2F58 5236 DB02 F335 8304 7D6C 65C9 F9B5 87F8 D412
OpenOffice.org Engineering at Sun: http://blogs.sun.com/GullFOSS
Please don't send personal mail to the [EMAIL PROTECTED] account, which I use
for
mailing lists only and don't read from outside Sun. Use [EMAIL PROTECTED]
Thanks.
pgpym6205TiVP.pgp
Description: PGP signature
