#5106 HIGH Update.: Attaching a file to gmail msg caused the journal to crash

[Zarro Boogs per Child]

#5106: Attaching a file to gmail msg caused the journal to crash
-------------------------------+--------------------------------------------
  Reporter:  kimquirk          |       Owner:  tomeu   
      Type:  defect            |      Status:  new     
  Priority:  high              |   Milestone:  Update.1
 Component:  journal-activity  |     Version:          
Resolution:                    |    Keywords:  review? 
  Verified:  0                 |  
-------------------------------+--------------------------------------------

Comment(by tomeu):

 Replying to [comment:10 rwh]:
 > In general I think this is the good approach, so a review+. However, I'd
 like to hear comment on my DBus comment so I'll leave the review? for now.
 >
 > '''collapsedentry.py'''
 >
 > {{{
 > if not self._allow_resume:
 >    logging.debug('self._frame.set_child_visible(%r, %r)' %
 (self._resume_button, False))
 > }}}
 >
 > Do we need this debug msg? Marco would say: 80 cols...

 We don't need it, forgot to take out.

 > {{{
 > def _create_icon(self):
 > }}}
 > Why do we connect_after, and to the release-event instead of clicked?
 This was already there, so I assume it is correct.

 Yeah, it works well. I'm not absolutely sure it's the best thing to do,
 but 'clicked' requires the item to be made clickable and that has other
 consequences like changing the cursor.

 I'm not sure about connect_after, but I had to spent quite a bit of time
 playing with it because of drag and drop and having nested items accepting
 clicks.

 > '''journalactivity.py'''
 > {{{
 >     @dbus.service.method(J_DBUS_INTERFACE, in_signature='i',
 out_signature='s')
 >     def ChooseObject(self, parent_xid):
 > }}}
 >
 > Wouldn't it be nicer design-wise to make this function blocking and
 return the object_id in combination with calling it asynchronously? Maybe
 even a race here because you connect to the DBus signal after calling the
 ChooseObject function. You could connect before and set self._chooser_id
 to None before calling _self.chooser_id = journal.ChooseObject().

 This makes sense, I'll think a bit more about it.

 > '''objectchooser.py'''
 >         height = (self.get_screen().get_height() / style.GRID_CELL_SIZE
 - 1) * style.GRID_CELL_SIZE
 >
 > Marco 80 cols :)

 Yup.

 > {{{
 >     def __realize_cb(self, chooser, parent):
 >         self.window.set_transient_for(parent)
 > }}}
 >
 > Can't you just do this in the constructor? That's what I did with the
 unfullscreen button at least.

 In the constructor you don't have self.window yet, right? Perhaps in your
 case you were setting a  gtk.Window transient, but I need to set the
 GdkWindow of a GtkWindow transient, because I have a window xid, not a
 parent gtk.Window.

 > Use of gtk.RESPONSE_DELETE_EVENT sounds as if dangerous stuff is
 happeing :) I think gtk.RESPONSE_REJECT or gtk.RESPONSE_CANCEL is more
 appropriate.

 Yeah, but that's needed for backwards compatibility. You don't really need
 to check the return code, just if get_selected_object returns None.

 > '''lib/sugar/graphics/objectchooser.py'''
 > Looks much simpler then the previous version I think!

 Yeah, the previous version was really a horrible hack.

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