Hello Eike,<br><br><div class="gmail_quote">2011/8/16 Eike Rathke <span dir="ltr"><<a href="mailto:ooo@erack.de">ooo@erack.de</a>></span><br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Hi Markus,<br>
<div class="im"><br>
On Tuesday, 2011-08-16 08:07:20 +0200, Markus Mohrhard wrote:<br>
<br>
> this patch might be a bit to big for 3-4, but who knows:<br>
> <a href="http://cgit.freedesktop.org/libreoffice/core/commit/?id=04d2e6469529b6187900659517d6f6dd5ea2cca5" target="_blank">http://cgit.freedesktop.org/libreoffice/core/commit/?id=04d2e6469529b6187900659517d6f6dd5ea2cca5</a><br>
<br>
</div>First thing that caught my eye: using toAsciiUpperCase() (or any other<br>
ASCII method) on anything that the user inputs as names is a no-no as it<br>
doesn't handle Unicode. Use ScGlobal::pCharClass->upper() instead.<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
The change to the ScFormulaCell ctor apparently unconditionally tries to<br>
adjust name tokens, this is completely unnecessary in most situations<br>
and should be triggered only if the sheets of new and old position<br>
differ.<br></blockquote><div><br>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.<br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Before looping with pCode->GetNext...() it needs a pCode->Reset() first.<br></blockquote><div><br>Thanks. I forgot that. <br><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
In ScTable::CopyToClip() all local range names are copied if none exist<br>
yet on the target. Best would be if only names used by the formulas to<br>
be copied would be copied if not already there, which adjustRangeName<br>
attempts to do. So in CopyToClip initialize<br>
pTable->mpRangeName = new ScRangeName;<br></blockquote><div><br>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. <br>
<br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Btw, please use 'r' prefix for parameters passed as reference, so<br>
rNewDoc instead of aNewDoc, and please "const as const can", for example<br>
it should be const ScDocument* pOldDoc<br>
<br>
Apart from that, I'm currently somewhat handicapped with equipment and<br>
can't check if the change really does what it is supposed to do.<br>
<font color="#888888"></font></blockquote><div></div><div><br>Regards,<br>Markus <br></div></div><br>