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 (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
