#10771 NORM 11.2.0-: Paint undo functionality with clippings seems to often cause duplication

[Zarro Boogs per Child]

#10771: Paint undo functionality with clippings seems to often cause duplication
--------------------------------------------------------+-------------------
           Reporter:  greenfeld                         |       Owner:  godiard                          
               Type:  defect                            |      Status:  new                              
           Priority:  normal                            |   Milestone:  11.2.0-M3                        
          Component:  paint/drawing-activity (oficina)  |     Version:  Development build as of this date
         Resolution:                                    |    Keywords:                                   
        Next_action:  code                              |    Verified:  0                                
Deployment_affected:                                    |   Blockedby:                                   
           Blocking:                                    |  
--------------------------------------------------------+-------------------

Comment(by manuq):

 Replying to [comment:4 godiard]:
 > Manu:
 >
 > Good work, I am very happy when we remove code :) And the remaining code
 works better!
 >
 > I have a few comments:
 >
 >  * In line 1199 there are a remaining line "self.undo_times -= 1"
 >
 >  * This:
 >
 > {{{
 > +        if self.undo_index == 0:
 > +            # first undo:
 > +            pass
 > +        elif self.undo_index > 0:
 > +            self.undo_index -= 1
 > }}}
 > can be:
 > {{{
 > +        if self.undo_index > 0:
 > +            self.undo_index -= 1
 > }}}
 > right?

 Yes, I'll change that.

 >
 >  * I know is old code, but we can replace:
 >
 > {{{
 > +        elif len(self.undo_list) == 12:
 > by
 > +        elif len(self.undo_list) == MAX_UNDO_STEPS:
 > }}}
 > and define a constant.

 Of course! I'll do.

 >  * Is possible remove undo_index and use len(undo_list) ?

 No, because the list may have the pixmaps for redo.  Those are the pixmaps
 after the index position.  If there is nothing to redo, then: undo_index
 == len(undo_list) - 1

 >  * There are a few uses of undo_times in Desenho.py
 >
 > {{{
 > Desenho.py:                #destroy the undo screen of polygon start
 > Desenho.py:                widget.undo_times -= 1
 > Desenho.py:                #destroy the undo screen of polygon start
 > Desenho.py:                widget.undo_times -= 1
 > }}}

 Oh, I need to check that.  Thanks.

 >
 >  * Is a good opportunity to rename self.undo_index/self.undo_list to
 self._undo_index/self._undo_list

 Yes, I'll do.

 Wait for a better patch from your comments.  Thanks for the great review.

-- 
Ticket URL: <http://dev.laptop.org/ticket/10771#comment:5>
One Laptop Per Child <http://laptop.org/>
OLPC bug tracking system