Bug 148810 - EDITING: Undo doesn't restore the bullet ( PPTX/PPT )
Summary: EDITING: Undo doesn't restore the bullet ( PPTX/PPT )
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: medium normal
Assignee: Justin L
URL:
Whiteboard: target:7.5.0 target:7.6.0
Keywords: bibisected, bisected
: 113759 (view as bug list)
Depends on:
Blocks: PPTX-Paragraph
  Show dependency treegraph
 
Reported: 2022-04-26 20:20 UTC by Xisco Faulí
Modified: 2023-02-22 11:29 UTC (History)
8 users (show)

See Also:
Crash report or crash signature:


Attachments
PPTX sample file (20.63 KB, application/vnd.openxmlformats-officedocument.presentationml.presentation)
2022-04-26 20:20 UTC, Xisco Faulí
Details
ODP sample file ( good behaviour ) (10.87 KB, application/vnd.oasis.opendocument.presentation)
2022-04-26 20:21 UTC, Xisco Faulí
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2022-04-26 20:20:55 UTC
Created attachment 179790 [details]
PPTX sample file

Steps to reproduce:
1. Open attached document
2. Put the cursor before 'Hello'
3. Delete the bullet
4. Undo

-> The bullet is not restored. This happens with pptx files but not with odp files

Reproduced in


and

Version: 6.4.0.0.alpha1+
Build ID: 9bc848cf0d301aa57eabcffa101a1cf87bad6470
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3; 
Locale: es-ES (es_ES.UTF-8); UI-Language: en-US
Calc: threaded
Comment 1 Xisco Faulí 2022-04-26 20:21:20 UTC
Created attachment 179791 [details]
ODP sample file ( good behaviour )
Comment 2 Xisco Faulí 2022-04-26 20:21:36 UTC
Reproduced in

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: f1dc51468137971295393ab438b57fe5f77c1420
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3
Locale: es-ES (es_ES.UTF-8); UI: en-US
Calc: threaded
Comment 3 Xisco Faulí 2022-04-26 20:22:33 UTC
At least in

Version: 6.0.0.0.alpha1+
Build ID: 6eeac3539ea4cac32d126c5e24141f262eb5a4d9
CPU threads: 8; OS: Linux 5.10; UI render: default; VCL: gtk3; 
Locale: es-ES (es_ES.UTF-8); Calc: group threaded

the bullet is restored after two undos. Setting as regression
Comment 4 Ezinne 2022-04-28 11:34:50 UTC
Reproducible in:

Version: 7.4.0.0.alpha0+ / LibreOffice Community
Build ID: 5453f75a1e682992f3a725781bb563b8cc76cf1b
CPU threads: 8; OS: Linux 5.13; UI render: default; VCL: gtk3
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded
Comment 5 raal 2022-05-02 20:20:38 UTC
This seems to have begun at the below commit.
Adding Cc: to Tamás Zolnai; Could you possibly take a look at this one?
Thanks
 0c16af003f631594a0b58be9d066b5f14c4fc542 is the first bad commit
commit 0c16af003f631594a0b58be9d066b5f14c4fc542
Author: Jenkins Build User <tdf@pollux.tdf>
Date:   Fri Dec 22 08:56:22 2017 +0100

    source 02ea9bc36ab47d68940da55f5012677dfaf0a8b8  

https://git.libreoffice.org/core/+/02ea9bc36ab47d68940da55f5012677dfaf0a8b8
    tdf#114613: Repair function does not work after opening PPTX file
Comment 6 Justin L 2022-07-23 12:36:53 UTC
Interesting - it was the opposite way around in 4.2 - 5.2.
ODP was fixed in 5.3 (a missing screen redraw) by
commit ee33745ced5ae12f9ae7735fac16a7298ccae474
Author: Noel Grandin on Mon Oct 24 14:12:12 2016 +0200
    tdf#103334 - EDITING: Undo on bullet point style
    
    I have checked the normal model and the editing model after UNDO, and
    all seems to be well, this is purely a rendering/lack-of-invalidation
    issue.

Confirmed that PPTX broke in 6.1 with comment 5's commit.
Prior to that, an undo chain included:
-rename shape
-delete outline text (Click to add Text)
-delete title text (Click to add Title)
...
Comment 7 Justin L 2022-07-23 17:20:50 UTC
Tamás' commit is too "on target" to just be coincidence, but reverting all UndoManager stuff in this file doesn't "fix" the problem. I'd guess it just exposed some other abnormality.
Comment 8 Justin L 2022-07-23 17:33:48 UTC
The same problem occurs with PPT - already in LO 3.5 so assuming OOo inherited.
Comment 9 Justin L 2022-07-26 12:07:21 UTC
I noticed that "when it worked" it required TWO undo steps to get the bullet back, while only one undo is possible now.

In debugging this, I see that ODP deletes two paragraph attributes (EE_PARA_OUTLLEVEL and EE_PARA_BULLETSTATE) while PPT deletes 8 attributes (including EE_PARA_NUMBULLET and EE_PARA_BULLETSTATE)

Problem likely caused by commit 88c1c1fa45b6d25694ac805be5258ee7190d6dd6
Author: Rüdiger Timm on Fri Jun 6 11:29:34 2008 +0000
where in editeng/source/editeng/editeng.cxx on a KEY_BACKSPACE we "hide" the BULLETSTATE before starting an undo.


But it also is related to Outliner::ParaAttribsChanged. For some reason the level (despite being at para depth of 0) is never specified as an OUTLLEVEL property and thus is lost in this Outliner reset. And there is also an undo-related OutlinerUndoChangeDepth - so depth really has some funkiness to it...
Comment 10 Justin L 2022-07-26 18:53:07 UTC
Based on my debugging, I figured that explicitly setting OUTLLEVEL when importing a NUMBULLET would work (oox/source/drawingml/textparagraphproperties.cxx). I was a bit surprised that it didn't work with a zero. However, it DOES work with a higher number. Zero should not be special. Either a void or a -1 is "special".
Comment 11 Justin L 2022-07-26 21:52:54 UTC
Oh great. It appears the reason "zero" doesn't work is because the depth is already zero - so it doesn't create a RES_PARA_OUTLLEVEL.

I think the reason it loses the property is because WID_NUMLEVEL (PROP_NumberingLevel) is not considered a bParaAttrib because it is less than EE_PARA_START, and thus is easily thrown out. This stuff is ridiculously complex.

I can "fix" it by forcing a full setDepth in Outliner::SetDepth()
-    if ( nNewDepth == pPara->GetDepth() )
-       return;
Comment 12 Justin L 2022-07-28 14:05:11 UTC
The issue does not at all seem to be with numbering visibility: EE_PARA_BULLETSTATE[4017], aka NumberingIsNumber which has a SfxItemKind::StaticDefault of true (and so the numbering is hidden only if there is a property specifically hiding it.)

The issue is always the mismatch of Para.GetDepth() and EE_PARA_OUTLLEVEL. Functions are setting Para Depth without providing a corresponding paragraph property. When the depth is -1 (the default), then no bullet is displayed.

(In reply to Justin L from comment #10)
> I figured that explicitly setting OUTLLEVEL when importing a NUMBULLET would work
Well, I was correct except that when I thought I was setting the property, the helper function only set the depth. So the PPTX fix is to change the helper to also fall back to the normal method of setting a property. This involved using the REAL EE_PARA_OUTLLEVEL instead of the fake WID_NUMLEVEL. http://gerrit.libreoffice.org/c/core/+/137569

For PPT, it is more easily done. http://gerrit.libreoffice.org/c/core/+/137543
Comment 13 Commit Notification 2022-07-28 23:46:20 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

tdf#148810 ppt import: Depth set by EE_PARA_OUTLLEVEL

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 14 Commit Notification 2022-08-02 06:15:48 UTC
Justin Luth committed a patch related to this issue.
It has been pushed to "master":

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

tdf#148810 pptx import: Depth set by EE_PARA_OUTLLEVEL

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 15 Gerald Pfeifer 2022-12-13 21:32:40 UTC
Happy to confirm this as resolved with

Version: 7.6.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 01c9c971e43782800ebf63acc763a7e7fba096c1
CPU threads: 8; OS: Linux 6.0; UI render: default; VCL: gtk3
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded

One issue remains (which it appears is not triggered by this fix,
mind): after Undo the cursor position jumps from the very beginning
of the list item to the very end. I filed bug #152500 for that.
Comment 16 Commit Notification 2023-01-30 18:24:34 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/2c99e7737ef2b536a4d9f28c3ddd37fe6afa9ceb

tdf#148810: sd: move UItest to CppUnittest

It will be available in 7.6.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 17 Xisco Faulí 2023-02-22 11:23:26 UTC
*** Bug 113759 has been marked as a duplicate of this bug. ***