Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area (2)
Hi Eicke,
The second one:
0001-fdo42286-extend-down-but-also-shrink-if-cells-are-em.patch
For consistency it makes sense to also shrink the area, as
re-initializing the filter area with only one cell or one row selected
.
erroneously included. Please go ahead with this one.
OK
Please check that a defined data base range did not change behavior with
your previous changes.
I have look at the code where GetDataArea is called. The only places
where a behaviour change could happen are in function GetDBData (
sc/source/ui/docshell/docsh5.cxx )
if bOnlyDown is true. In this case, the area will be shrink - if needed
- only for the number of rows. Before, all 4 sides (top, right, left
and bottom) could be shrink. But this change exists only if we shrink.
For expanding, the behaviour was already ensured as described (bOnlyDown
= true would have expanded only down, not the other directions).
This now makes the behaviour symmetric between shrinking / expanding the
area. Based on flags and description, the former behaviour was buggy,
but maybe something has been build depending of it. Despite knowing what
is changed, I was not able to produce a way of using it that seemed to
be problematic. I see you have worked on this function, maybe something
will strike you immediately :- ).
So here the patch. If no one object, I will push it during my Christmas
Holiday.
Best regards
Pierre-André
From 994b2c2e5a760503f8e466ae8068cf1cc453a712 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Andr=C3=A9=20Jacquod?= pjacq...@alumni.ethz.ch
Date: Thu, 15 Dec 2011 19:29:17 +0100
Subject: [PATCH 1/2] fdo42286 better solution: extend and shrink end of row if needed
---
sc/source/core/tool/dbdata.cxx |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/sc/source/core/tool/dbdata.cxx b/sc/source/core/tool/dbdata.cxx
index 3d60554..3cc4146 100644
--- a/sc/source/core/tool/dbdata.cxx
+++ b/sc/source/core/tool/dbdata.cxx
@@ -544,10 +544,8 @@ void ScDBData::UpdateReference(ScDocument* pDoc, UpdateRefMode eUpdateRefMode,
void ScDBData::ExtendDataArea(ScDocument* pDoc)
{
// Extend the DB area to include data rows immediately below.
-SCCOL nCol1a = nStartCol, nCol2a = nEndCol;
-SCROW nRow1a = nStartRow, nRow2a = nEndRow;
-pDoc-GetDataArea(nTable, nCol1a, nRow1a, nCol2a, nRow2a, true, true);
-nEndRow = nRow2a;
+// or shrink it if all cells are empty
+pDoc-GetDataArea(nTable, nStartCol, nStartRow, nEndCol, nEndRow, false, true);
}
namespace {
--
1.7.3.4
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area (2)
Hi Pierre-André, On Sunday, 2011-12-04 22:25:51 +0100, Pierre-André Jacquod wrote: The first one is my favorite: 0001-fdo42286-strict-use-of-GetDataArea-and-strict-extens.patch it extends the area down. It takes into account the cells strictly below the already selected area. It never shrinks / shortens the selected area. This is the one that implement in my opinion the best the behaviour of adding data below already active area. The second one: 0001-fdo42286-extend-down-but-also-shrink-if-cells-are-em.patch has the same logic, except it allows to shrink the area, if cells are emptied. This the filter is allowed to extend, it could be seen as logic that it is also allowed to shrink. For consistency it makes sense to also shrink the area, as re-initializing the filter area with only one cell or one row selected would produce the same result (plus additional columns). Also, if the area was never shrunk and later data added in that area but not part of the actual data area (not contiguous area) that data would be erroneously included. Please go ahead with this one. Please check that a defined data base range did not change behavior with your previous changes. Regarding variable names, it may be better to stick with naming conventions of already existing code, i.e. it should be bShrink and bNeedExtend, makes the code easier to read. Thanks. The last one: 0001-fdo42286-extend-down-even-if-last-row-empty-but-a-co.patch extend down, but also if data is added to the first cell bellow. so if you have something like (o means empty cell, x cell with data), initial filter only on B2:D3 o oXXXo oXXXo X and add the last X below right, the the last line will be included in the area and shown with empty cells selection. I do not like this, since it suddenly take into account a column which was not part of the initial filtered area. Mainly because it doesn't fulfill the contiguous data area aspects, for which at least E3 or D4 would also have to contain data. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgpfl0x8bZg1f.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area (2)
Hello, To give you a background, this extend range downward functionality was Thanks for your explanation. Following the remark of Eicke, I ended up with 3 possible patches :-/ which all have small differences in behaviour. I have a favorite, but do not want to choose alone. Please push the one you think is the best for the filter drop-down. The first one is my favorite: 0001-fdo42286-strict-use-of-GetDataArea-and-strict-extens.patch it extends the area down. It takes into account the cells strictly below the already selected area. It never shrinks / shortens the selected area. This is the one that implement in my opinion the best the behaviour of adding data below already active area. The second one: 0001-fdo42286-extend-down-but-also-shrink-if-cells-are-em.patch has the same logic, except it allows to shrink the area, if cells are emptied. This the filter is allowed to extend, it could be seen as logic that it is also allowed to shrink. The last one: 0001-fdo42286-extend-down-even-if-last-row-empty-but-a-co.patch extend down, but also if data is added to the first cell bellow. so if you have something like (o means empty cell, x cell with data), initial filter only on B2:D3 o oXXXo oXXXo X and add the last X below right, the the last line will be included in the area and shown with empty cells selection. I do not like this, since it suddenly take into account a column which was not part of the initial filtered area. Due to my job, I have not been very available last week, and it will be the same this week. (I will not be able to code / push until 9th). would be nice if this could be inserted before branching to 3.5.0 As prerequisite for working, the following commits are needed: 7359ad4fc772bc355905ef8b4a4a7b44dcfc1ebe 2e5023f974dd94dfeec0554ce07d0544f9ce7638 e42ee773ffc12e38d596ce2aa016f0849c4e5ac6 Regards Pierre-André 0001-fdo42286-strict-use-of-GetDataArea-and-strict-extens.patch Description: application/mbox 0001-fdo42286-extend-down-but-also-shrink-if-cells-are-em.patch Description: application/mbox 0001-fdo42286-extend-down-even-if-last-row-empty-but-a-co.patch Description: application/mbox ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area (2)
On Tue, 2011-11-29 at 19:39 +0100, Pierre-André Jacquod wrote:
Hello,
sorry, I was sure there was more about it...but too early this morning,
I could not sort out all points.
Now you also changed bOnlyDown=false to bOnlyDown=true, which leads to
not include newly appended columns of an area. Why?
by the way, this reflect the comment in
void ScDBData::ExtendDataArea(ScDocument* pDoc)
{
// Extend the DB area to include data rows immediately below.
which comment is from Kohei ( 9f9ff37f ) who toke this from a previous
comment. Hence, the change would make the call be in line with the comment.
This is the original reason why I started to play with it and changed it
like this, since comment and implementation were not coherent.
So, it's possible that there is some kind of code merging madness going
on here.
To give you a background, this extend range downward functionality was
present for quite some time in Go-OO, which was never merged into the
official OOo. Naturally, when we did the LibreOffice, that
functionality was merged in.
During the 3.3 release period, we were constantly merging changes from
OOo almost on every milestone release (IIRC). And I noticed that OOo
added similar extend range downward functionality during that period
but quite in late milestone, but did it differently than the way we did
it.
We ended up merging that change anyway, and because we both did it
differently, the merge may have ended up making the code around here
funny. That would explain some of the inconsistencies you may be seeing
in this neighborhood.
Kohei
--
Kohei Yoshida, LibreOffice hacker, Calc
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area
Hi Eike
Now you also changed bOnlyDown=false to bOnlyDown=true, which leads to
not include newly appended columns of an area.
half true: the condition is exactly that the newly appended column data
touch by an angle the already defined area (like C3 to B2) and that all
other cells below the area are empty.
All columns right/left of the area are filtered.
Why?
very bad reason: I did not really intended to commit this change yet. I
wanted to ask you about it. But it seems I was too deeply thinking about
it, I oversee my mixing. F**
less bad reason: If you activates the auto-filter, and append a column
as mentioned above, you suddenly have columns filtered without seeing
it, the drop-down at the top of the column not being present.
I will change it (due to point 1), except you oppose :- )
Grounded speculation: if user adds a column or row immediately adjacent
to a data area it is usually related to the already existing data and
she wants that to be included, maybe even doesn't know that earlier the
filter was setup using a selection.
A good point. On the other hand,this makes impossible to filter in the
middle of data. But this case is very special.
Else, I have attached 3 patches for review touching this function:
* the first one make the flag onlyDown working also for shrinking... No
reason that you can just extend down, but shrink on all direction? This
seems to be more coherent with the flag activation.
* the second patch make the shrinking corresponding to the name of the
function: GetDataArea: i.e. shrinks as long as the outer row/column is
empty, and not just delete the last one since empty.
* the 3d one: just minor clean up: variable scope reduction and better
documentation.
Thanks for all
best regards
Pierre-André
From cfd04dda7e441ffa3dcc5a21b6b8f87cbfda05b0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pierre-Andr=C3=A9=20Jacquod?= pjacq...@alumni.ethz.ch
Date: Tue, 29 Nov 2011 09:10:26 +0100
Subject: [PATCH 3/3] reduce scope of var and better comment of function GetDataArea
---
sc/source/core/data/table1.cxx | 40 +---
1 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/sc/source/core/data/table1.cxx b/sc/source/core/data/table1.cxx
index ce2051b..5de641f 100644
--- a/sc/source/core/data/table1.cxx
+++ b/sc/source/core/data/table1.cxx
@@ -741,17 +741,19 @@ bool ScTable::GetDataStart( SCCOL rStartCol, SCROW rStartRow ) const
void ScTable::GetDataArea( SCCOL rStartCol, SCROW rStartRow, SCCOL rEndCol, SCROW rEndRow,
bool bIncludeOld, bool bOnlyDown ) const
{
-// bIncludeOld = true ensure that the returned area contains at least the initial area,
-// independently of the case if this area has empty rows / columns at its borders
-// bOnlyDown = true means extend the inputed area only down, i.e increase only rEndRow
+// return the smallest area containing at least all contiguous cells having data. This area
+// is a square containing also empty cells. It may shrink or extend the area given as input
+// Flags as modifiers:
+//
+// bIncludeOld = true ensure that the returned area contains at least the initial area,
+// independently of the emptniess of rows / columns (i.e. does not allow shrinking)
+// bOnlyDown = true means extend / shrink the inputed area only down, i.e modifiy only rEndRow
+
bool bLeft = false;
bool bRight = false;
bool bTop = false;
bool bBottom = false;
-bool bChanged;
-bool bFound;
-SCCOL i;
-SCROW nTest;
+bool bChanged = false;
do
{
@@ -782,12 +784,12 @@ void ScTable::GetDataArea( SCCOL rStartCol, SCROW rStartRow, SCCOL rEndCol, S
if (rStartRow 0)
{
-nTest = rStartRow-1;
-bFound = false;
-for (i=rStartCol; i=rEndCol !bFound; i++)
+SCROW nTest = rStartRow-1;
+bool needExtend = false;
+for ( SCCOL i = rStartCol; i=rEndCol !needExtend; i++)
if (aCol[i].HasDataAt(nTest))
-bFound = true;
-if (bFound)
+needExtend = true;
+if (needExtend)
{
--rStartRow;
bChanged = true;
@@ -798,12 +800,12 @@ void ScTable::GetDataArea( SCCOL rStartCol, SCROW rStartRow, SCCOL rEndCol, S
if (rEndRow MAXROW)
{
-nTest = rEndRow+1;
-bFound = false;
-for (i=rStartCol; i=rEndCol !bFound; i++)
+SCROW nTest = rEndRow+1;
+bool needExtend = false;
+for ( SCCOL i = rStartCol; i=rEndCol !needExtend; i++)
if (aCol[i].HasDataAt(nTest))
-bFound = true;
-if (bFound)
+needExtend = true;
+if (needExtend)
{
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area (2)
Hello,
sorry, I was sure there was more about it...but too early this morning,
I could not sort out all points.
Now you also changed bOnlyDown=false to bOnlyDown=true, which leads to
not include newly appended columns of an area. Why?
by the way, this reflect the comment in
void ScDBData::ExtendDataArea(ScDocument* pDoc)
{
// Extend the DB area to include data rows immediately below.
which comment is from Kohei ( 9f9ff37f ) who toke this from a previous
comment. Hence, the change would make the call be in line with the comment.
This is the original reason why I started to play with it and changed it
like this, since comment and implementation were not coherent.
Regards
Pierre-André
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area
Hi Pierre-André, On Sunday, 2011-11-27 20:23:33 +0100, Pierre-André Jacquod wrote: I wouldn't say it's wrong unless I checked the original intention behind that code when GetDataArea() is called with bIncludeOld=false, maybe it's just the call in ExtendDataArea() that should pass true instead? Done, you're right. Finally, changed the call of the flags and documented the flags' meaning. If you could cherry-pick 88611e702a18d2 for 3.4.5, would be nice. Now you also changed bOnlyDown=false to bOnlyDown=true, which leads to not include newly appended columns of an area. Why? Further some tests have shown me that the behaviour (regarding area) is not the same, depending if the filter is activated with Data-Filter-AutoFilter or Standard filter. I fear some parts will need to be quite overhauled. And the difference exactly is ...? Visually: the standard filter adapts and shows the selected area at activation of the filter, the auto-filter does not reflect the fact that it will take into account a wider area. This area extension happens only when the drop-down button is activated and the list of possible value is calculated... and the area extended. Probably because people want a selection within an auto-filtered area to stay active and nevertheless filter for new values. Reselecting the entire data area in that moment isn't a good user-experience. Out of subject: from a user perspective, I am not convinced from te fact that the filter is allowed to change an area that has been user-defined. No problem to auto-extend an area if auto-determined. But if a user takes the time to do the area, this should be holly. But this point belongs to the user-interface list I guess... Grounded speculation: if user adds a column or row immediately adjacent to a data area it is usually related to the already existing data and she wants that to be included, maybe even doesn't know that earlier the filter was setup using a selection. Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgpNyoc15PpTQ.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area
Hi, BUT this is a workaround, to compensate the not so correct behaviour of GetDataArea :-( The behaviour in bINcludeOld = false seems me strange (ergh I think wrong but you told me to be cautious), since it removes from the side of the area that has not yet been extended above the last line / column, if empty. The implementation seems to me flawed, since it does it even if no extension has been possible (e.g rEndRow = MAXROW), and suppress just ONE line, if empty. What about the new last one ? if also empty, we are exactly in the same situation as before, just a bit smaller. Hence it seems me that the original programmer (12 years ago) just assumed that only the last line / column could be empty, not e.g. the last 4 rows. I wouldn't say it's wrong unless I checked the original intention behind that code when GetDataArea() is called with bIncludeOld=false, maybe it's just the call in ExtendDataArea() that should pass true instead? Done, you're right. Finally, changed the call of the flags and documented the flags' meaning. If you could cherry-pick 88611e702a18d2 for 3.4.5, would be nice. Many thanks desired effect. Further some tests have shown me that the behaviour (regarding area) is not the same, depending if the filter is activated with Data-Filter-AutoFilter or Standard filter. I fear some parts will need to be quite overhauled. And the difference exactly is ...? Visually: the standard filter adapts and shows the selected area at activation of the filter, the auto-filter does not reflect the fact that it will take into account a wider area. This area extension happens only when the drop-down button is activated and the list of possible value is calculated... and the area extended. More subtly, this leads to 2 different ranges stored within the file if you do select some area but not all data, activate standard filter , save or if you do exactly the same with same data / input except activating auto-filter instead of standard filter. Out of subject: from a user perspective, I am not convinced from te fact that the filter is allowed to change an area that has been user-defined. No problem to auto-extend an area if auto-determined. But if a user takes the time to do the area, this should be holly. But this point belongs to the user-interface list I guess... Best regards Pierre-André ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [PATCH] fdo#42286, do not shrink the selected area
Hi Pierre-André, On Thursday, 2011-11-24 22:47:56 +0100, Pierre-André Jacquod wrote: here attached an new patch (hopefully wiser) to solve the mentioned bug. This ensure that the call to ExtendDataArea does as commented in the code sc/source/core/tool/dbdata.cxx : // Extend the DB area to include data rows immediately below. and does not shrink the original area... So having a smaller area after the function is clearly wrong. If you agree, I will push it to master. Yes, that looks good, if it solves your problem then please go ahead. BUT this is a workaround, to compensate the not so correct behaviour of GetDataArea :-( I wouldn't say it's wrong unless I checked the original intention behind that code when GetDataArea() is called with bIncludeOld=false, maybe it's just the call in ExtendDataArea() that should pass true instead? About the root cause: I am still studing the code of the both mentioned functions and their integration with filters and filtered areas. Before touching it, I would like to define what should be the desired effect. Further some tests have shown me that the behaviour (regarding area) is not the same, depending if the filter is activated with Data-Filter-AutoFilter or Standard filter. I fear some parts will need to be quite overhauled. And the difference exactly is ...? Eike -- LibreOffice Calc developer. Number formatter stricken i18n transpositionizer. GnuPG key 0x293C05FD : 997A 4C60 CE41 0149 0DB3 9E96 2F1A D073 293C 05FD pgpxdPKUpHM6E.pgp Description: PGP signature ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [PATCH] fdo#42286, do not shrink the selected area
hello, here attached an new patch (hopefully wiser) to solve the mentioned bug. This ensure that the call to ExtendDataArea does as commented in the code sc/source/core/tool/dbdata.cxx : // Extend the DB area to include data rows immediately below. and does not shrink the original area... So having a smaller area after the function is clearly wrong. If you agree, I will push it to master. BUT this is a workaround, to compensate the not so correct behaviour of GetDataArea :-( I did this patch in order to bring the correction in time for 3.5.0 3.4.5, but I agree, that this is suppressing a symptom and not the root cause. About the root cause: I am still studing the code of the both mentioned functions and their integration with filters and filtered areas. Before touching it, I would like to define what should be the desired effect. Further some tests have shown me that the behaviour (regarding area) is not the same, depending if the filter is activated with Data-Filter-AutoFilter or Standard filter. I fear some parts will need to be quite overhauled. As said, won't be ready for 3.5, so first this short term solution. Thanks toa all for having helped me so far. Best regards Pierre-André 0001-fdo-42286-do-not-shrink-the-filtered-area-when-calli.patch Description: application/mbox ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
