Re: [Libreoffice] [PUSHED] Allow GoUpSel, GoDownSel, GoLeftsel, GoRightSel to use "By" property

Hi Kohei,

On Wednesday, 2011-08-10 17:25:38 -0400, Kohei Yoshida wrote:

> On Wed, 2011-08-10 at 18:06 -0300, Olivier Hallot wrote:
> > I had to add this patch to prevent the table boudaries crash issue
> > that raised when one use the By property for GoLeftSel and GoRightSel.
>
> I would put the boundary check before incrementing and decrementing the
> column / row position, to be consistent with the previous if statement
> which checks the next position before moving the position, not after.
>
> So, attached is my proposed change.

Well, here's what I would do instead ;-)

The check should be done before the calls to isCellQualified(), surely
calling that with rCol/rRow out of bounds doesn't make much sense and at
best would return false anyway, hopefully. That immediately leads to the
new conditions can be moved one more up into the for loop conditions,
which shows that it is the same condition as the surrounding if block
that can be removed then, and for negative nMovX/nMovY the loop variable
be reversed, leaving only code of the attached solution. All untested.

Even the remaining if blocks checking for nMovX/nMovY/>0/<0 could be
removed now as the loops care for that, but may serve better
readability. Actually no runtime improvement would result from that, au
contraire, as there would be still the assignment and check of the loop
variable in addition.

  Eike

-- 
 PGP/OpenPGP/GnuPG encrypted mail preferred in all private communication.
 Key ID: 0x293C05FD - 997A 4C60 CE41 0149 0DB3  9E96 2F1A D073 293C 05FD

diff --git a/sc/source/ui/view/tabview2.cxx b/sc/source/ui/view/tabview2.cxx
--- a/sc/source/ui/view/tabview2.cxx
+++ b/sc/source/ui/view/tabview2.cxx
@@ -86,53 +86,39 @@ void moveCursorByProtRule(
 
     if (nMovX > 0)
     {
-        if (rCol < MAXCOL)
+        for (SCCOL i = 0; i < nMovX && rCol < MAXCOL; ++i)
         {
-            for (SCCOL i = 0; i < nMovX; ++i)
-            {
-                if (!isCellQualified(pDoc, rCol+1, rRow, nTab, bSelectLocked, bSelectUnlocked))
-                    break;
-                ++rCol;
-            }
+            if (!isCellQualified(pDoc, rCol+1, rRow, nTab, bSelectLocked, bSelectUnlocked))
+                break;
+            ++rCol;
         }
     }
     else if (nMovX < 0)
     {
-        if (rCol > 0)
+        for (SCCOL i = 0; i > nMovX && rCol > 0; --i)
         {
-            nMovX = -nMovX;
-            for (SCCOL i = 0; i < nMovX; ++i)
-            {
-                if (!isCellQualified(pDoc, rCol-1, rRow, nTab, bSelectLocked, bSelectUnlocked))
-                    break;
-                --rCol;
-            }
+            if (!isCellQualified(pDoc, rCol-1, rRow, nTab, bSelectLocked, bSelectUnlocked))
+                break;
+            --rCol;
         }
     }
 
     if (nMovY > 0)
     {
-        if (rRow < MAXROW)
+        for (SCROW i = 0; i < nMovY && rRow < MAXROW; ++i)
         {
-            for (SCROW i = 0; i < nMovY; ++i)
-            {
-                if (!isCellQualified(pDoc, rCol, rRow+1, nTab, bSelectLocked, bSelectUnlocked))
-                    break;
-                ++rRow;
-            }
+            if (!isCellQualified(pDoc, rCol, rRow+1, nTab, bSelectLocked, bSelectUnlocked))
+                break;
+            ++rRow;
         }
     }
     else if (nMovY < 0)
     {
-        if (rRow > 0)
+        for (SCROW i = 0; i > nMovY && rRow > 0; --i)
         {
-            nMovY = -nMovY;
-            for (SCROW i = 0; i < nMovY; ++i)
-            {
-                if (!isCellQualified(pDoc, rCol, rRow-1, nTab, bSelectLocked, bSelectUnlocked))
-                    break;
-                --rRow;
-            }
+            if (!isCellQualified(pDoc, rCol, rRow-1, nTab, bSelectLocked, bSelectUnlocked))
+                break;
+            --rRow;
         }
     }
 }

Attachment: pgpWC7xCaDkEc.pgp
Description: PGP signature