Hi Markus,
It took me a little to get to your patch, but finally I've been able to
get around to reviewing it.
On Thu, 2011-03-24 at 17:43 +0100, Markus Mohrhard wrote:
> so there is the next try. I've just followed Koheis' suggestions and
> made the anonymous db a part of ScTable.
So, this looks much better. And it works pretty solid during sheet
relocation.
There are still issues with undo and file import/export, but I guess
these are known issues?
Asides from that, there are several minor nitpicks.
ScTable now has a new data member pDBDataNoName. We need to properly
initialize it to NULL in the constructor, and delete its instance in the
destructor.
You decided to expose ScTable instance outside of ScDocument via
ScTable* GetTable(SCTAB nTab), but we by design encapsulate ScTable
inside ScDocument and don't allow the code outside of it to access
ScTable. So, we need to add a wrapper method to ScDocument to return
sheet's anonymous db range indirectly. Adding GetAnonymousDBData() that
takes a table index as the parameter will just do.
Also, this is very minor but ...
+void ScTable::SetAnonymousDBData(ScDBData* aDBData)
+{
+ if (pDBDataNoName)
+ delete pDBDataNoName;
+ pDBDataNoName = aDBData;
+}
calling delete on a NULL pointer is a no-op, and is perfectly safe. So
there is no need to check for NULL pointer here. You can just call
delete unconditionally. And...
- if (pData && pData->GetName() != aStrNoName)
+ if (pData && ( !OUString(pData->GetName()).match(aStrNoName) ) )
you need to use equals to check for (in)equality instead of match().
Other than that, it looks pretty good. :-)
Incidentally, we have a feature freeze on Monday, and it would be nice
to be able to push this change in before the freeze. Do you think
that's doable, or do you feel that's too close?
> P.S. Sry that patch is not created with git format-patch but I don't
> know how to create a patch against a specific point
Nah, that's not an issue at all. No worries.
Kohei
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
