Bug Hunting Session
Bug 89216 - Annoying feature: empty cell treated as zero within range/array
Summary: Annoying feature: empty cell treated as zero within range/array
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium major
Assignee: Teraslilja
URL:
Whiteboard: target:6.0.0
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-02-08 00:21 UTC by Teraslilja
Modified: 2018-03-02 20:20 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Contains basic function that stacks nonempty content of cells of given range (37.10 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-02-08 00:21 UTC, Teraslilja
Details
Demo of 'shadow range' method (15.89 KB, application/vnd.oasis.opendocument.spreadsheet)
2015-02-11 23:08 UTC, LeMoyne Castle
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Teraslilja 2015-02-08 00:21:03 UTC
Created attachment 113216 [details]
Contains basic function that stacks nonempty content of cells of given range

It seems impossible for basic function to identify empty cell from zero at given array range.

Expecting: VarType of Variant of empty cell is either Empty or Null.
Feature(bug?): VarType of Variant of empty cell is actually returned as Double.


Function StackNonEmpty( range As Variant, delimiter As String )
    Dim result As String

    result = ""

    If NOT IsMissing( range ) Then
        If NOT IsArray( range ) Then
            result = range
        Else
	    Dim oCell As Variant
	    Dim bInclude, bString as Boolean
	    Dim sCell As String
	    Dim sType As String

	    Dim row, col As Integer
            For row = LBound( range, 1 ) To UBound( range, 1 )
                For col = LBound( range, 2 ) To UBound( range, 2 )
                    oCell = range( row, col )
		    Select Case VarType( oCell )
		    Case 0
			bInclude = False
			bString = False
			sType="Empty"
		    Case 1
			bInclude = False
			bString = False
			sType="Null"
		    Case 2
			bInclude = True
			bString = False
			sType="Integer"
		    Case 3
			bInclude = True
			bString = False
			sType="Long"
		    Case 4
			bInclude = True
			bString = False
			sType="Single"
		    Case 5
			bInclude = True
			bString = False
			sType="Double"
		    Case 7
			bInclude = True
			bString = False
			sType="Date"
		    Case 8
			bInclude = True
			bString = True
			sType="String"
		    Case 9
			bInclude = False
			bString = False
			sType="Object"
		    Case 11
			bInclude = True
			bString = False
			sType="Boolean"
		    Case 12
			bInclude = False
			bString = False
			sType="Variant"
		    Case Else
			bInclude = False
			bString = False
			sType="Unexpected"
		    End Select

                    If bInclude Then
                    	If bString Then
                    	    sCell = range( row, col )

		            If Len( sCell ) > 0 Then
	                        If result <> "" Then
	                            result = result & delimiter
	                        End If
	                        result = result & sCell & ": " & sType
	                    End If
                    	Else
                            If result <> "" Then
	                        result = result & delimiter
	                    End If
	                    result = result & oCell & ": " & sType
                    	End If
                    End If
                Next
            Next
        End If
    End If

    StackNonEmpty = result
End Function
Comment 1 raal 2015-02-08 08:35:46 UTC
Hello,
you can identify cell content with this code.
Is this code what you need?


function isempty()

Dim Doc As Object
Dim Sheet As Object
Dim Cell As Object

Doc = ThisComponent
Sheet = Doc.Sheets(0)
Cell = Sheet.getCellByPosition(1,1)        'Cell "B2" (0-based!)



With com.sun.star.table.CellContentType
  Select Case Cell.Type
    Case .EMPTY 
      MsgBox "Content: Empty"
    Case .VALUE
      MsgBox "Content: Value"
    Case .TEXT
      MsgBox "Content: Text"
    Case .FORMULA
     MsgBox "Content: Formula"
  End Select
End With
end function
Comment 2 Teraslilja 2015-02-08 11:16:27 UTC
Well ... No.

The function needs a *range of cells* as parameter, and apparently it is given as Array, even if function declares otherwise.

Function StackNonEmpty( range As com.sun.star.table.CellRange, delimiter As String )
    Dim result As String

    MsgBox "IsArray = " & IsArray( range )
    result = ""
...

Shows a message box with text: "IsArray = True"
Comment 3 LeMoyne Castle 2015-02-11 23:03:38 UTC
The use of a range in a spreadsheet function does produce an array of values rather than an array of cells, at least within Basic.

The solution/workaround to get the cell status into a Basic macro is to use the ISBLANK function to create a shadow range in the worksheet.  This range records whether or not the corresponding cell in the original study range is empty or not (column D in attachment). The Function then gets passed two arrays and can work as desired.

Here is sample (similar, working code in attachment): 

Function StackNonEmpty2( range As Variant, IsEmpty As Variant, delimiter As String )

    ' NEED to check parameters and exit gracefully
    ' NEED to check for workable dimension: range larger than IsEmpty => bug
   
    Dim result, tag As String
    Dim row, col As Integer

    result = ""
    For row = LBound( range, 1 ) To UBound( range, 1 )
        For col = LBound( range, 2 ) To UBound( range, 2 ) 
            If NOT CBool( IsEmpty( row, col ) ) then 
	        Select Case VarType(range(row, col)): 
	           Case 5: tag = "Double"
                   Case 8: tag = "String" 
	           Case Else: tag = "other type"
                end select
	        If result <> "" Then
                    result = result & delimiter
                End If
	        result = result & range( row, col ) & ": " & tag
            End If        	
        Next
    Next
    StackNonEmpty2 = result
End Function

There is almost certainly another solution that passes ADDRESS information to the function.  The function would then interrogate the spreadsheet directly as in raal's example above.  This second method would allow the Basic code to gather more information about the Cells' type and status.
Comment 4 LeMoyne Castle 2015-02-11 23:08:25 UTC
Created attachment 113327 [details]
Demo of 'shadow range' method

Attachment demonstrates solution by passing shadow range that has ISBLANK() values for original target range to get required info into Basic macro.
Comment 5 Teraslilja 2015-02-12 08:01:20 UTC
Nice idea, but I think that this time the solution won't work with cases, where the range is rather large, both in rows AND columns (in my example there were just a single column for simplicity). 

In my opinion, the only correct fix is to pass value of type of "Empty" or "Null" for empty cells in the array.
Comment 6 Robinson Tryon (qubit) 2016-08-05 21:40:59 UTC
(In reply to Teraslilja from comment #5)
> Nice idea, but I think that this time the solution won't work [...]

This bug has an assignee and it sounds like there's been actual useful discussion in the comments, so I'm tentatively changing status to NEW.

That being said, it hasn't had any activity for over a *year*, so unless there's going to be ongoing discussion, I suggest we close it. We can always open a new bug if desired.
Comment 7 Teraslilja 2016-08-07 20:38:00 UTC
(In reply to Robinson Tryon (qubit) from comment #6)
> 
> This bug has an assignee and it sounds like there's been actual useful
> discussion in the comments, so I'm tentatively changing status to NEW.
> 
> That being said, it hasn't had any activity for over a *year*, so unless
> there's going to be ongoing discussion, I suggest we close it. We can always
> open a new bug if desired.

After waiting an year, I decided to fix this myself. It did take couple days to identify the place, where the parameter of function is filled. And another day to write a patch.

Warning: If user defined macros assumes that only string/double types are passed, they shall fail or change behaviour.


But anyway, I believe that this is STILL only a partial fix, because I spotted two different places and there are more, e.g. case svExternalSingleRef or (potentially) when bUseVBAObjects is true at ScInterpreter::ScMacro().

Typically the current code behaves so that it checks, if value of cell is a string, then a string is passed. Otherwise a double is passed, even thought the content of cell can be e.g. empty, boolean etc.


The partial patch:
diff --git a/sc/source/core/tool/interpr4.cxx b/sc/source/core/tool/interpr4.cxx
index 055d6d9..c07dbac 100644
--- a/sc/source/core/tool/interpr4.cxx
+++ b/sc/source/core/tool/interpr4.cxx
@@ -3280,14 +3280,30 @@ void ScInterpreter::ScMacro()
                         {
                             nIdx[ 1 ] = static_cast<sal_Int32>(nMatCol+1);
                             SbxVariable* p = refArray->Get32( nIdx );
-                            if (pMat->IsString(nMatCol, nMatRow))
+                            if (pMat->IsEmpty(nMatCol, nMatRow))
+                            {
+                                p->PutEmpty();
+                            }
+                            if (pMat->IsEmptyPath(nMatCol, nMatRow))
+                            {
+                                p->PutEmpty();
+                            }
+                            else if (pMat->IsBoolean(nMatCol, nMatRow))
+                            {
+                                p->PutBool( pMat->Get(nMatCol, nMatRow).GetBoolean() );
+                            }
+                            else if (pMat->IsString(nMatCol, nMatRow))
                             {
                                 p->PutString( pMat->GetString(nMatCol, nMatRow).getString() );
                             }
-                            else
+                            else if (pMat->IsValue(nMatCol, nMatRow))
                             {
                                 p->PutDouble( pMat->GetDouble(nMatCol, nMatRow));
                             }
+                            else
+                            {
+                                p->PutEmpty();
+                            }
                         }
                     }
                     pPar->PutObject( refArray );
@@ -3449,11 +3465,11 @@ bool ScInterpreter::SetSbxVariable( SbxVariable* pVar, const ScAddress& rPos )
                 }
                 break;
             default :
-                pVar->PutDouble( 0.0 );
+                pVar->PutEmpty();
         }
     }
     else
-        pVar->PutDouble( 0.0 );
+        pVar->PutEmpty();
     return bOk;
 }
Comment 8 Teraslilja 2016-08-07 22:11:09 UTC
And a patch for case svExternalSingleRef:

diff --git a/sc/source/core/tool/interpr4.cxx b/sc/source/core/tool/interpr4.cxx
index 055d6d9..5da022e 100644
--- a/sc/source/core/tool/interpr4.cxx
+++ b/sc/source/core/tool/interpr4.cxx
@@ -3189,14 +3189,24 @@ void ScInterpreter::ScMacro()
                     bOk = false;
                 else
                 {
-                    if ( pToken->GetType() == svString )
-                        pPar->PutString( pToken->GetString().getString() );
-                    else if ( pToken->GetType() == svDouble )
-                        pPar->PutDouble( pToken->GetDouble() );
-                    else
+                    switch( pToken->GetType() )
                     {
-                        SetError( errIllegalArgument );
-                        bOk = false;
+                        case svEmptyCell:
+                            pPar->PutEmpty();
+                            break;
+
+                        case svString:
+                            pPar->PutString( pToken->GetString().getString() );
+                            break;
+
+                        case svDouble:
+                            pPar->PutDouble( pToken->GetDouble() );
+                            break;
+
+                        default:
+                           SetError( errIllegalArgument );
+                           bOk = false;
+                           break;
                     }
                 }
             }
Comment 9 Teraslilja 2016-08-09 11:36:27 UTC
As I suspected, there are similar code (if not string, then double) used in other parts of calc, like e.g.

./sc/source/core/tool/interpr5.cxx:            if (!pMat1->IsString(i,j) && !pMat2->IsString(i,j))
./sc/source/core/tool/interpr5.cxx-            {
./sc/source/core/tool/interpr5.cxx-                fVal = pMat1->GetDouble(i,j);
./sc/source/core/tool/interpr5.cxx-                fSum += fVal * fVal;
./sc/source/core/tool/interpr5.cxx-                fVal = pMat2->GetDouble(i,j);

./sc/source/core/tool/rangeseq.cxx:            if ( pMatrix->IsString( nCol, nRow ) )
...
./sc/source/core/tool/rangeseq.cxx-            else
./sc/source/core/tool/rangeseq.cxx-                pColAry[nCol] = pMatrix->GetDouble( nCol, nRow );

./sc/source/core/tool/interpr3.cxx:            if (!pMat1->IsString(i,j))
./sc/source/core/tool/interpr3.cxx-            {
./sc/source/core/tool/interpr3.cxx-                fVal = pMat1->GetDouble(i,j);
...
./sc/source/core/tool/interpr3.cxx-            }

etc
Perhaps a place for another bug report ...
Comment 10 Xisco Faulí 2017-07-13 09:22:34 UTC
Setting Assignee back to default. Please change it back if you're still working on this issue
Comment 11 Xisco Faulí 2017-09-24 16:36:56 UTC
Hello Teraslilja,
Thanks for the patch.
Could you please submit the patch to gerrit as described here:
https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch?
Comment 12 Teraslilja 2017-10-02 21:53:05 UTC
(In reply to Xisco Faulí from comment #11)
> Hello Teraslilja,
> Thanks for the patch.
> Could you please submit the patch to gerrit as described here:
> https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch?

The patch(es) above won't work any more. The object passing a cell range to basic macro has been changed. So I have written a new patch that I will submit later this week.

diff --git a/sc/source/core/tool/interpr4.cxx b/sc/source/core/tool/interpr4.cxx
index b2dc0adee55e..518a1475a5f2 100644
--- a/sc/source/core/tool/interpr4.cxx
+++ b/sc/source/core/tool/interpr4.cxx
@@ -3544,11 +3545,12 @@ bool ScInterpreter::SetSbxVariable( SbxVariable* pVar, const ScAddress& rPos )
                 }
                 break;
             default :
-                pVar->PutDouble( 0.0 );
+                pVar->PutEmpty();
         }
     }
     else
-        pVar->PutDouble( 0.0 );
+        pVar->PutEmpty();
+
     return bOk;
 }
Comment 13 Xisco Faulí 2017-10-02 21:55:05 UTC
Thanks.
Please, do not forget to submit it, otherwise, your work will be in vain
Comment 14 Commit Notification 2017-11-20 16:08:54 UTC
Matti Lehtonen committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=3ed87ba36e59f774917eac49389770da404cbc06

tdf#89216 forward empty cells as empty to BASIC instead of 0.0

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Eike Rathke 2017-11-20 16:19:01 UTC
Not sure if this might have some side effects not yet aware of. In Basic, arithmetic calculations take an empty SbxVar as 0 and string operations as empty string, as they should, so in that context we're good.
Comment 16 Xisco Faulí 2017-12-25 14:42:14 UTC
A polite ping to Matti Lehtonen &  Eike Rathke: is this bug fixed? if so, could you please close it as RESOLVED FIXED ? Thanks
Comment 17 Eike Rathke 2018-03-02 20:20:41 UTC
Let's call this fixed.