Bug 62090 - Mid statement doesn't work as expected
Summary: Mid statement doesn't work as expected
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.0.1.2 release
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Noel Power
URL:
Whiteboard: BSA target:4.1.0 target:4.0.3
Keywords: regression
Depends on:
Blocks:
 
Reported: 2013-03-10 08:42 UTC by Zarko Zivanov
Modified: 2016-04-24 18:39 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Document ilustrating wrong Mid statement behaviour (14.03 KB, application/vnd.oasis.opendocument.spreadsheet)
2013-03-10 08:42 UTC, Zarko Zivanov
Details
The result of using Mid(s,5,10,"lazy") (43.41 KB, image/png)
2013-03-10 08:44 UTC, Zarko Zivanov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Zarko Zivanov 2013-03-10 08:42:27 UTC
Created attachment 76259 [details]
Document ilustrating wrong Mid statement behaviour

Problem description: 
In LibreOffice 3, for this Basic code:

s = "The lightbrown fox"
Mid(s,5,10,"lazy") 

the resulting string was "The lazy fox". In LibreOffice 4, string replacement isn't done correctly and works ONLY if replacement string is long EXACTLY as the part it replaces.

Steps to reproduce:
1. Try included ods file

Current behavior:
Mid statement replacement produces string with wong characters

Expected behavior:
Mid statement should replace string as in LibreOffice 3
Operating System: Ubuntu
Version: 4.0.1.1 rc
Last worked in: 3.5.4 release
Comment 1 Zarko Zivanov 2013-03-10 08:44:43 UTC
Created attachment 76260 [details]
The result of using Mid(s,5,10,"lazy")
Comment 2 Joel Madero 2013-03-18 17:15:47 UTC
updating version as you've said last working version was 3.5.4 release. We use version field to show first version we see problem, not latest it's been tested on (we use comments to say we've tested on newer version and see same problem)

Also this will not constitute a major bug (no loss of data, no crash, no memory issues). This is just a normal bug. Marking as NORMAL. 

Not confirming the issue as I want to see what it looks like using the last version it worked in order to see the difference.
Comment 3 Joel Madero 2013-03-18 17:40:47 UTC
Additional note for any QA personnel - I tried to bibisect this but see exact same behavior in earliest bibisect as I do with 4.0.0.3 release as well as 4.1 master :-/
Comment 4 Zarko Zivanov 2013-03-18 18:31:38 UTC
I'm not sure that 3.5.4 was last working version, but it was last form 3.x that I used.
Comment 5 Julien Nabet 2013-03-18 21:31:12 UTC
On pc Debian x86-64 with master sources updated yesterday, I reproduced the problem.

Noel: one for you?
Comment 6 Julien Nabet 2013-03-18 22:19:58 UTC
Noel, what about this patch? I tested it for this case and it works but perhaps I forgot something.

diff --git a/basic/source/runtime/methods.cxx b/basic/source/runtime/methods.cxx
index 5dc45ed..12dfff2 100644
--- a/basic/source/runtime/methods.cxx
+++ b/basic/source/runtime/methods.cxx
@@ -1266,7 +1266,7 @@ RTLFUNC(Mid)
                 {
                     aResultStr = aArgStr;
                     aResultStr.remove( nStartPos, nLen );
-                    aResultStr.insert( nStartPos, rPar.Get(4)->GetOUString().getStr(), nLen);
+                    aResultStr.insert( nStartPos, rPar.Get(4)->GetOUString().getStr(), std::min(nLen, rPar.Get(4)->GetOUString().getLength()));
                 }
 
                 rPar.Get(1)->PutString( aResultStr.makeStringAndClear() );
Comment 7 Joel Madero 2013-03-18 22:20:23 UTC
Julien - do you see this with 3.6? If not, I must be misunderstanding something, I'd like to get a bibisect if possible
Comment 8 Julien Nabet 2013-03-18 22:26:10 UTC
(In reply to comment #7)
> Julien - do you see this with 3.6? If not, I must be misunderstanding
> something, I'd like to get a bibisect if possible

Sorry Joel, I don't have anymore 3.6 or 4.0 sources (I focus on master sources). However I tested on 3.5.4 (Debian package version) and I didn't reproduce the bug.
Comment 9 Julien Nabet 2013-03-18 22:35:34 UTC
Also, I think this block should be common (before the if bCompatibility)
sal_Int32 nArgLen = aArgStr.getLength();
if( nStartPos + 1 > nArgLen )
{
    StarBASIC::Error( SbERR_BAD_ARGUMENT );
    return;
}
Because if not, Mid(s, 20, 10, "lazy") will have the same kind of effect.
Comment 10 Noel Power 2013-03-19 14:13:51 UTC
(In reply to comment #5)
> On pc Debian x86-64 with master sources updated yesterday, I reproduced the
> problem.
> 
> Noel: one for you?

I guess so, no idea what changed here, presumeably either aoo resync or String->OUString work 'cause I don't recall any other changes to this stuff since Libreoffice came into existence ( could be wrong about that though )
Comment 11 Noel Power 2013-03-19 15:33:34 UTC
(In reply to comment #6)
> Noel, what about this patch? I tested it for this case and it works but
> perhaps I forgot something.
> 
> diff --git a/basic/source/runtime/methods.cxx
> b/basic/source/runtime/methods.cxx
> index 5dc45ed..12dfff2 100644
> --- a/basic/source/runtime/methods.cxx
> +++ b/basic/source/runtime/methods.cxx
> @@ -1266,7 +1266,7 @@ RTLFUNC(Mid)
>                  {
>                      aResultStr = aArgStr;
>                      aResultStr.remove( nStartPos, nLen );
> -                    aResultStr.insert( nStartPos,
> rPar.Get(4)->GetOUString().getStr(), nLen);
> +                    aResultStr.insert( nStartPos,
> rPar.Get(4)->GetOUString().getStr(), std::min(nLen,
> rPar.Get(4)->GetOUString().getLength()));
>                  }
>  
>                  rPar.Get(1)->PutString( aResultStr.makeStringAndClear() );

yes, this looks good ( 'cause the old UniString/String used autocorrect that )
Comment 12 Noel Power 2013-03-19 15:35:55 UTC
(In reply to comment #9)
> Also, I think this block should be common (before the if bCompatibility)
> sal_Int32 nArgLen = aArgStr.getLength();
> if( nStartPos + 1 > nArgLen )
> {
>     StarBASIC::Error( SbERR_BAD_ARGUMENT );
>     return;
> }
> Because if not, Mid(s, 20, 10, "lazy") will have the same kind of effect.

I'd prefer not to change it just yet, I am not sure if changing it is correct, previously this scenario would result in the string being appended to the end of the string, that behaviour has now changed ( it's another silent correction that UniString/String used to do )
Comment 13 Commit Notification 2013-03-19 15:50:04 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=14d452d0c2874f47f0b6ac1cd1c725195d5d69a9

fix for fdo#62090 Mid function regression



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 14 Commit Notification 2013-03-19 16:02:56 UTC
Noel Power committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0054b438013ef412c2df3b7ebe93dd1935ec07af

follow on fix for fdo#62090 ensure out of range startpos is handled



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 Commit Notification 2013-04-16 12:42:05 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-4-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=69371d3fae481adb2b21701952570f829fcd8311&h=libreoffice-4-0

fix for fdo#62090 Mid function regression


It will be available in LibreOffice 4.0.3.

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 16 Commit Notification 2013-04-16 12:49:08 UTC
Noel Power committed a patch related to this issue.
It has been pushed to "libreoffice-4-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=df4b1b9be4f818ef912bfb58b514bab1acdd6e71&h=libreoffice-4-0

follow on fix for fdo#62090 ensure out of range startpos is handled


It will be available in LibreOffice 4.0.3.

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 17 Markus Mohrhard 2013-06-02 03:17:07 UTC
@Noel: Is this bug fixed?
Comment 18 Noel Power 2013-06-04 09:09:23 UTC
(In reply to comment #17)
> @Noel: Is this bug fixed?

yup, should be
Comment 19 Jean-Baptiste Faure 2016-04-24 18:39:50 UTC
Version set from description.

Best regards. JBF