[Sugar-devel] [PATCH v2 Paint] Changes made to save the last added text item. (OLPC #5917)

[James Cameron]

On Sun, Oct 17, 2010 at 02:17:17PM +0530, Ishan Bansal wrote:
> New variable text_status defined which could keep the track of the status of
> text being entered and save it when activity is stopped.

What you do not say in this commit comment is how you have implemented
the change.  Having reviewed your code change, I see you are forcing the
text entry to complete if the mouse leaves the drawing area while a text
entry is in progress.  In detail:

1.  when the instance of the drawing area is created, you set your state
variable to -1,

2.  when a mouse down event occurs with the text tool selected, you set
your state variable to 0, representing text entry in progress,

3.  when the mouse moves with the text entry in progress, you set your
state variable to 1, representing a mouse movement in progress during
text entry,

4.  when the mouse leaves the drawing area with the state variable set to
1, you attempt to complete the drawing of the text.

Problems with this design:

a.  the text entry will be prematurely terminated if the user moves the
mouse outside the drawing entry, which will be unexpected and annoying,

b.  the mouse does not move if the Ctrl/Q shortcut is used, therefore
this code will not trigger,

c.  when system load is high, it is possible for a mouse leave event to
arrive before a mouse move event, and so this code may not trigger.

> Signed-off-by: Ishan Bansal<ishan at seeta.in>, Anubhav Aggarwal<anubhav at seeta.in>

There should be a space before <

> ---
>  Area.py |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> v1->v2: Patch updated
> 
> diff --git a/Area.py b/Area.py
> index 2dca7da..a2db51b 100644
> --- a/Area.py
> +++ b/Area.py
> @@ -113,6 +113,8 @@ class Area(gtk.DrawingArea):
>          self.connect("leave_notify_event", self.mouseleave)
>          self.connect("enter_notify_event", self.mouseenter)
>  
> +        self.text_status = -1
> +

Given my comments at the top of this message, you may not need to use
the following comments if you abandon the design ...

Please use a more appropriate variable name.  "text_status" doesn't tell
me anything about what it is for.  "pending_text_state" may be more
useful.

You are using -1 to mean initialised, 0 to mean disarmed, and 1 to mean
armed.  These constants are used throughout your patch without further
explanation.  If you still need a state variable, please define
constants, such as:

PENDING_TEXT_STATE_RESET = -1
PENDING_TEXT_STATE_DISARMED = 0
PENDING_TEXT_STATE_ARMED = 1

The -1 state does not seem to be needed.

>          target = [('text/uri-list', 0, TARGET_URI)]
>          self.drag_dest_set(gtk.DEST_DEFAULT_ALL, target,
>                  gtk.gdk.ACTION_COPY | gtk.gdk.ACTION_MOVE)

> @@ -303,6 +305,7 @@ class Area(gtk.DrawingArea):
>          # text
>          if self.tool['name'] == 'text':
>              self.d.text(widget,event)
> +            self.text_status = 0
>              
>          # This fixes a bug that made the text viewer get stuck in the canvas
>          elif self.estadoTexto is 1:

(the above is in the mouse down event handler)

> @@ -473,6 +476,10 @@ class Area(gtk.DrawingArea):
>                  self.configure_line(self.line_size)
>                  self.d.polygon(widget,coords,True,self.tool['fill'],"moving")
>  
> +        if self.tool['name'] == 'text':
> +           if self.text_status == 0:
> +              self.text_status = 1
> +
>          gtk.gdk.event_request_motions (event)
>  
>      def mouseup(self,widget,event): 

(the above is in the mouse move event handler)

> @@ -571,6 +578,10 @@ class Area(gtk.DrawingArea):
>              size = self.tool['line size']
>              widget.queue_draw_area(self.x_cursor-size, self.y_cursor-size, size*2, size*2)
>  
> +        if self.tool['name'] == 'text':
> +           if self.text_status == 1:
> +	           self.d.text(widget,event)
> +
>      def mouseenter(self, widget, event):
>          if self.tool['name'] in ['pencil','eraser','brush','rainbow']:
>              self.drawing = False

(the above is in the mouse leave event handler)

-- 
James Cameron
http://quozl.linux.org.au/