On Mon, Oct 20, 2008 at 3:19 PM, Eben Eliason <[EMAIL PROTECTED]> wrote: > Thanks for the reviews so far! While updating my jhbuild I came > accros a couple other related patches (attached). I'm building again > as I write this, so I'll try to rebase all of my patches today. > > The first is just visual, the second is a change to sugar-toolkit > which is required to support the highlighting of the tray on drag, and > the last just makes use of the go-up/go-down arrows which have been in > the them for some time, but never referenced in the tray code, which > was using left/right instead.
Look good, just some nitpicks: 0001-Size-and-position-clippings-correctly-when-dragged.patch + context.set_icon_pixbuf(pixbuf, pixbuf.props.width / 2, + pixbuf.props.height / 2) Named parameters can give very useful info to the casual reader without making the code much more verbose. I prefer to write this instead: + context.set_icon_pixbuf(pixbuf, hot_x=pixbuf.props.width / 2, + hot_y=pixbuf.props.height / 2) 0001-Add-drag-active-property-to-tray-control-8604.patch Cannot we just use gtk.Widget.drag_highlight and gtk.Widget.drag_unhighlight? http://pygtk.org/docs/pygtk/class-gtkwidget.html#method-gtkwidget--drag-highlight I think you can push most of the patches unless you need to do substantial changes. Thanks, Tomeu > On Mon, Oct 20, 2008 at 6:05 AM, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: >> On Fri, Oct 17, 2008 at 7:06 PM, Eben Eliason <[EMAIL PROTECTED]> wrote: >>> On Fri, Oct 17, 2008 at 12:48 PM, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: >>>> On Fri, Oct 17, 2008 at 6:11 PM, Eben Eliason <[EMAIL PROTECTED]> wrote: >>>>> This is a set of patches I worked on recently, and need to rebase on >>>>> the latest jhbuild before I post them officially. I wanted to expose >>>>> them for comments before I put in that effort, since there are no >>>>> doubt other things that will need to be changed upon review. >>>>> >>>>> Thanks! >>>>> >>>>> - Eben >>>>> >>>>> PS. I just noticed upon attaching that the first isn't actually a >>>>> clipboard patch, but it was part of a sprint on clipboard and >>>>> drag'n'drop in general, so I include it. >>>> >>>> [PATCH] Lock cursor to center of icons in favorites view on drag (#7408) >>>> >>>> Looks good, you can as well calculate again the hot point from the >>>> pixbuf in the context, instead of adding two private members more to >>>> the class. >>> >>> Oh really? I didn't see a way to pull that back out. Could you give >>> me a pointer? >> >> You are right, I expected that gtk.DragContext would have an accesor >> as well as a setter. >> >>>> [PATCH] Add descriptions to clippings (#5751) >>>> >>>> Not sure if we should do the formatting in model/clipboardobject.py, >>>> that looks to me more like a presentation issue so should live in the >>>> view classes? >>> >>> I debated this back and forth. My decision to put it in the model >>> meant that we could simple return a description easily in the correct >>> format given the clipping type. Otherwise, the view needs to special >>> case based on the type of the clipping. It seemed to me (ignoring the >>> details of implementation) that I really just wanted "description" to >>> be "just a string" in the model, which the view could call up at any >>> point. From this perspective, it's natural for the model to return an >>> appropriate string, and for the view to color, size, position, etc. >>> that string as it wants, right? >> >> I'm ok with the concept of a single-line description of a clipping >> living in the model, but the _MAX_DESCRIPTION_LENGTH value seems to me >> like a UI decision. What about moving it to be an argument of >> get_description()? > > That sounds like a good idea. > >> + if key == 'STRING': >> >> I'm not sure all text clippings will have the STRING target, I would do >> instead: >> >> + if key in ['STRING', 'text/plain', ...]: >> >> (I'm not really sure which are the most common targets that contain >> text, I would check the logs after a paste, these are printed in >> there). > > I'll take a look, thanks. I was always unsure about the best way to > get that info; I just needed something that worked to get the rest up > and running. I'll look at your previous patch to similar effect, > also. > > - Eben > >>>> [PATCH] Prevent duplicate clippings on drag within clipboard (#8606) >>>> >>>> Sounds good as a temporary measure, but I think that Gtk+ has support >>>> in its trays for reordering elements with DnD. Or it may be some >>>> extension to gtk+? Worth investigating. >>> >>> Yes, that's the long term solution. I just wanted something to clean >>> up the behavior to prevent confusion in the meantime. There is a TODO >>> somewhere in there where I mention we should be rearranging instead, I >>> think. It might be the case that doing it right makes the rest of >>> this patch obsolete, but I'm not sure. >> >> Sure, I think that your solution is very good in the meantime. >> >> Thanks, >> >> Tomeu >> > _______________________________________________ Sugar mailing list [email protected] http://lists.laptop.org/listinfo/sugar
