Issue 120015 - Application crash when undo "Graphic styles" action.
Application crash when undo "Graphic styles" action.
Status: CLOSED FIXED
Product: Impress
Classification: Application
Component: formatting
3.4.0
All All
: P3 normal (vote)
: 4.0.0
Assigned To: AOO issues mailing list
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 01:53 UTC by Ma Bingbing
Modified: 2013-07-11 13:35 UTC (History)
2 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---


Attachments
Fix patch for this issue (3.78 KB, patch)
2012-06-18 01:58 UTC, Ma Bingbing
jiazema: review?
Details | Diff
Partial solution with using refcounting of SfxStyleSheetBase (4.10 KB, patch)
2012-06-19 15:07 UTC, Armin Le Grand
no flags Details | Diff
Corrected patch (5.72 KB, patch)
2012-06-19 16:38 UTC, Armin Le Grand
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Ma Bingbing 2012-06-18 01:53:47 UTC
1. New a presentation document.
2. Create a Drawing object.
3. Press F11 and new a "Graphic style" with "Untitled1","Untitled2".
4. Select the Drawing object and apply "Unitled1" then apply style "Untitled2".
5. Press F11 then delete "Untitled1" from style list in "Style list" dialog.
6. Close "Style List" dialog then press Ctrl+Z, application crash.
Comment 1 Ma Bingbing 2012-06-18 01:58:59 UTC
Created attachment 78364 [details]
Fix patch for this issue

Root Cause:
When apply one style sheet to one object,the old style sheet will be saved in the SdrUndoAttrObj,but when the old style is deleted by user,this reference will not be changed,so the app will crash when undo.

Resolution:
Make the SdrUndoAtrrObj as one listener to the applied style sheet, if this style sheet is destroyed,we change the referenced style sheet to the default style sheet.
Comment 2 Armin Le Grand 2012-06-18 19:48:30 UTC
ALG: Shouldnt the deletion of the Style also create a undo action, so that the stack is complete and undoing would first restore the style, then reset the style at the SdrObject. I thik it is an error that no undo action gets created for deletingthe StyleSheet.
Comment 3 Li Feng Wang 2012-06-19 02:36:49 UTC
Change status to CONFIRMED.
Comment 4 Armin Le Grand 2012-06-19 14:57:06 UTC
ALG: I investigated, none of the actions on the opened styles dialog (F11) creates any undo/redo actions, this is missing implementation functionality and too much to create now for this fix.
I saw that SdrUndoAttrObj which is used to protocol SfxItemSet changes to SdrObjects uses SfxStyleSheet* to remember the used StyleSheets. One possibility would be to use the Strings Sheet.GetName(), GetFamily() to remember the Style instead and do a SfxStyleSheetBasePool::Find(..) on Undo/Redo to detect if the style still exists. The default style could then be used. This still allows no true undo functionality, settng the default style is not correct.
Interestingly SfxStyleSheet is derived from SfxStyleSheetBase and that is derived from comphelper::OWeakTypeObject, thus refcounted. It is possible to use

rtl::Reference< SfxStyleSheetBase > mxUndoStyleSheet;

instead of

SfxStyleSheet*				pUndoStyleSheet;

This allows to survive the removal of the StyleSheet from the StyleSheetPool. When undoing/redoing code is needed to check if it still is in the pool, add it if not and use it.
This allows full undo/redo even with removed StyleSheets; on undo a deleted StyleSheet is re-inserted to the StyleSheetPool, on redo it will stay there as if deletion did not happen. This is acceptable due to the fact that StyleSheet removal has no undo itself and that it will be removed anyways with the next save/load cycle (AFAIK only used StyleSheets survive, but not sure right now).

I will add the code so far as patch. It still triggers one assertion (not find parent), but parent works and is set. I'm pretty sure the assertion is wrong.
Comment 5 Armin Le Grand 2012-06-19 15:07:23 UTC
Created attachment 78394 [details]
Partial solution with using refcounting of SfxStyleSheetBase
Comment 6 Armin Le Grand 2012-06-19 16:36:54 UTC
ALG: The patch creates living documents which throw errors on saving. Investigation shows that in SfxStyleSheetBasePool::Remove the UNO API implementation is got and the object gets disposed, thus being invalid. Re-inserting such an object is of course erroneous.
I checked object deletion by watching SfxStyleSheetBase constructors and destructors, works well just by refcounting. It seems an error to dispose the object when it gets removed from the StyleSheetPool.
Also a small correction in SdrUndoAttrObj::ensureStyleSheetInStyleSheetPool. Insert the StyleSheet without parent and set the parent afterwards, this makes the assertion go away.
Adding a 2nd patch proposal with the mentioned changes.
Comment 7 Armin Le Grand 2012-06-19 16:38:19 UTC
Created attachment 78395 [details]
Corrected patch
Comment 8 Ma Bingbing 2012-06-20 01:30:20 UTC
(In reply to comment #2)
> ALG: Shouldnt the deletion of the Style also create a undo action, so that
> the stack is complete and undoing would first restore the style, then reset
> the style at the SdrObject. I thik it is an error that no undo action gets
> created for deletingthe StyleSheet.

  Thanks for you suggestion. It is a better solution and I am agree with you.
  I have some questions about adding undo action for deleting StyleSheet.
    1. If the "Style list" isn't closed, the undo action just re-add the style sheet to "Style sheet", it is all right.
    2. If the "Style list" is closed, we may have some choices:
       a. open the "Style list " dialog and re-add the style sheet
       b. re-add the style sheet and show nothing in the UI
       c. re-add the style sheet and reset the style at the SdrObject

  Do you have any ideas about them? Thanks!
Comment 9 Armin Le Grand 2012-06-20 09:06:38 UTC
ALG: Hi Ma Bingbing,

I would try to get as close to the behaviour as if the actions on the dialog would produce undo actions (what is just missing), thus the less possible change in behaviour will happen when someone would implement these missing undos. This means when undoing, first the (a) 'delete style' and then the (b) 'Style applied to SdrObject' would be undone.

Since we only have (b), (a) would be implicit and only if needed (if a deleted style was used by (b)). It will re-add the style to the model and apply it to the object, so everything is as before the undo.
If the dialog is open, it will show again the style. If not open, no visual feedback. This is exactly the same as if undo (b) existed and was executed.

Comitting the change now.
Comment 10 Armin Le Grand 2012-06-20 09:18:15 UTC
ALG: Comitted, done.
Comment 11 Li Feng Wang 2012-08-30 06:25:18 UTC
Have no crash according reproduce procedure.
Verified pass.
Comment 12 Li Feng Wang 2012-08-30 06:25:51 UTC
Verified pass on AOo trunk r1378003.