OK, I've pushed all of the patches except for "Add descriptions to clippings", which I want to think through carefully before resubmitting. The changes pushed take into account the suggestions here and discussed in IRC, where Tomeu unofficially r-plussed them.
I'll take a closer look into adding descriptions tomorrow; Tomeu's effort in this area is already in master, so a very close approximation of the final behavior can already be seen. - Eben On Mon, Oct 20, 2008 at 9:33 AM, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: > 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
