I see this behavior as well. The arrow always tracks out of bounds but only on this bubble. Perhaps it is ignoring the gap?

boxpointer has code to slide the bubble over if the pointer would end up off the edge (http://git.gnome.org/browse/gnome-shell/commit/?id=6adc12368cf09c2056606c10314de2d9674a2557), but this gets thwarted in this instance by the code to keep the bubble fully on-screen (and not touching the screen edge).

It seems like the possible solutions are:

   1. Allow the bubble to go offscreen in this case

   2. Redesign the summary area so this case can't happen

   3. Add additional cases to the bubble border drawing code
      to handle pointer/corner overlap and draw something like:

          /--------\
          |        |
          \------- |
                  \|

I think 3 is probably the right one - if we make boxpointer more generic, we'll squash future problems too.

Created attachment 179515 [details] [review]
Patch to add an alternate bubble arrow drawing

I added a little check at bubble positioning to detect corner-arrow.

Then the drawing is a changed to make some half-arrow at corners if needed.

Comment on attachment 179515 [details] [review]
Patch to add an alternate bubble arrow drawing

This seems to break bubbles entirely for me. If I use notify-send to send myself a test notification, and then click on the icon in the summary area, nothing happens (well, if I click enough times, I get some warnings in the terminal where I started gnome-shell, but it doesn't ever show the bubble).

I'm not sure why it would work for you and not me. Are you using any extensions/custom CSS?

>+const Corner = {
>+    NONE: 0,
>+    TOP: 1,
>+    BOTTOM: 2,
>+    LEFT: 4,
>+    RIGHT: 8
>+};

Use (1 << 0), etc, rather than specific bit values.

Also, "Corner" is an odd name for an enum whose values are sides, not corners...

I think the code can be rewritten to just use StCorner here. (And use null or undefined for Corner.NONE.)

>+        this._dismissCorner = Corner.NONE;

"dismiss" is a bad word to use in this context. At first I was reading it as "the corner that has something to do with dismissing the notification". Maybe "_arrowCorner".

>+        let [x2, y2] = [boxWidth-halfBorder, boxHeight-halfBorder];

spaces around the "-"s.

also, those coordinates are only correct after the cr.translate() a few lines later, so you should probably move them there.

>+            if (this._arrowOrigin == 0) {

>+            } else if (this._arrowOrigin == width) {

The normal case draws from his._arrowOrigin - halfBase to his._arrowOrigin + halfBase, so if this._arrowOrigin is anywhere from 0 to halfBase, the normal case would end up drawing the arrow partly off the edge. So these checks need to be "if (this._arrowOrigin < halfBase)" and "if (this._arrowOrigin > width - halfBase)". (And similarly in the other cases.)

>+        let [centerX, centerY] = [sourceX + (sourceWidth / 2), sourceY + (sourceHeight / 2)];

call that sourceCenterX, sourceCenterY to clarify that it refers to the source, not the bubble.


>+            if (centerY < margin) { // Not enough space to the left
>+                this._dismissCorner |= Corner.TOP;

so, here, I think you can just do something like:

    this._arrowCorner = (this._arrowSide == St.Side.LEFT) ? St.Corner.TOPLEFT : St.Corner.TOPRIGHT;

then you don't need the Corner flags type.


I'm not sure about all the math changes... and it would be easier to test if the patch actually worked for me.

Created attachment 179542 [details] [review]
Patch to add an alternate bubble arrow drawing

(In reply to comment #5)
> (From update of attachment 179515 [details] [review])
> This seems to break bubbles entirely for me. If I use notify-send to send
> myself a test notification, and then click on the icon in the summary area,
> nothing happens (well, if I click enough times, I get some warnings in the
> terminal where I started gnome-shell, but it doesn't ever show the bubble).

Just a little mistake…


> >+        let [x2, y2] = [boxWidth-halfBorder, boxHeight-halfBorder];
> 
> spaces around the "-"s.
> 
> also, those coordinates are only correct after the cr.translate() a few lines
> later, so you should probably move them there.

As they use other vars that are defined before translation,
I think it's ok this way.
They are only here to be easier to see what we are drawing after, not a real
calculation.


> >+            if (this._arrowOrigin == 0) {
> 
> >+            } else if (this._arrowOrigin == width) {
> 
> The normal case draws from his._arrowOrigin - halfBase to his._arrowOrigin +
> halfBase, so if this._arrowOrigin is anywhere from 0 to halfBase, the normal
> case would end up drawing the arrow partly off the edge. So these checks need
> to be "if (this._arrowOrigin < halfBase)" and "if (this._arrowOrigin > width -
> halfBase)". (And similarly in the other cases.)
> 

It's not a real check as the real one is done in setPosition now.
I rewrite the conditions to reflect that.


Other reviews applied.

(In reply to comment #6)
> As they use other vars that are defined before translation,
> I think it's ok this way.
> They are only here to be easier to see what we are drawing after, not a real
> calculation.

Right, but someone just reading the code, in order, might get confused by it the way it is now.

> It's not a real check as the real one is done in setPosition now.

Oops, yes, I noticed that when I got down to that part, but then forgot to go back and delete my earlier comments.


So, two visual problems with the current patch, given a notification pointing to the right-most spot in the summary area:

    - once _arrowCorner gets set, it never gets cleared, so the arrow
      keeps pointing out the corner even after you hover over the icon
      and it slides away from the corner again.

    - The St.Side.BOTTOM and St.Corner.BOTTOMRIGHT case is obviously
      drawing the wrong set of lines; probably a bad copy and paste from
      one of the other cases?

What would be really awesome, to verify that all of the cases are correct, would be a test program in tests/interactive/, that would demonstrate each of the possible cases (eg, by drawing 8 different boxpointers with the parameters set to trigger each of the 8 corner arrow cases). It shouldn't be too much work.

Created attachment 179550 [details] [review]
Patch to add an alternate bubble arrow drawing

I think it's ok now.

I'll submit a test … soon?

Comment on attachment 179550 [details] [review]
Patch to add an alternate bubble arrow drawing

Hm... not quite right. The bubble ends up farther from the screen when the arrow is in the corner than it does when it's not in the corner. This makes the whole bubble jump around as the summaryitem slides in and out

Created attachment 179786 [details] [review]
Patch to add an alternate bubble arrow drawing

I think this one is better. When there is enough space for border radius, I draw the radius then check if a plain arrow can be drawn, if not, I draw a half arrow until there is no more space for the radius, in which case I draw the half arrow at the border, no matter if it points to the center of the source

Looks good. Thanks!