[sugar] [PATCH] Fix Keep Icon in Journal (Was: Recursive Signal Loop.)

[Eben Eliason]

Well, here's what I've got at present.  It seems to be a worthwhile
cleanup patch even though it doesn't fix the problem I set out to fix.
 It's true that the new Journal designs will eliminate the problem
once we get around to implementing them.  I suppose we'll have to live
with the slow feedback on the favorite buttons in the interim.

- Eben


On Fri, Apr 25, 2008 at 10:55 AM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>
> On Thu, Apr 24, 2008 at 11:37 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
>  > On Thu, Apr 24, 2008 at 5:25 PM, Bert Freudenberg <bert at freudenbergs.de> wrote:
>  >  >
>  >  >
>  >  >  On 24.04.2008, at 22:56, Eben Eliason wrote:
>  >  >
>  >  >
>  >  > > On Thu, Apr 24, 2008 at 4:47 PM, Jameson Chema Quinn
>  >  > > <jquinn at cs.oberlin.edu> wrote:
>  >  > >
>  >  > > > you check the keep property before you set it, and do not touch it if
>  >  > you
>  >  > > > are not going to change it.
>  >  > > >
>  >  > >
>  >  > > That does in fact sound like a reasonable way to handle it.  It
>  >  > > doesn't require an extra time around "the loop"....it simply stops it
>  >  > > directly at the signal handler for the KeepIcon change event by not
>  >  > > overwriting with the same value.  Don't I feel stupid now....
>  >  > >
>  >  >
>  >  >
>  >  >  I'd even consider it a bug if overwriting a property with the same value
>  >  > would emit a change event - but maybe this is how the framework works?
>  >
>  >  Good point.  That seems not to be the case.
>  >
>  >  In other news, despite the much cleaner underlying code, my initial
>  >  treatment fails to alleviate the symptom!  It appears that the redraw
>  >  doesn't happen until after all of the signal handling business has run
>  >  its course.  This includes, as mentioned in my first message,
>  >  refreshing the entire view.  I looked at this again, and there is no
>  >  check for the particular CollapsedEntry which changed at all.
>  >  Instead, it loops over each one, refreshing each by setting the
>  >  jobject property of each, which of course resets the keep icon, resets
>  >  the object icon, creates a brand new palette from scratch and attaches
>  >  it to the object icon, looks up and formats the date, reads the list
>  >  of buddies and creates their associated objects and palettes, and a
>  >  number of other smaller tasks.  That's for /each/ entry shown, all
>  >  because one toggle button was clicked!  This seems like serious
>  >  overkill, but I'm not sure if there's an obvious way to isolate the
>  >  changes.  It does seem that, if anything, either the query.py or
>  >  listview.py code should be more intelligent about determining what
>  >  changes are worth doing such comprehensive updates for.  This is not
>  >  one of them.
>  >
>  >  Tomeu, you worked on the query and list code.  Do you have any insight
>  >  into this?
>
>  The lazy programmer that coded this (me) thought that the simple
>  solution implemented was efficient enough and could be written in a
>  simple way with many mainteinance advantages.
>
>  Eben, your new designs will render this code totally obsolete, so
>  what's the point in changing this right now?
>
>  About the signal loop, it's my opinion that most property setting code
>  should only do its thing if the new value is different to the current
>  one. This can affect performance considerably, as well.
>
>  Thanks,
>
>  Tomeu
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_keep_icon_in_journal.diff
Type: text/x-patch
Size: 4831 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/sugar/attachments/20080425/d00f65b7/attachment.bin