Bug Hunting Session
Bug 63546 - EDITING:In Calc when writing direction of cell is changed to RTL, text remains left-aligned.
Summary: EDITING:In Calc when writing direction of cell is changed to RTL, text remain...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
4.0.2.2 release
Hardware: Other All
: medium normal
Assignee: Eike Rathke
URL:
Whiteboard: target:4.2.0
Keywords:
Depends on:
Blocks: RTL-CTL
  Show dependency treegraph
 
Reported: 2013-04-15 09:39 UTC by navin patidar
Modified: 2013-11-16 00:22 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
test doc (8.72 KB, application/vnd.oasis.opendocument.spreadsheet)
2013-04-18 13:59 UTC, Lior Kaplan
Details
screenshot of regression with range selection (75.25 KB, image/png)
2013-04-18 17:40 UTC, Lior Kaplan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description navin patidar 2013-04-15 09:39:39 UTC
Steps to reproduce bug. 
1.  open calc. 
2.  Write some text in cell.
3.  Now change text writing direction from LTR to  RTL.

Result :
Text remains left aligned.
Comment 1 abdulmajeed 2013-04-16 11:26:28 UTC
conform with master on Ubuntu
Comment 2 Lior Kaplan 2013-04-17 06:08:00 UTC
Confirmed on 4.0.2.2.

Notice the fix should only be applied to left/right alignment, not touching center / justified alignments.
Comment 3 navin patidar 2013-04-17 08:27:42 UTC
i've submitted a patch for review.


https://gerrit.libreoffice.org/#/c/3424/
Comment 4 Commit Notification 2013-04-18 09:36:20 UTC
navin patidar committed a patch related to this issue.
It has been pushed to "master":

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

fix fdo#63546 : set appropriate alignment when writing direction is changed.



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 5 Lior Kaplan 2013-04-18 13:59:22 UTC
Created attachment 78181 [details]
test doc

I'm reopening the issue as the patch creates a regression. I would like to revert the patch if there isn't a quick fix.

The regression is the changing the directionality aligns all the fields to the right, regardless of their original alignment (left, center, right, justify). 

Using undo, I can see that it's actually two operations - changed directionality and then overriding the alignment to the right.
Comment 6 Caolán McNamara 2013-04-18 14:58:31 UTC
The two operations appearing in the undo list could be fixed by bundling the undo actions together using EnterListAction and LeaveListAction I believe.

wrt the text direction change making the aligns all to the right, regardless of their original alignment (left, center, right, justify), are you using the actual pushed patch from the second gerrit update which doesn't do that for me, or the original unpushed patch ?
Comment 7 Lior Kaplan 2013-04-18 15:11:36 UTC
The pushed fix (16f508686c29bfa244ca6f81b5ab3bbaf5fef2a7), as notified above.
Comment 8 Commit Notification 2013-04-18 15:29:37 UTC
Caolan McNamara committed a patch related to this issue.
It has been pushed to "master":

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

Related: fdo#63546 bundle both changes together as one undo



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 9 Lior Kaplan 2013-04-18 17:15:50 UTC
(In reply to comment #5)
> The regression is the changing the directionality aligns all the fields to
> the right, regardless of their original alignment (left, center, right,
> justify). 

After some checks with Caolan it seems the "regression" is only when selecting a range of cells and changing directionality. On a single cell the fix works as expected.
Comment 10 Lior Kaplan 2013-04-18 17:40:08 UTC
Created attachment 78190 [details]
screenshot of regression with range selection
Comment 11 Commit Notification 2013-04-29 06:16:39 UTC
navin patidar committed a patch related to this issue.
It has been pushed to "master":

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

fdo#63546: set appropriate alignment when wrt direction of cells is changed.



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 Lior Kaplan 2013-05-22 16:48:06 UTC
Both commits verified (original problem + regression fix) on a build from master (4.1 branch point).
Comment 13 Ahmad Harthi 2013-06-09 07:49:13 UTC
*** Bug 65563 has been marked as a duplicate of this bug. ***
Comment 14 Commit Notification 2013-10-29 18:30:59 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

Revert "fdo#63546: set appropriate alignment when wrt direction of cells is changed."



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-10-29 18:31:17 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

Revert "Related: fdo#63546 bundle both changes together as one undo"



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-10-29 18:31:36 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

Revert "fix fdo#63546 : set appropriate alignment when writing direction is changed."



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 Eike Rathke 2013-10-29 18:38:42 UTC
Triggered by bug 70750 inspecting the code it turned out that the
changes for this bug did several things wrong:
* looping over individual cells of a large selected range results in
  slicing and merging ScAttrArray for each row in each column which is
  utterly slow (the reason for fdo#70750)
* instead of executing the changes in ScFormatShell calls should had
  been forwarded through ScTabViewShell to be passed down to ScViewFunc,
  ScDocFunc, ScDocument, ...
* setting hard attributes on all cells (except if already centered or
  block justified) is not wanted
  * value cells for example should stay automatically right justified if
    no justification is given yet
  * overwriting already existing left or right justification may not be
    what the user expects
  * applied styles' justification is overridden this way

Reverted all related changes on master, reopening this bug.
Comment 18 Eike Rathke 2013-10-30 19:28:53 UTC
I have a hard time to figure out how a solution should really look like,
sensible would be to do the left/right justification only during
rendering, and not set any justification attributes, but that again
would mean that the same file when loaded in different releases would be
rendered differently, which isn't good either.

Using justification attributes again would mean having to inspect the
cell type and act differently on text cells and value cells, then for
text cell (ranges) inspect centered or block attribution. All this could
slow down things again for large ranges even if we implemented it to use
already available chunks in cell storage and attribution. There's always
a worst case with large fragmentation..

I'm undecided.
Comment 19 Ady 2013-10-30 20:46:29 UTC
As a simple user, I admit I don't see the problem (currently testing in Calc 
Version: 4.1.3.2
Build ID: 70feb7d99726f064edab4605a8ab840c50ec57a , on Windows).

One thing is a RTL or LTR _direction_ of text (a paragraph in Writer, or text 
in a cell in Calc), and a different thing is the _alignment_ of such text.

I am "playing" with RTL and LTR directions and with right- or left- alignment 
in several combinations in Calc and in Writer, and I can set whichever 
combination I want. Perhaps I am missing something.

Regards,
Ady.
Comment 20 Ahmad Harthi 2013-10-31 09:22:38 UTC
(In reply to comment #19)
> As a simple user, I admit I don't see the problem (currently testing in Calc 
> Version: 4.1.3.2
> Build ID: 70feb7d99726f064edab4605a8ab840c50ec57a , on Windows).
> 
> One thing is a RTL or LTR _direction_ of text (a paragraph in Writer, or
> text 
> in a cell in Calc), and a different thing is the _alignment_ of such text.
> 
> I am "playing" with RTL and LTR directions and with right- or left-
> alignment 
> in several combinations in Calc and in Writer, and I can set whichever 
> combination I want. Perhaps I am missing something.
> 
> Regards,
> Ady.

I don't know if your build have the patch reverted, but you can see it in LO 3.6.x
Comment 21 Ady 2013-10-31 11:03:23 UTC
(In reply to comment #20)
> (In reply to comment #19)
> > As a simple user, I admit I don't see the problem (currently testing in Calc 
> > Version: 4.1.3.2
> > Build ID: 70feb7d99726f064edab4605a8ab840c50ec57a , on Windows).
> > 
> > One thing is a RTL or LTR _direction_ of text (a paragraph in Writer, or
> > text 
> > in a cell in Calc), and a different thing is the _alignment_ of such text.
> > 
> > I am "playing" with RTL and LTR directions and with right- or left-
> > alignment 
> > in several combinations in Calc and in Writer, and I can set whichever 
> > combination I want. Perhaps I am missing something.
> > 
> > Regards,
> > Ady.
> 
> I don't know if your build have the patch reverted, but you can see it in LO
> 3.6.x

My comment was directed more towards Eike Rathke, who probably knows about 
version 4.1.3.2 Build ID: 70feb7d99726f064edab4605a8ab840c50ec57a (on Windows) 
and whether some patch is applied to it. My point is that, in the version I am 
testing, there is no problem of mixing whichever text _direction_ with 
whichever _alignment_ I want; the result is adequate every time, both in Writer and in Calc.

Regards,
Ady.
Comment 22 Ady 2013-10-31 11:32:33 UTC
To clarify me prior comments:

1_ Click RTL -> the text direction is correctly set to RTL direction and simultaneously right-align.
2_ Click LTR -> the text direction is correctly set to LTR direction and simultaneously left-align.
3_ Whichever text direction is set (RTL or LTR), I can independently modify the alignment to whatever I want, without affecting the text direction.

Generally speaking, when RTL is used, a right-alignment would be desired too. Generally speaking, when LTR is used, a left-alignment text would be desired too. So, simultaneously changing the alignment when initially setting / changing the text direction seems reasonable. If a different alignment (including "justify" or "center") is desired, the user can change it too.

Perhaps I am missing some case where, instead of text, the changes affect other type of values (e.g. numbers) in a way that is not desired nor easily corrected by the user?

The reason I am mentioning all this is because the current "Target" in this bug is set to 4.1.0 and 4.2.0, while I am currently testing with adequate results in 4.1.3.2 (on Windows).

Regards,
Ady.
Comment 23 Eike Rathke 2013-10-31 12:29:20 UTC
As lined out in comment 17 the patches "solved" this in 4.1.x by applying hard left/right alignment attributes to all cells. You don't notice much wrong when clicking RTL writing direction on a selected range that previously didn't have a specific alignment, but if you select a range that includes not only text cells but also value cells and click LTR you'll notice immediately that then also value cells get left aligned, which most certainly is not wanted. Furthermore, applying these hard attributes overrides alignment originating from cell styles, even if that style already said the cell should be right aligned (for RTL), so modifying the alignment in the cell style will not have any effect anymore.
Comment 24 Ady 2013-11-01 11:45:54 UTC
(In reply to comment #23)
> As lined out in comment 17 the patches "solved" this in 4.1.x by applying
> hard left/right alignment attributes to all cells. You don't notice much
> wrong when clicking RTL writing direction on a selected range that
> previously didn't have a specific alignment, but if you select a range that
> includes not only text cells but also value cells and click LTR you'll
> notice immediately that then also value cells get left aligned, which most
> certainly is not wanted. Furthermore, applying these hard attributes
> overrides alignment originating from cell styles, even if that style already
> said the cell should be right aligned (for RTL), so modifying the alignment
> in the cell style will not have any effect anymore.

Currently this bug #63546 Description says:
 "EDITING:In Calc when writing direction of cell is changed to RTL, text remains left-aligned."

so that's what I was mainly testing and reporting (i.e. text in cells, apply RTL direction, check whether the text remains left-aligned".

@Eike Rathke, I now understand what you are saying, and it may affect prior patches related to this bug report too; it was just not matching the current "Description", so I was missing your point.

Thank you and Regards,
Ady.
Comment 25 Eike Rathke 2013-11-15 16:03:45 UTC
A question for understanding: is it expected that when changing writing direction to RTL text should also be right aligned if it consists of non-RTL letters, e.g. is written in Latin script? Or should it be left aligned in that case?
Comment 26 Ahmad Harthi 2013-11-15 16:12:00 UTC
(In reply to comment #25)
> A question for understanding: is it expected that when changing writing
> direction to RTL text should also be right aligned if it consists of non-RTL
> letters, e.g. is written in Latin script? Or should it be left aligned in
> that case?

* RTL and LTR functions used to decide where to put weak characters and how to mix different directionality words.

* Alignment should follow the "document/parent object" directionality, if RTL all text should be aligned right by default... and so on.
Comment 27 Ahmad Harthi 2013-11-15 16:20:59 UTC
(In reply to comment #26)
> (In reply to comment #25)
> > A question for understanding: is it expected that when changing writing
> > direction to RTL text should also be right aligned if it consists of non-RTL
> > letters, e.g. is written in Latin script? Or should it be left aligned in
> > that case?
> 
> * RTL and LTR functions used to decide where to put weak characters and how
> to mix different directionality words.
> 
> * Alignment should follow the "document/parent object" directionality, if
> RTL all text should be aligned right by default... and so on.

To make it more clear there is a difference between applying RTL to text or to (document/parent object), if you apply RTL to text it hasn't to be aligned right, it should keep the previous alignment as is...
Comment 28 Eike Rathke 2013-11-15 16:50:16 UTC
This entire bug is only about the text alignment within one cell, not text portions or weak characters. Which is similar to paragraph level. Also, this is about the alignment that should result if the WRITING DIRECTION was set to RTL for a cell, which overrides the sheet's writing direction. Of course when no direction is set at cell level then the sheet's direction shall be considered.

Still I'd like to know what alignment should result if the writing direction is RTL (sheet or cell level) and the text for example starts with a Latin letter. Should the cell's alignment be right or left in that case (if no explicit alignment is set).
Comment 29 Faisal Menawer 2013-11-15 17:05:41 UTC
when writing direction changed to RTL the alignment should changed
to the right whatever the text was (Latin letter, arabic ...) except if the alignment set for center of justified.
Comment 30 Commit Notification 2013-11-15 23:47:19 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

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

made horizontal cell alignment depend on writing direction, fdo#63546



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.