Bug 148868 - Quirk when changing the first character after a picture in a table
Summary: Quirk when changing the first character after a picture in a table
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
6.4.0.3 release
Hardware: All All
: medium normal
Assignee: Michael Stahl (allotropia)
URL:
Whiteboard: target:7.5.0 target:7.3.5 target:7.4....
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2022-04-30 16:07 UTC by Adalbert Hanßen
Modified: 2022-08-10 22:23 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Playground with screenshots, can be used to reproduce the bug (456.32 KB, application/vnd.oasis.opendocument.text)
2022-04-30 16:07 UTC, Adalbert Hanßen
Details
Example of the issue with table (270.72 KB, application/vnd.oasis.opendocument.text)
2022-04-30 18:09 UTC, Telesto
Details
Example file (270.23 KB, application/vnd.oasis.opendocument.text)
2022-04-30 18:09 UTC, Telesto
Details
Bibisect log (3.22 KB, text/plain)
2022-04-30 19:28 UTC, Telesto
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Adalbert Hanßen 2022-04-30 16:07:16 UTC
Created attachment 179863 [details]
Playground with screenshots, can be used to reproduce the bug

See the attached file with screenshots. The file can be used to reproduce the bug.
Comment 1 Telesto 2022-04-30 18:09:11 UTC
Created attachment 179864 [details]
Example of the issue with table

Select "Hello' and try to replace with World by select 'Hello' (or deleting it with backspace.
Comment 2 Telesto 2022-04-30 18:09:50 UTC
Created attachment 179865 [details]
Example file

Same issue without table, if the document being empty except for image with some line below
Comment 3 Telesto 2022-04-30 18:13:30 UTC
Also in
Version: 6.4.0.0.beta1+ (x64)
Build ID: 20be5cd0bdc57d812bf34a2debfe48caa51de881
CPU threads: 4; OS: Windows 6.3 Build 9600; UI render: default; VCL: win; 
Locale: nl-NL (nl_NL); UI-Language: en-US
Calc: CL

but still OK with 
Version: 6.3.7.0.0+ (x86)
Build ID: 726535ec30f12697ceccd2f0640d9371a64dc5bd
CPU threads: 4; OS: Windows 6.3; UI render: default; VCL: win; 
Locale: nl-NL (nl_NL); UI-Language: en-US
Calc: CL
Comment 4 Telesto 2022-04-30 19:28:07 UTC
Created attachment 179866 [details]
Bibisect log

Bisected to
author	Michael Stahl <Michael.Stahl@cib.de>	2019-12-05 16:49:33 +0100
committer	Michael Stahl <michael.stahl@cib.de>	2019-12-06 14:44:42 +0100
commit e75dd1fc992f168f24d66595265a978071cdd277 (patch)
tree 1e1870aac20c62254eb5b3b0f64711ea74730d54
parent 35191846feb19751e247cd228d7dcc6ddfdf2c8b (diff)
tdf#121300 sw: consistent fly at-pargraph selection
As a follow-up to commit 28b77c89dfcafae82cf2a6d85731b643ff9290e5, add
IsSelectFrameAnchoredAtPara() function and use it to harmonize at-para
fly selection across all relevant operations:

* CopyImpl:
  - it had a pre-existing bugs that would lose a fly anchored to the
    2nd (1st fully selected) node of a redline
  - remove a bunch of code for finding the last node of the body content,
    which doesn't matter for the remaining at-fly checks
  - flys that already existed at the insert position need to have their
    anchors corrected

* DeleteRange:
  - get rid of the bDelFwrd checks

* MoveRange:
  - the ALLFLYS flag would be obsoleted by the new selection, were it
    not for the writerfiler "special hack", see below

* also adapt A11y and UI selection, SwRootFrame::CalcFrameRects()

The selection behavior is changed:

* the bDelFwrd case is quite odd, some code was checking whether a
  deletion was "forward" or "backward" and removing only the flys at the
  point node while retaining the flys at the mark node; this worked in a
  very non-obvious way relying on sw_GetJoinFlags actually calling
  Exchange() on the cursor, and then SwUndoDelete stored it in
  m_bJoinNext, but it turns out that only SwUndoMove also has a
  m_bJoinNext and it's dead, and no other Undo has such a flag, so this
  only worked for "delete". It's not obvious what the value of this is,
  let's just ignore the "direction".

* Selections exclude the start and end position except if it's a fully
  selected node or at the start or end of a section (i.e. Ctrl+A should
  also select every at-para fly).

* An exception is made in case the selection looks like it's a
  backspace/delete joining 2 paragraphs; flys are not deleted in that
  case because it seemed annoying (and as it happens, Word does not
  appear to delete flys in that case), particularly if both of the nodes
  are already empty. This is done with a heuristic, it's conceivable to
  pass down some flag from DelLeft()/DelRight() but probably this is
  good enough.

A special hack is needed to keep writerfilter happy for now; it likes to
anchor flys at nodes which it then deletes in RemoveLastParagraph(),
which likely could be improved there.  The ALLFLYS usage in
SwRangeRedline::MoveFromSection() could be removed (because the
end-of-section check already handles the case) except for this, because
it turns out that the ODF import runs SetRedlineFlags with a temporarily
reset IsInXMLImport() flag because of its effect in thints.cxx, so
during the move IsSelectFrameAnchoredAtPara() can't check it.

tdf#108124 scenario works better, now everything that's selected is both
copied and deleted.

Fixes the problem where an at-para fly at the 2nd node of a redline
where the 1st node is partially deleted was lost on ODF export.
Comment 5 Telesto 2022-04-30 19:38:06 UTC
Adding CC: to Michael Stahl

The regression part might be debatable from design perspective; nit might well do what it was intended to do..

Regression keyword is only based on: did work differently previously and in this case 'better' compared to the current situation.
Comment 6 Michael Stahl (allotropia) 2022-06-14 11:29:30 UTC
i'm not sure if this is a bug or not, but with the fix done for bug 133957 it's now relatively easy to special-case this.

on the one hand i find it a little inconsistent when a deletion doesn't delete everything that's selected.

on the other hand, there are convenient shortcuts to select short pieces of text such as by double clicking on a word, in order to replace the text by typing, and in that case it's inconvenient to delete the image.

so let's try this out, interpret selection from the UI within one paragraph then typing similar to "Replace"; if selection spans multiple paragraph nothing changes.
Comment 7 Commit Notification 2022-06-14 11:30:20 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d72bee64a97650507d042f17846b6fc427b8434c

tdf#148868 sw: handle selection then Insert similar to Replace

It will be available in 7.5.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 8 Commit Notification 2022-06-14 19:29:26 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-7-3":

https://git.libreoffice.org/core/commit/67eec140556edb42280def88e987448aaa221c5e

tdf#148868 sw: handle selection then Insert similar to Replace

It will be available in 7.3.5.

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 9 Commit Notification 2022-06-15 06:28:39 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/4e8295638e68295d73b49ddb80e23c3509a49b3e

tdf#148868: sw_uiwriter3: Add unittest

It will be available in 7.5.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 10 Commit Notification 2022-06-15 08:48:43 UTC
Michael Stahl committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/74fab976cd1a37f035e1cc9fbbb0a5e84fcd20dc

tdf#148868 sw: handle selection then Insert similar to Replace

It will be available in 7.4.0.0.beta2.

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 11 Adalbert Hanßen 2022-06-22 07:12:10 UTC
I tested it with Version of 2022-06-22 07:53:03: 7.5.0.0.alpha0+ / LibreOffice Community
Build ID: 086055b0d7e44d1d07b3f23af55503e6a3924d87
CPU threads: 4; OS: Linux 5.4; UI render: default; VCL: gtk3
Locale: de-DE (de_DE.UTF-8); UI: en-US
Calc: threaded

with the sample I had originally supplied.

The example at the text "Then enter the new character you want to type (or the whole sentence, how much does not matter if it is at least one more character):" (on page 2 of 5) still goes wrong: I start marking the text Starting with the colon. As soon as I highlight the T at the beginning of the sentence, the image gets highlighted too.

The same failure holds for the equivalent example in a table on page 4.

The same holds for the example on page 5 of 5 around the text "Guess, what happens, if I press Shift-► now!"

Unfortunately I have to reopen this bug report, although some progress could be observed, but it is not a complete breakthrough.
Comment 12 Michael Stahl (allotropia) 2022-06-22 09:26:16 UTC
yes of course, that is to be expected: the special handling not to delete the text is only for inserting via the keyboard.

if you copy the selected text, the image will be copied; if you cut the selected text, the image will be copied and deleted - this isn't going to change.

this sort of inconsistency isn't ideal, but unfortunately we can't predict what the user is going to do next when displaying a selection.
Comment 13 Buovjaga 2022-07-03 12:11:25 UTC
Let's close again per the previous comment.
Comment 14 Gabor Kelemen (allotropia) 2022-08-10 22:23:19 UTC
Verified in

Version: 7.5.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: 4e2ce2a460458f17ee4360c45a2da2fc4b4d753e
CPU threads: 14; OS: Windows 10.0 Build 19044; UI render: Skia/Raster; VCL: win
Locale: en-US (hu_HU); UI: en-US
Calc: threaded

Replacing the word after the image does not remove the image.