Issue 122676 - undo-redo works correctly only after double click
undo-redo works correctly only after double click
Status: VERIFIED FIXED
Product: Impress
Classification: Application
Component: ui
4.0.0-dev
PC Windows 7
: P3 critical (vote)
: 4.0.0
Assigned To: Armin Le Grand
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-03 15:53 UTC by Prachi
Modified: 2013-07-15 02:25 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---
jsc: 4.0.0_release_blocker+


Attachments
Attachment showing when undo button clicked 1 time (181.76 KB, image/png)
2013-07-03 15:53 UTC, Prachi
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description Prachi 2013-07-03 15:53:56 UTC
Created attachment 80993 [details]
Attachment showing when undo button clicked 1 time

Overview: while executing AOOTest-1429 Undo and Redo functionality works normally as it should be only if  Undo and Redo command button clicked 2 times.  

Steps to Reproduce:
  1) Open a new presentation.
  2) Insert a drawing object, or a text box, or a fontwork, and keep focus on it
  3) change fill type in area section of sidebar ,let say change it to gradient( default fill type is color )
  4) Press undo button.
  5) press redo button.

Actual Results: After pressing undo button area section showing gradient fill type.
                
Expected Results: After pressing undo button area section should show color fill type.

NOTES:
Instead of reverting back to color fill type, the fill type remains gradient doesn’t get reverted to its original fill type (color).when clicking 1 more time on undo button the fill type changes to color .
Same with redo command.Its also works correctly only if it clicked 2 times to get the expected result.

Build Date & Hardware:
Build: AOO 4.0 Rev. 1489073
OS: windows 7

Test case Reference :Test Case ID AOOTest-1429 :: Version : 1 
Area section - Impress - undo/redo operations
Comment 1 Edwin Sharp 2013-07-03 18:08:19 UTC
Not sure what is meaning of "area section of sidebar".
New presentation
Insert picture
Change contrast in sidebar to 45%
Undo (4% contrast appears, sidebar=properties-graphic)
Undo (0% original contrast appears, sidebar=properties-graphic)
Undo (blank slide appears, sidebar=properties-layouts)
Redo (picture appears, sidebar=properties-layouts)
Redo (contrast changes, sidebar=properties-layouts)
Redo (contrast changes, sidebar=properties-layouts)

Apart from the double click as reported by Prachi, sidebar is stuck during redo in properties-layouts.
It is expected it will change to properties-graphic.

Rev. 1491860 Debian
Comment 2 Armin Le Grand 2013-07-04 14:50:56 UTC
ALG: Ih Prachi, I see what you mean. When changing the FillStyle, internally two values will get changed, the FillStyle and the data for it. Each object has a FillStyle and for each FillStyle a set (or defaulted) value. Tus, two undo-actions get created since these actions are executed from the panel individually. There is no easy workaround for this, but of course from user view, this is not correct (even when it is from internal view). I propose to change to enhancement and fix after 4.0 ASAP. Okay?
Comment 3 Edwin Sharp 2013-07-04 16:06:09 UTC
We will be the laughingstock of the computing world if such a major release will contain this defect.
Comment 4 Edwin Sharp 2013-07-04 16:08:46 UTC
(In reply to Edwin Sharp from comment #1)
> Not sure what is meaning of "area section of sidebar".

Missed the attachment, sorry...
Comment 5 Armin Le Grand 2013-07-04 16:15:58 UTC
ALG: Edwin, from my POV it's only a cosmetic defect. The functionality is not broken, you can (un)do what you intend (even whit more granularity, in principle ;-)). It's not a data loss, not a crash.
Comment 6 Armin Le Grand 2013-07-04 16:21:12 UTC
ALG: At comment 1: All works as expected. When typing '45' of course you will have a phase with '4'. Maybe a longer wait period wouzld be good for the input fields, but then others will complain.
The Redo is not supposed to select the objects (selection is not and was never part of the undos in the graphic parts of the office). The sidebar orients on the selection, thus it's okay from my POV that it stays on what makes sense with the selection (in this case: no selection), it's not 'stuck' in any way.
Comment 7 Armin Le Grand 2013-07-04 16:24:13 UTC
ALG: Also there is #121924#. That task is for 'merging' the created undos in a useful manner, e.g. in the example from comment 1 it would 'merge' the changes from 'contrast 0->4' and 'contrast 4->45' to one undo in the future. Same is e.g. true for position/rotation/size.
Comment 8 Edwin Sharp 2013-07-04 16:28:47 UTC
After such a long long wait the public will have high expectations (rightfully so!).
Elementary operation like undo-redo must work flawlessly.
If needed, release date to be September 1st.
Comment 9 Armin Le Grand 2013-07-04 16:34:28 UTC
ALG: I see your point, but Undo/Redo works flawlessly, it just has more steps than expected from user's view. Undo/Redo not working flawlessly means crashes or data loss from my POV.
Comment 10 Edwin Sharp 2013-07-04 16:54:48 UTC
(In reply to Armin Le Grand from comment #9)
> Undo/Redo works flawlessly, it just has more
> steps than expected from user's view. 

No one will use a software that is wasting time.
The attention span of people nowadays is very short.
Uninstall will follow shortly.
Comment 11 jsc 2013-07-05 08:45:29 UTC
remove showstopper request, this is not critical.

Easy workaround is possible. I am willing to grant the showstopper flag again when a useable, uncritical fix comes in time.

Before you request a showstopper please think about our criteria. We accept only issues that fulfill our showstopper criteria or simple issues where a fix is already in place and which are not critical.

And moving the release date based on such simple issue is not an option because than we will never release. And the next release is coming as well.
Comment 12 jsc 2013-07-05 08:47:29 UTC
and keep in mind showstopper rquests have to be checked and this will take time that can't be used to fix real problems.
Comment 13 Armin Le Grand 2013-07-05 12:28:21 UTC
ALG: Investigated further, I may have found a safe way to do that, need more checks on it...

@Edwin: Thanks for your comments and be ensured that I also see that error (as I wrote several times) and want to fix it and do investigate. I think you want to do the right thing, but I have to state that your comments did not increase my motivation (which I have anyways). Please be a little bit more constructive in the future. Thanks!
Comment 14 Armin Le Grand 2013-07-05 13:27:42 UTC
ALG: The change works with Draw/Impress, checking other apps. Also looking in the processing chain (sfx) if and how this might go wrong..
Comment 15 Armin Le Grand 2013-07-05 13:31:43 UTC
ALG: Works well in Draw/Calc/Writer and Chart (which has no sidebar currently but is also capable of adding draw objects).
Comment 16 Armin Le Grand 2013-07-05 13:42:26 UTC
ALG: Using the SfxDispatcher::Execute call which allows a null-terminated list of SfxPoolItems to be handed over instead of doing two calls with just one SfxPoolItem. The difference is that only one SlotID can be handed over, Checked that the adding of SfxPoolItems to an SfxItemSet is directly done in SfxDispatcher::Execute itself, thus the SfxPoolItems always arrive at the application as SfxItemSet. Prerequisite to make this work is that the SlotIDs used lead to the correct execution, this is done in DrawViewShell::FuTemporary and in a single case the slots SID_ATTR_FILL_STYLE, SID_ATTR_FILL_COLOR, SID_ATTR_FILL_GRADIENT, SID_ATTR_FILL_HATCH and SID_ATTR_FILL_BITMAP are handled using SetAttributes at the SdrDrawView. Thus, using a single Execute call with two SfxPoolItems and the slot of the original command and not SID_ATTR_FILL_STYLE works well. This path exists in the other apps, too, so I would say the change is safe.
Resetting the ReleaseBlocker flag now with a possible solution which I evaluate as not dangerous. Preparing to add the patch, too.
Comment 17 jsc 2013-07-05 13:45:16 UTC
grant showstopper flag, fix is available
Comment 18 Armin Le Grand 2013-07-05 14:10:48 UTC
ALG: Played around with various actions and using Undo/Redo massively to evtl. trigger unexpected effects, but found none. Flag is granted, preparing commit.
Comment 19 Armin Le Grand 2013-07-05 14:14:55 UTC
ALG: Unfortunately changes in AreaProperyPanel conflict with changes on trunk, need checkout and rebuild, that will take a while.
Comment 20 Armin Le Grand 2013-07-05 18:02:55 UTC
ALG: Okay, build done, checked again, works as expected. Comitting.
Comment 21 Armin Le Grand 2013-07-05 18:04:04 UTC
ALG: Comitted (r1500087), done.
Comment 22 SVN Robot 2013-07-05 18:10:23 UTC
"alg" committed SVN revision 1500087 into trunk:
i122676 Call Execute only once to create only one Undo-ction
Comment 23 liuping 2013-07-15 02:25:30 UTC
Verified on AOO400m3(Build:9702)  -  Rev. 1502185 2013-07-10 14:15:55 (Mi, 10 Jul 2013) on Win7 OS, Pass