Bug 90447 - vba calls to end(xlup) and find change cell/sheet focus
Summary: vba calls to end(xlup) and find change cell/sheet focus
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: BASIC (show other bugs)
Version:
(earliest affected)
4.0.0.0 beta1
Hardware: Other All
: medium minor
Assignee: Justin L
URL:
Whiteboard: target:5.0.0
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2015-04-04 11:01 UTC by Justin L
Modified: 2015-12-17 08:51 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
xls macro that demonstrates sheet focus change (28.00 KB, application/vnd.ms-excel)
2015-04-04 11:01 UTC, Justin L
Details
Testcase included in the fix. (2.18 MB, application/vnd.ms-excel)
2015-04-07 14:39 UTC, Justin L
Details
testcase: on excel the cell selection does not change. In LO find selects the found cell. (45.50 KB, application/vnd.ms-excel)
2015-04-07 17:41 UTC, Justin L
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Justin L 2015-04-04 11:01:22 UTC
Created attachment 114610 [details]
xls macro that demonstrates sheet focus change

When calling a function like Sheets("mySheet")...End(xlup), the UI switch to mySheet.   In Excel, you need to add .Select to "jump" to that sheet/cell.  It also worked like Excel in LO 3.6.

977cf448a89278afffc3dd6ece1dea3d0d695345 is the first bad commit
commit 977cf448a89278afffc3dd6ece1dea3d0d695345
Author: Bjoern Michaelsen <bjoern.michaelsen@canonical.com>
Date:   Mon Dec 10 12:09:25 2012 +0000

    source-hash-cbc44df67cfd13849f3de85edcdd39b5fec8b06c
    
    commit cbc44df67cfd13849f3de85edcdd39b5fec8b06c
    Author:     Ricardo Montania <ricardo@linuxafundo.com.br>
    AuthorDate: Thu Sep 13 12:17:40 2012 -0300
    Commit:     Olivier Hallot <olivier.hallot@alta.org.br>
    CommitDate: Fri Sep 14 00:00:16 2012 +0000

...
# bad: [977cf448a89278afffc3dd6ece1dea3d0d695345] source-hash-cbc44df67cfd13849f3de85edcdd39b5fec8b06c
git bisect bad 977cf448a89278afffc3dd6ece1dea3d0d695345
# good: [bc819bc0c4d8592212f84069eb7f65e539517166] source-hash-d9412fb4755377b8358a46a249cfe29a22ea9451
git bisect good bc819bc0c4d8592212f84069eb7f65e539517166

Last good date:  	2012-09-11 05:31:43 (GMT)

Likely culprit (compiling failure, so I can't confirm):
author	Noel Power <noel.power@suse.com>	2012-09-11 07:48:02 (GMT)
committer	Michael Meeks <michael.meeks@suse.com>	2012-09-12 11:49:49 (GMT)
commit	4597483e00bffcc4e30d379dcf6fad42bc565e56 (patch)
tree	b590428b0e3ae3c7056e6c18f008d7179dc5ba38
parent	c414499bbd456389ac6cacf677327bff9e6b43f9 (diff)
targetted VBA re-work.
Comment 1 raal 2015-04-04 14:12:08 UTC
LibreOffice 3.5.0 
Build ID: d6cde02
and
Version: 4.4.3.0.0+
Build ID: 3eba5eb1774ab621a1f0f4dcc7e82cce6c025b0a
TinderBox: Linux-rpm_deb-x86_64@46-TDF, Branch:libreoffice-4-4, Time: 2015-03-27_09:07:12
and
version 3.6.0alpha1+ (Build ID: 9afb6e)
and
Version: 4.3.0.0.alpha1+
Build ID: c15927f20d4727c3b8de68497b6949e72f9e6e9e

in these versions after run of macro focus jump to Sheet2  (-It should NOT change the focus to Sheet2  (that is the bug being reported here)) after the open file.

Clicking into the file (Clicking anywhere on this sheet will run a "find" macro.  It also should NOT change the focus to sheet2, and report the same number.)

version 3.6.0alpha1+ (Build ID: 9afb6e) -  doesn't jump into sheet2

Version: 4.4.3.0.0+
Build ID: 3eba5eb1774ab621a1f0f4dcc7e82cce6c025b0a - jump into the sheet2

Need to check with excel, so leaving unconfirmed for now.
Comment 2 Justin L 2015-04-04 19:24:53 UTC
sc/source/ui/vba/vbarange.cxx:  ScVbaRange::End() doesn't save/restore the ActiveSheet - only the ActiveCell.  Adding a couple of lines to restore the activeSheet fixes .End(xlup).
Comment 3 Julien Nabet 2015-04-05 09:35:12 UTC
Justin L: would you be interested in submitting a patch to review?
Comment 4 Justin L 2015-04-06 04:48:18 UTC
I want to see if I can re-write it - so that it doesn't ->Select() ANYTHING.  That would stop the screen from jumping around (switching between sheets) which looks terrible.
Comment 5 Justin L 2015-04-06 19:31:26 UTC
suggested patch submitted https://gerrit.libreoffice.org/15175
Comment 6 raal 2015-04-07 09:22:16 UTC
(In reply to raal from comment #1)

> 
> Need to check with excel, so leaving unconfirmed for now.

Excel 2010 - as reporter say, no jumps when open file nor click on first sheet. Setting to new.
Comment 7 Justin L 2015-04-07 14:39:52 UTC
Created attachment 114669 [details]
Testcase included in the fix.

A complete re-write of this function is really needed to work properly, because all of the current code DEPENDS on the state of ActiveCell to work, and so produces a lot of visual noise. However, since a complete re-write is way too complicated for me at this stage, I'll stick with a functional fix.  Perhaps someone else will be kind enough to complete the TODO that already existed for this function.

The fix includes a testcase that fairly exhaustively tests the ::end() function, ensuring that the re-write will not break anything.  After compiling, the test is run by compiling "make sc.subsequentcheck".
Comment 8 Justin L 2015-04-07 17:41:40 UTC
Created attachment 114674 [details]
testcase: on excel the cell selection does not change.  In LO find selects the found cell.

For the other half of this bug report, a proposed patch for the find() function can be found here: https://gerrit.libreoffice.org/15189

I think this is safe to do this fix.  I tested on Excel 2003 and 2013, and Raal tested 2010 - find does not affect the ActiveCell in MSOffice.

I looked in LO code for anything that might depend on the find function, but didn't find anything.  So I think that changing this will only "break" VBA code that was designed for running on LO, but not on Excel - and that is not very likely.  In any case, if this breaks anyone's macro, they were depending on bad behaviour.  The code can be modified to add a .Select call to the end and get it working again easily enough.

Although I would like to use this command, I think I'll just leave it in the 5.0 development branch and not push it into 4.4, just to allow lots of time for someone to dispute my change.
Comment 9 Julien Nabet 2015-04-07 17:49:27 UTC
I put Noel and Uray in cc since they're the Basic experts (see https://wiki.documentfoundation.org/FindTheExpert) but it seems they've been on other subjects this last year (unless I missed something, in this case, sorry :-()

Anyway, don't hesitate to ping dev mailing list (see http://nabble.documentfoundation.org/Dev-f1639786.html) or go to IRC (search "IRC" there https://wiki.documentfoundation.org/Development) if needed.

As for me, I can't help here, I just try to contribute when it's enough "low hanging fruit" for me.
Comment 10 Commit Notification 2015-04-08 13:37:24 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=15e758d199e0d7305e9587cd609f494f9ad55285

tdf#90447 vba end() needed to restore activesheet too.

It will be available in 5.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 11 Commit Notification 2015-04-14 17:20:24 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

tdf#90447 vba find() should NOT select the found cell.

It will be available in 5.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 12 Robinson Tryon (qubit) 2015-12-17 08:51:49 UTC Comment hidden (obsolete)