Merge lp:~tigrangab/granite/popover_fix into lp:~elementary-pantheon/granite/granite

[Tigran Gabrielyan]

popover: Added a NONE to the position enum.

Since the default pop position is set to TOPRIGHT so the condition if (old_pos != pos) { is not met and compute_shadow not called. I added a NONE enum to use as the default value.

[David Gomes (davidgomes)]

>a bit carried away fixing code style issues
I can tell, each branch has its own goal. Therefore, this branch should only fix the popover issue and you should then submit another branch with code style issues.

Besides, I never used PopOvers and unless somebody who has understands this branch and reviews it for me, I'll have to review it. Thus, I'd prefer if you explained the issue more elaboratedly because I don't really understand, thank you:

>Since the default pop position is set to TOPRIGHT so the condition if (old_pos != pos) { is not met and compute_shadow not called.

review: Needs Fixing

[lp:~tigrangab/granite/popover_fix]

626. By Tigran Gabrielyan on 2013-09-06

Fix arrow position in popover when the position needs to be top right

[Tigran Gabrielyan (tigrangab)]

Ok. So the way it was at first was "PopPosition pos = PopPosition.TOPRIGHT;"

Now inside of compute_pop_position it sets "var old_pos = pos;"
So old_pos is now set to TOPRIGHT.

At the end of compute_pop_position after it calculates the new position, it checks

if (old_pos != pos) {
     compute_shadow (get_allocated_width (), get_allocated_height ());
}

If the "pos" calculated is TOPRIGHT, compute_shadow won't be called since old_pos is set to TOPRIGHT initially and the old_pos = pos.

This causes the arrow to show bottom left position (arrow_up is default false and the offset isn't drawn to the right because compute_shadow isn't called).

[David Gomes (davidgomes)]

I get it, it's a good fix (logically), but is it reproducable in any way so I can just confirm it fixes the issue? (bureaucracy)

If this is an issue you noted that could happen but is not really happening anywhere, I'll merge it anyway, I like this.

Anyway I'm totally +1 on the code.

review: Approve

[Tigran Gabrielyan (tigrangab)]

You can try my RTL branch on slingshot with and without this. In Slingshot you can add setDefaultDirection(Gtk.TextDirection.RTL); in the constructor so you dont have to change your language. You'll see the arrow is in the bottom left.

[David Gomes (davidgomes)]

review: Approve

=== modified file 'lib/Widgets/PopOver.vala'
--- lib/Widgets/PopOver.vala	2013-03-20 15:42:25 +0000
+++ lib/Widgets/PopOver.vala	2013-09-06 20:20:34 +0000
@@ -52,6 +52,7 @@
      */
     public enum PopPosition
     {
+        NONE,
         TOPLEFT,
         TOPRIGHT,
         BOTTOMLEFT,
@@ -64,7 +65,7 @@
         }
     """;
 
-    PopPosition pos = PopPosition.TOPRIGHT;
+    PopPosition pos = PopPosition.NONE;
     protected bool arrow_up = false;
     protected double arrow_offset = 35.0;