Bug 70750 - RTL: RTL crash when selecting whole worksheet
Summary: RTL: RTL crash when selecting whole worksheet
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.1.1.2 release
Hardware: All All
: medium normal
Assignee: Eike Rathke
URL:
Whiteboard: BSA target:4.1.4
Keywords:
Depends on:
Blocks: mab4.1
  Show dependency treegraph
 
Reported: 2013-10-22 07:48 UTC by Michael
Modified: 2013-11-21 14:34 UTC (History)
4 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 Michael 2013-10-22 07:48:10 UTC
Problem description: 
Clicking the RTL control for cell format for a large number of cells makes calc unresponsive. Working on CENTOS 6.4 fully updated.

Steps to reproduce:
1. Choose a small number of cells
2. Click the RTL cell format control is OK
3. Choose the whole worksheet (top leftmost cell, before the A1)
4. Click the RTL cell format control, and calc becomes unresponsive

Current behavior:
I guess calc is just taking along time to process the request when a very large number of cells is involved.

Expected behavior:
The bug was not present in previous versions. I would know as I work in RTL mode quite frequently.
Operating System: Linux (Other)
Version: 4.1.1.2 release
Comment 1 Caolán McNamara 2013-10-22 09:17:22 UTC
Broken between 4e3e171262aed0e52fa76158950d5be770249e80..9cd7f7aded9ba171bf91a8223e6e8a868aedd792
Comment 2 Caolán McNamara 2013-10-22 09:21:37 UTC
regression due to 706e3b8e43df94310b2fe8458da67875073a046c "fdo#63546: set appropriate alignment when wrt direction of cells is changed"
Comment 3 Caolán McNamara 2013-10-22 09:24:41 UTC
Well, its clear why its slow, its iterating over every cell. Is there a common pattern in calc to handle these all-cells cases.
Comment 4 Michael 2013-10-22 09:37:59 UTC
(In reply to comment #3)
> Well, its clear why its slow, its iterating over every cell. Is there a
> common pattern in calc to handle these all-cells cases.

Yes, I was not aware this is a particular occurence, ie, "all-cells". It does not happen in other cases such as changing "all-cells" background to yellow, or setting borders in all cells, or changing font. Only with RTL direction setting it happens.
Comment 5 Markus Mohrhard 2013-10-22 09:38:55 UTC
Iterating like that through all the cells is wrong. This needs some improvements to make use of the data structures used for storing cell attributes.
Comment 6 Markus Mohrhard 2013-10-22 10:00:18 UTC
Actually we should have a method that applies the formatting to a whole range alredy which should be the better solution.
Comment 7 Eike Rathke 2013-10-22 10:27:54 UTC
Argh.. ScFormatShell is the wrong place anyway to handle it, these modifications should be done through ScTabViewShell instead, like all other requests of ScFormatShell are forwarded through that.

Unfortunately it's not just applying an attribute, but applying it conditionally. This might mean having to implement a whole new chain of function set handling this special case, through ScViewFunc, ScDocFunc, ScDocument, ScTable, ScColumn, which then could take advantage of applying the attribute to a block of rows instead of slicing and merging the attribute array for every row.
Comment 8 Eike Rathke 2013-10-29 18:41:02 UTC
Reverted all changes related to bug 63546 on master.
Comment 9 Eike Rathke 2013-10-30 19:33:54 UTC
So, with the removal of the problematic code in master this performance problem should be gone there, but still remains in 4-1.

Question is, should we revert it also there for 4.1.4 and thus remove the fix of bug 63546 given that implementation wasn't what actually might be needed?
Comment 10 Eike Rathke 2013-11-16 00:19:45 UTC
Pending review for 4-1 as https://gerrit.libreoffice.org/6691
Comment 11 Michael 2013-11-16 17:46:53 UTC
(In reply to comment #9)
> So, with the removal of the problematic code in master this performance
> problem should be gone there, but still remains in 4-1.
> 
> Question is, should we revert it also there for 4.1.4 and thus remove the
> fix of bug 63546 given that implementation wasn't what actually might be
> needed?

Will be very happy to check - it will be available in 4.1.4 or can a pre-master compiled X64 linux version be downloaded somewhere ?
Comment 12 Commit Notification 2013-11-20 09:36:16 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "libreoffice-4-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6ea1fb229ba3a273b373cad3c567e346597f799b&h=libreoffice-4-1

resolved fdo#70750 reverted inappropriate implementation of fdo#63546


It will be available in LibreOffice 4.1.4.

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 13 Eike Rathke 2013-11-21 14:34:37 UTC
(In reply to comment #11)
> Will be very happy to check - it will be available in 4.1.4 or can a
> pre-master compiled X64 linux version be downloaded somewhere ?

Dailies are available under http://dev-builds.libreoffice.org/daily/ an x86_64 under http://dev-builds.libreoffice.org/daily/master/Linux-rpm_deb-x86_64@46-TDF/