Bug 45020 - [EDITING] selecting cells with Shift-Arrow with filtered rows/columns does not skip hidden rows/columns
Summary: [EDITING] selecting cells with Shift-Arrow with filtered rows/columns does no...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
3.5.0 RC1
Hardware: Other Linux (All)
: medium normal
Assignee: Markus Mohrhard
URL:
Whiteboard: target:4.2.0 target:6.3.0 target:7.4.0
Keywords: regression
: 48071 48273 48276 49882 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-20 23:21 UTC by Cor Nouws
Modified: 2022-03-30 19:40 UTC (History)
10 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Cor Nouws 2012-01-20 23:21:32 UTC
- spreadsheet
- in cell A1-A4:
apple
pear
melon
mango

- select A2 and A3
- format > Row > Hide
- select A1
- press Shift+down
 > cursor selects hidden cell
   need to press  Shift+down 3 times in order to select A4 as well
Comment 1 Cor Nouws 2012-01-20 23:22:48 UTC
annoyed me now and then the past months, but the bug is there already in 3.3.4
Not in OOo 3.3.0
Comment 2 Rainer Bielefeld Retired 2012-03-31 23:18:34 UTC
*** Bug 48071 has been marked as a duplicate of this bug. ***
Comment 3 OfficeUser 2012-04-01 04:58:34 UTC
Added regression keyword due this comment:
https://bugs.freedesktop.org/show_bug.cgi?id=48071#c3
Comment 4 Markus Mohrhard 2012-04-03 18:58:46 UTC
I'm not sure if this is really a bug.

Kohei explicitly removed this for Bug 33756.(http://cgit.freedesktop.org/libreoffice/core/commit/?id=4ac51b9cd2b37996542eaaf13a57fa5ececa6dab)

@Kohei: Any opinion on this bug?
Comment 5 Kohei Yoshida 2012-04-03 19:08:52 UTC
It's a bug.  Hidden cells should always be treated as if they don't exist when expanding selection.

Having said that, this cursor navigation code has become such a complex monster we need to refactor it so that we can guard it with a decent amount of unit tests.  Otherwise it would be extremely hard to avoid introducing another regression when fixing a bug (any bug) in this code.  I believe we talked about this briefly before when we were talking about some unit test areas (with me, you and Eike).

When expanding selection or navigating the cell cursor, we need to factor in:

1) hidden rows/columns
2) protected areas
3) merged cells
4) modifier keys (shift / ctrl / both)

So, one has to check all these possibilities when modifying the cursor navigation code.  And from my own experience it's way too easy to introduce regressions in this code...
Comment 6 Markus Mohrhard 2012-04-03 19:58:17 UTC
(In reply to comment #5)
> It's a bug.  Hidden cells should always be treated as if they don't exist when
> expanding selection.
> 
> Having said that, this cursor navigation code has become such a complex monster
> we need to refactor it so that we can guard it with a decent amount of unit
> tests.  Otherwise it would be extremely hard to avoid introducing another
> regression when fixing a bug (any bug) in this code.  I believe we talked about
> this briefly before when we were talking about some unit test areas (with me,
> you and Eike).

I remember something like that now. I'm just getting a bit old and start forgetting things ;)

I agree that this would need a real unit test and I think we can easily write one for that. I will look into it. I think in this case it even makes a lot of sense to write the tests now before actually implementing the code.

I fear that this code is not testable by ucalc because it needs vcl and the view part of sc. So our best shot on testing this is writing a real unit test. We should get a good test for this by mocking ScDocument (at least a part) and moving as much functionality for this behavior into an own class that has only dependencies to ScDocument.

> 
> When expanding selection or navigating the cell cursor, we need to factor in:
> 
> 1) hidden rows/columns
> 2) protected areas
> 3) merged cells
> 4) modifier keys (shift / ctrl / both)
> 
> So, one has to check all these possibilities when modifying the cursor
> navigation code.  And from my own experience it's way too easy to introduce
> regressions in this code...

I suspect i will have a lot of fun with this.
Comment 7 Kohei Yoshida 2012-04-03 20:29:33 UTC
You just need to separate the cell cursor navigation code from ScTabView into its own class, then set unit test against that class.  That way you won't need to have all those view classes instantiated in order to unit-test it.  That was the idea I had in mind...
Comment 8 Markus Mohrhard 2012-04-03 20:57:43 UTC
That is exactly what I'm doing. Everything else is bringing us back to linking against nearly all of LibO. And the remaining parts of sc that we can't remove from this part of the code can be easily mocked.
The mokcing objects can be shared with future similar unit tests. The unit test then only needs to compile this new class and link against the static library containing the mocked classes. For exmaple we will have no chance to remove the dependency to ScDocument in the new class but adding the real ScDocument into a unit test results in having nearly all of calc in the test.
Comment 9 Kohei Yoshida 2012-04-03 21:02:33 UTC
Gotcha.  Sounds like you are on the right track. :-)
Comment 10 Markus Mohrhard 2012-04-04 00:59:39 UTC
*** Bug 48276 has been marked as a duplicate of this bug. ***
Comment 11 Markus Mohrhard 2012-04-04 01:00:33 UTC
*** Bug 48273 has been marked as a duplicate of this bug. ***
Comment 12 Markus Mohrhard 2012-04-10 03:06:50 UTC
This one is getting nasty. I have a patch that fixes this bugs and opens another one.

We have more or less the same code in two places meant for the same feature behaving differently. I'm currently working on getting all into an own class that is responsible for all cursor movement and having a unit test with mocking for this.

This will take some time and will not make it into 3-5.
Comment 13 Cor Nouws 2012-04-10 03:52:20 UTC
(In reply to comment #12)

> This will take some time and will not make it into 3-5.

Though I do not touch it often, I still have a mouse that I can use.
So I can manage for now. 
I hope there will be a solution in 3.6 that makes you hapy too  :-)
Comment 14 Rainer Bielefeld Retired 2012-05-13 22:27:28 UTC
*** Bug 49882 has been marked as a duplicate of this bug. ***
Comment 15 Markus Mohrhard 2012-08-05 21:38:43 UTC
I have finally a patch for the row problems. One small mistake is still in that I will fix next week. Test is already in master and columns will come next.
Comment 16 Not Assigned 2012-08-05 23:02:46 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

skip hidden rows when moving with the cursor, fdo#45020
Comment 17 Markus Mohrhard 2012-08-05 23:06:33 UTC
This patch is for rows and should be tested very carefully. I hope I found all corner cases around hidden rows and the performance is still good enough.

If you test it and can report that it works correctly in master I can propose it for 3-6.
Comment 18 Not Assigned 2012-08-09 14:15:52 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

fix column navigation with CTRL + direction, fdo#45020
Comment 19 Markus Mohrhard 2012-08-09 14:21:29 UTC
Both rows and columns are now implemented.

Please check in a daily build and we can then think about integrating it into 3-6.
Comment 20 crxssi 2013-08-08 14:48:58 UTC
Is this stalled?  Is there a regression?  How is this marked "Resolved/Fixed" a year ago?????

I was just about to report this exact bug in LO 4.1....  When using shift-arrow in Calc to select across hidden rows and/or columns, it is not skipping the hidden areas.

This is a NIGHTMARE for spreadsheet users who have large hidden sections and use keyboard navigation...
Comment 21 Cor Nouws 2013-08-08 14:57:13 UTC
Hi Markus,

(In reply to comment #19)
> Both rows and columns are now implemented.
> 
> Please check in a daily build and we can then think about integrating it
> into 3-6.

Have not been able to check dailys (do not exist for my OS/CPU) but checking in Master (Build ID: b14688f07f2d2e1e53cf0b99bd2f7a055e2ddb3d): it is not resolved.

(sorry for not replying earlier - the recent comment pushed my attention to this one)
Comment 22 Markus Mohrhard 2013-09-05 14:06:34 UTC
I need more details than a simple it does not work. It obviously works for my test cases otherwise I would not have closed the bug report.

Without detailed instructions I consider this bug fixed.
Comment 23 OfficeUser 2013-09-05 18:40:48 UTC
Hi Markus.

"fix column navigation with CTRL + direction, fdo#45020"

From the notes of your patch I understand that you have fixed something related to CTRL key. But this issue is related to SHIFT key.

Please try the attchement
https://bugs.freedesktop.org/attachment.cgi?id=59248
of my duplicate issue
https://bugs.freedesktop.org/show_bug.cgi?id=48071
to reproduce the problem.

I see that is not fixed in
Version: 4.1.1.2
Build ID: 7e4286b58adc75a14f6d83f53a03b6c11fa2903

(But I don't know if your patch already is included there.)
Comment 24 crxssi 2013-09-05 21:38:10 UTC
(In reply to comment #23)

> Please try the attchement
> https://bugs.freedesktop.org/attachment.cgi?id=59248
> of my duplicate issue
> https://bugs.freedesktop.org/show_bug.cgi?id=48071
> to reproduce the problem.

This example is a good one.  Clear and concise.  The problem is still present when I test in:

* LO 4.0.4.2 under Mageia Linux (Mageia build)
* LO 4.1.1.2 under RHEL 6.1 Linux (vanilla official build)
Comment 25 Commit Notification 2013-09-27 15:53:19 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

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

ignore hidden row/columns when navigating, fdo#45020



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 26 Tomaz Vajngerl 2013-10-13 17:25:13 UTC
Tested and it works correctly also when selecting.
Comment 27 Commit Notification 2019-01-02 17:04:43 UTC
Zdeněk Crhonek committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/+/5cca6d1daf7b25a1e134beaa77073e8ba7dfbf05%5E%21

uitest for bug tdf#45020

It will be available in 6.3.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 28 raal 2019-12-07 15:21:29 UTC
The test exist, set status to Verified.
Comment 29 Commit Notification 2022-03-30 19:40:40 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/551141c90f969421be77d9dc90a8feac791267ab

tdf#45020: sc: move UItest to CppUnittest

It will be available in 7.4.0.

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

Affected users are encouraged to test the fix and report feedback.