GNOME Bugzilla – Bug 678164
popupMenu: Flip the menu if it would end outside the monitor
Last modified: 2013-03-03 21:33:13 UTC
I need something like this for the IBus candidates popup and I'd like to know what people think of this approach here. We could do it directly in boxpointer but it didn't feel correct to me that boxpointer should be overriding the arrow side that is was instructed to use.
Created attachment 216512 [details] [review] boxpointer: Allow setting the arrow side on setPosition It's useful to be able to change the arrow side without having to create a whole new box pointer.
Created attachment 216513 [details] [review] messageTray: Remove unused method parameter The alignment of the arrow tip regarding the source actor is set with setSourceAlignment() and its default is already 0.5 .
Created attachment 216514 [details] [review] popupMenu: Flip the menu if it would end outside the monitor This flips the menu if it ends outside the monitor and we have enough space to fit it inside, on the opposite side of the source actor.
Review of attachment 216514 [details] [review]: The code looks correct, but why is it in popupMenu? Wouldn't it be better if instead of boxpointer.setPosition(...); if (boxpointerDoesntFit(boxpointer)) boxpointer.setPosition(..., flippedArrow); BoxPointer.setPosition() (or rather BoxPointer._reposition() I guess) just did the right thing? I can't really think of a case where we wouldn't want the menu to flip, but if we think it is needed, an 'allowFlip' property in BoxPointer might be nicer than the arrowSide parameter to setPosition() ...
Review of attachment 216512 [details] [review]: Minor comment below, otherwise looks good. However, I'm not quite sure we want this patch at all, see later review ... ::: js/ui/boxpointer.js @@ +347,3 @@ return; + this.setPosition(this._sourceActor, this._arrowAlignment); Not a big fan of unrelated changes in patches, but OK ...
Review of attachment 216513 [details] [review]: Looks good, please make sure to push this patch first
Created attachment 217335 [details] [review] boxpointer: Flip side if we would end outside the monitor -- (In reply to comment #4) > The code looks correct, but why is it in popupMenu? Wouldn't it be better if > instead of > > boxpointer.setPosition(...); > > if (boxpointerDoesntFit(boxpointer)) > boxpointer.setPosition(..., flippedArrow); > > BoxPointer.setPosition() (or rather BoxPointer._reposition() I guess) just did > the right thing? Yes this is more logical. I just wasn't sure if it would be OK to go behind the BoxPointer API user and change things "magically". I did this now and changed the comment to document the new behavior. > I can't really think of a case where we wouldn't want the menu > to flip, but if we think it is needed, an 'allowFlip' property in BoxPointer > might be nicer than the arrowSide parameter to setPosition() ... Ok, let's leave that for another patch when/if we end up needing it. (In reply to comment #5) > ::: js/ui/boxpointer.js > @@ +347,3 @@ > return; > > + this.setPosition(this._sourceActor, this._arrowAlignment); > > Not a big fan of unrelated changes in patches, but OK ... I merged this here as it now is actually needed so that we reset this._arrowSide to the default value whenenever the box is repositioned from the outside.
Review of attachment 217335 [details] [review]: LGTM
should we get this landed, then ?
Attachment 216513 [details] pushed as 0242801 - messageTray: Remove unused method parameter Attachment 217335 [details] pushed as 8d017ce - boxpointer: Flip side if we would end outside the monitor
*** Bug 635645 has been marked as a duplicate of this bug. ***