Re: [Libreoffice] [REVIEW] fix for fdo#39850 and fdo#39820: update range names and database ranges in formula cells

Hello Eike,

2011/8/16 Eike Rathke <ooo@erack.de>

> Hi Markus,
>
> On Tuesday, 2011-08-16 08:07:20 +0200, Markus Mohrhard wrote:
>
> > this patch might be a bit to big for 3-4, but who knows:
> http://cgit.freedesktop.org/libreoffice/core/commit/?id=04d2e6469529b6187900659517d6f6dd5ea2cca5
>
> First thing that caught my eye: using toAsciiUpperCase() (or any other
> ASCII method) on anything that the user inputs as names is a no-no as it
> doesn't handle Unicode. Use ScGlobal::pCharClass->upper() instead.

I think I inherite it from some other places that use range names. So then
we need to check for all other places too and adjust them accordingly.


> The change to the ScFormulaCell ctor apparently unconditionally tries to
> adjust name tokens, this is completely unnecessary in most situations
> and should be triggered only if the sheets of new and old position
> differ.

No it's not that easy. If we have the same sheet and two different
documents, we need to adjust too. And we can't simply check that the
ScDocument instances are different because they will always be. Either we
have a pointer to the original ScDocument instance in the copy document or
we must always adjust our range names. I used the second approach because I
didn't like the other approach.


> Before looping with pCode->GetNext...() it needs a pCode->Reset() first.

Thanks. I forgot that.


> In ScTable::CopyToClip() all local range names are copied if none exist
> yet on the target. Best would be if only names used by the formulas to
> be copied would be copied if not already there, which adjustRangeName
> attempts to do. So in CopyToClip initialize
> pTable->mpRangeName = new ScRangeName;

Yes, I know. And as I have written I plan to change this in master, at the
moment this wouldn't change our behaviour so I don't see a problem with
this.  I didn't want to introduce more changes than necessary into the
stable branch.


> Btw, please use 'r' prefix for parameters passed as reference, so
> rNewDoc instead of aNewDoc, and please "const as const can", for example
> it should be const ScDocument* pOldDoc
>
> Apart from that, I'm currently somewhat handicapped with equipment and
> can't check if the change really does what it is supposed to do.

Regards,
Markus