Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
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
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
Hello, so there is the next try. I've just followed Koheis' suggestions and made the anonymous db a part of ScTable. Patch is of course under LGPLv3+/MPL. Markus 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 2011/3/22 Kohei Yoshida > On Tue, 2011-03-22 at 19:59 +0100, Markus Mohrhard wrote: > > This time with the patch. Sry. > > > > 2011/3/22 Markus Mohrhard > > Hello, > > > > > > > > so I have reworked it. I haven't reworked the import and > > export as these affect some areas I don't fully understand. > > Hi Markus, > > So, I've tested your patch, and it works as long as the sheets are not > moved around, but once you start moving sheets around it starts to act a > bit weird. > > Here is an example. Let's say you start with an empty document. You > put some data into Sheet1, and set autofilter there. Now, move Sheet1 > to the right of Sheet3 so that the sheets are in this order (from left > to right): Sheet2, Sheet3 and Sheet1. Now, put some data into Sheet2 > and set autofilter. The autofilter on Sheet1 now vanishes. > > This was what I was concerned about earlier. Because we are using the > sheet index as part of the name, and use it to look up the sheet-local > anonymous DB data in ScDocShell::GetDBData(), it becomes susceptible to > sheet relocation. Note that sheet index (nTab in ScTable) is always > numbered 0, 1, 2, from left to right. So when you move a sheet, Calc > updates its index to reflect its new position. > > Thinking about this a bit, I think the best solution is *not* to store > the sheet-local anonymous DB's in the global ScDBCollection instace, but > store them directly in the respective ScTable instance, and adjust the > lookup code accordingly. I think this is a much better solution given > the constraint, and one that I would feel more comfortable with. Sorry > I didn't think of this sooner :-/ > > Regarding the import export, let's not worry about that at this stage. > We should first focus on getting the core functionality working first, > before worrying about file import / export. > > Regards, > > Kohei > > > anonymous db range.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
On Tue, 2011-03-22 at 19:59 +0100, Markus Mohrhard wrote: > This time with the patch. Sry. > > 2011/3/22 Markus Mohrhard > Hello, > > > > so I have reworked it. I haven't reworked the import and > export as these affect some areas I don't fully understand. Hi Markus, So, I've tested your patch, and it works as long as the sheets are not moved around, but once you start moving sheets around it starts to act a bit weird. Here is an example. Let's say you start with an empty document. You put some data into Sheet1, and set autofilter there. Now, move Sheet1 to the right of Sheet3 so that the sheets are in this order (from left to right): Sheet2, Sheet3 and Sheet1. Now, put some data into Sheet2 and set autofilter. The autofilter on Sheet1 now vanishes. This was what I was concerned about earlier. Because we are using the sheet index as part of the name, and use it to look up the sheet-local anonymous DB data in ScDocShell::GetDBData(), it becomes susceptible to sheet relocation. Note that sheet index (nTab in ScTable) is always numbered 0, 1, 2, from left to right. So when you move a sheet, Calc updates its index to reflect its new position. Thinking about this a bit, I think the best solution is *not* to store the sheet-local anonymous DB's in the global ScDBCollection instace, but store them directly in the respective ScTable instance, and adjust the lookup code accordingly. I think this is a much better solution given the constraint, and one that I would feel more comfortable with. Sorry I didn't think of this sooner :-/ Regarding the import export, let's not worry about that at this stage. We should first focus on getting the core functionality working first, before worrying about file import / export. Regards, Kohei ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
This time with the patch. Sry. 2011/3/22 Markus Mohrhard > Hello, > > so I have reworked it. I haven't reworked the import and export as these > affect some areas I don't fully understand. > > Patch is under LGPLv3+/MPL. > > Markus > 0002-support-for-one-anonymous-db-range-per-sheet-and-one.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
Hello, so I have reworked it. I haven't reworked the import and export as these affect some areas I don't fully understand. Patch is under LGPLv3+/MPL. Markus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
On Tue, 2011-03-22 at 00:22 +0100, Markus Mohrhard wrote: > Ok, > > > thanks for your advice. I will rework it but suggest that the static > method will be placed in ScDBCollection. Ah, yes. My finger slipped. I meant to type ScDB*, not ScDP*. :-P So many ScFooCollection in Calc core... Glad you caught that. :-) > But there is then there is the question whether an Undo should undo > the last change in the active sheet or undo the last change in any > sheet. Note that ,as I see it, this will only affect changes that were > made with anonymous db ranges. Normally we would want undo to work unless there is a good reason to skip it. For things that are invisible to the user (like the anonymous DB ranges), however, let's make it undoable if it affects the behavior of autofilter. If not, then let's not worry about that. My only concern is that, when the document is saved, all the anonymous ranges need to be saved with the document. When you undo the anonymous DB creation then immediately save the document, we may save unnecessary anonymous ranges to the document. This may not be a big deal in practice, but is something to be aware of. > Yes, I tested that. It seems that nTab is a property of ScTable and > not used to determine the sheet's Position. Good to hear that. I wasn't sure 100%... Kohei ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
Ok, thanks for your advice. I will rework it but suggest that the static method will be placed in ScDBCollection. But there is then there is the question whether an Undo should undo the last change in the active sheet or undo the last change in any sheet. Note that ,as I see it, this will only affect changes that were made with anonymous db ranges. Probably yes. The idea is to replace the concept of one global > anonymous DB name with one anonymous name per sheet *across the calc > code*, so we need to make changes in every place where the global > anonymous name is used currently. After this change, we will no longer > have a global anonymous name anymore (except for backward compatibility > handling in the import code, perhaps). > > With that in mind, let's create a central method that generates > sheet-local anonymous names, and use it everywhere. If I were you I > would create a static method inside ScDPCollection class for that. > > Yes, I tested that. It seems that nTab is a property of ScTable and not used to determine the sheet's Position. > > BTW, how does this handle sheet position changes? For example, let's > assume you have two sheets with auto filter applied (hence two > sheet-local anonymous DBs), and you swap their position. Would the DB > ranges be still valid? > > Kohei > > Markus ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [Patch] allow one anonymous db range per sheet in calc
Hello Markus, On Mon, 2011-03-21 at 19:33 +0100, Markus Mohrhard wrote: > this patch allows one anonymous db range per sheet instead of one per > document. There are still some places the old anonymous db range is > used, but I#m not quite sure whether i should change these. Probably yes. The idea is to replace the concept of one global anonymous DB name with one anonymous name per sheet *across the calc code*, so we need to make changes in every place where the global anonymous name is used currently. After this change, we will no longer have a global anonymous name anymore (except for backward compatibility handling in the import code, perhaps). With that in mind, let's create a central method that generates sheet-local anonymous names, and use it everywhere. If I were you I would create a static method inside ScDPCollection class for that. > Additionally it allows one autofilter per sheet. Nice! This is what people have been asking for. > I'd appreciate a better solution for generating strDBNoName, but i > have found no way to append an integer to String. We tend to use rtl::OUStringBuffer for appending integers to string. So, for instance, you could do rtl::OUStringBuffer aBuf; aBuf.append(ScGlobal::GetRscString(STR_DB_NONAME)); aBuf.append(nTab); aBuf.makeStringAndClear() // <= to get rtl::OUString instance. to generate sheet local anonymous names. BTW, how does this handle sheet position changes? For example, let's assume you have two sheets with auto filter applied (hence two sheet-local anonymous DBs), and you swap their position. Would the DB ranges be still valid? Kohei ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [Patch] allow one anonymous db range per sheet in calc
Hi, this patch allows one anonymous db range per sheet instead of one per document. There are still some places the old anonymous db range is used, but I#m not quite sure whether i should change these. Additionally it allows one autofilter per sheet. I'd appreciate a better solution for generating strDBNoName, but i have found no way to append an integer to String. Greetings Markus 0001-support-for-one-anonymous-db-range-per-sheet.patch Description: Binary data ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
