GNOME Bugzilla – Bug 620504
Get rid of SingleView._dropGroup
Last modified: 2010-06-16 19:06:12 UTC
The dropGroup actor that singleView raises above the entire stage is at the root of a lot of messiness. In particular, the duplication of code from dnd.js in acceptDrop really isn't acceptable. dnd.js should be the sole place where we map from pointer location to drop target. Two alternate approaches come to mind: - We could raise empty actors only where we need actors (at the edge of the screen), and handle the remaining mouse-over behaviors from individual handleDragOver on actors. - We could have something like: Dnd.addDragMonitor({ dragMotion: function(stageX, stageY, targetActor) { returns [not a drop site, a copy drop, a move drop, continue normal processing] }, dragDrop: function(event, returns [failed drop, successful drop, continue normal processing] } }); (And Dnd.removeDragMonitor) I think I like the latter approach better for this - the former seems like it would result in very complex code.
Created attachment 163032 [details] [review] [dnd] Add monitoring hooks for motion/drop events Sometimes it is desirable to be able to react to DND events that happen outside a target actor's bounds, e.g. to implement reactive screen edges. Add a simple interface which allows to hook into drag motion and drop handling without jumping through ugly hoops.
Created attachment 163033 [details] [review] [linearView] simplify new-workspace-area Instead of a composite actor, which uses an internal child to create a stylable gradient overlay, make the new-workspace-area a simple actor and reuse the workspace shadow for the gradient overlay. This is not strictly in the scope of this bug, but it strikes me as a nice cleanup anyway.
Created attachment 163034 [details] [review] [linearView] Drop _dropGroup When in drag mode, the linear view raises a transparent actor covering the entire stage. That way the view can handle events like dragging items to the screen edges, but in order to not disable any drop target not handled by itself, it duplicates huge amounts of code from dnd.js. Cleanup that mess by using the new drag monitoring support.
Review of attachment 163032 [details] [review]: Basically looks good, some comments ::: js/ui/dnd.js @@ +57,3 @@ +function removeMonitor(monitorId) { + if (dragMonitors.length > monitorId) + dragMonitors.splice(monitorId, 1); This breaks the ID's of previously added monitors. I'd suggest just removeMonitor(monitor) and not have the "ID" thing @@ +328,3 @@ + // Call observer functions before iterating over all + // targets to avoid calling each monitor more than once + // per event Comment is confusing to me (even knowing our earlier conversation.) Maybe something like: // We call observers only once per motion with the innermost target // actor. If necessary, the observer can walk the parents itself. Saying "to avoid calling each monitor once per event" doesn't really help, since then that invites "why do we want to avoid calling the monitor once per event". This way of doing it pretty natural, so a simple descriptive comment should be enough. @@ +332,3 @@ + x: stageX, + y: stageY, + actor: this._dragActor, Better not to have an unprefixed 'actor' and a 'targetActor' -suggest dragActor: @@ +337,3 @@ + for (let i = 0; i < dragMonitors.length; i++) { + let motionFunc = dragMonitors[i].dragMotion; + if (motionFunc && typeof motionFunc == 'function') What's the typeof check about? If someone has 'dragMotion: <random non-function>' wouldn't it be better to get an exception than to have it silently do nothing?
Review of attachment 163033 [details] [review]: I'm unclear about the "reuse the workspace shadow for the gradient overlay" - I see that the code changes things so that the right shadow is always visible, but what code or functionality is this replacing? (Visually, the appearance does seem to match old and new) ::: js/ui/workspacesView.js @@ -1048,3 +1026,3 @@ this._leftShadow.hide(); - if (active < this._workspaces.length - 1) + if (active < this._workspaces.length) Isn't the new condition always true?
Review of attachment 163034 [details] [review]: Looks good to me; as expected, much cleaner. Just one comment - OK to commit with that fixed (and any fixes needed for my comments on the dnd.js patch) ::: js/ui/workspacesView.js @@ +617,3 @@ + if (leftWorkspace && + leftWorkspace.acceptDrop(self, actor, x, y, time)) + leftWorkspace.metaWorkspace.activate(time); Shouldn't you return true here and below?
(In reply to comment #5) > Review of attachment 163033 [details] [review]: > > I'm unclear about the "reuse the workspace shadow for the gradient overlay" - I > see that the code changes things so that the right shadow is always visible, > but what code or functionality is this replacing? (Visually, the appearance > does seem to match old and new) Before two actors were overlayed: the first one with background color and border and the second one with a gradient - I guess the intended effect was to fade out the border towards the right. I don't quite like it because: (1) it makes checking for hover/drop slightly more complicated (2) more importantly: the code is modeled to achieve a certain effect, and thus makes assumptions about the style - while exposing confusing internals ('new-workspace-area-internal') to theme authors So yeah, I'd like to land it because I think it's a tiny bit clearer, both on the code and the style side, but hardly a deal-breaker. > Isn't the new condition always true? Yup.
(In reply to comment #7) > (In reply to comment #5) > > Review of attachment 163033 [details] [review] [details]: > > > > I'm unclear about the "reuse the workspace shadow for the gradient overlay" - I > > see that the code changes things so that the right shadow is always visible, > > but what code or functionality is this replacing? (Visually, the appearance > > does seem to match old and new) > > Before two actors were overlayed: the first one with background color and > border and the second one with a gradient - I guess the intended effect was to > fade out the border towards the right. > > I don't quite like it because: > (1) it makes checking for hover/drop slightly more complicated > (2) more importantly: the code is modeled to achieve a certain effect, > and thus makes assumptions about the style - while exposing confusing > internals ('new-workspace-area-internal') to theme authors > > So yeah, I'd like to land it because I think it's a tiny bit clearer, both on > the code and the style side, but hardly a deal-breaker. Hmm, so doesn't this mean that before the (+) image was above the gradient, and now the image is under the gradient? (I was wondering about this when I tried out your patch - the image borders looked a bit fainter than I expected. But wasn't sure. It's not a very strong gradient...)
Created attachment 163335 [details] [review] [dnd] Add monitoring hooks for motion/drop events Updated according to comments.
Review of attachment 163335 [details] [review]: Looks good
Created attachment 163336 [details] [review] [linearView] simplify new-workspace-area Removed if(true). (In reply to comment #8) > Hmm, so doesn't this mean that before the (+) image was above the gradient, and > now the image is under the gradient? Oh, I didn't think of that - yes, probably. Although I'd argue that the purpose of the gradient is to blend the area with the background ... Redo the follow-up patch to work with the old code?
Created attachment 163337 [details] [review] [linearView] Drop _dropGroup (In reply to comment #6) > Shouldn't you return true here and below? Mmmmh - in practice we accept any drop, but do we really want to put it that explicitly here? Could you review the return values I use now if those make any sense?
(In reply to comment #12) > Created an attachment (id=163337) [details] [review] > [linearView] Drop _dropGroup > > (In reply to comment #6) > > Shouldn't you return true here and below? > > Mmmmh - in practice we accept any drop, but do we really want to put it that > explicitly here? Could you review the return values I use now if those make any > sense? What you did was what I was trying to suggest. I was commenting on the lack of any return at all in the two acceptDrop functions.
(In reply to comment #11) > Created an attachment (id=163336) [details] [review] > [linearView] simplify new-workspace-area > > Removed if(true). > > (In reply to comment #8) > > Hmm, so doesn't this mean that before the (+) image was above the gradient, and > > now the image is under the gradient? > > Oh, I didn't think of that - yes, probably. Although I'd argue that the purpose > of the gradient is to blend the area with the background ... > > Redo the follow-up patch to work with the old code? I don't have a strong opinion here.... in the end things are currently looking a bit ugly with or without the gradient: - The + overlaps the border (at least on a 1280x1024 monitor) - The outline of the + is "spindly" and ragged I have some slight preference for the old code, because we won't have to worry about it if we update the art to something else where the overlapping gradient is more obvious, or if we change the gradient to be stronger. But I'll leave it up to you. If you go with the new code, please add a comment somewhere about how we are reusing the shadow to get the fade-out effect and it overlaps the icon, even if you can't quite see it.
Comment on attachment 163336 [details] [review] [linearView] simplify new-workspace-area OK, so here's the deal then: I'll leave the NewWorkspaceArea object alone and adapt the follow-up patch, you do another review ;-) Seriously, taking a patch out of the series did require some modifications, so I'd appreciate a second pair of eye balls ...
Created attachment 163346 [details] [review] [linearView] Drop _dropGroup When in drag mode, the linear view raises a transparent actor covering the entire stage. That way the view can handle events like dragging items to the screen edges, but in order to not disable any drop target not handled by itself, it duplicates huge amounts of code from dnd.js. Cleanup that mess by using the new drag monitoring support.
Created attachment 163348 [details] [review] [linearView] Drop _dropGroup Sorry for the screw-up, this patch should be ready for review ...
Created attachment 163371 [details] [review] [linearView] Drop _dropGroup No idea why I didn't catch those before ...
Review of attachment 163371 [details] [review]: Looks good for me; one tiny comment about parameter naming. ::: js/ui/workspacesView.js @@ +639,3 @@ x: global.screen_width }); + this._leftShadow._delegate = { + acceptDrop: Lang.bind(this, function(self, actor, x, y, time) { 'self' here is odd - isn't the first parameter the source actor for the drag?
(In reply to comment #19) > 'self' here is odd - isn't the first parameter the source actor for the drag? Mmmh, yeah - it's actually the source actor's delegate, but self is definitively wrong. Renamed to 'source' (as sourceActor isn't right either) ... Attachment 163335 [details] pushed as c50e324 - [dnd] Add monitoring hooks for motion/drop events Attachment 163371 [details] pushed as 7514dfa - [linearView] Drop _dropGroup
Oh my, git-bz choked in the middle of updating the bug, sorry for the noise!