Merge lp:~markodolar/audience/preview-fixes into lp:~audience-members/audience/trunk

[MarkoD]

fixed preview popover not always pointing to correct location

Preview popover was misaligned when you first hovered over progress scale. It was also slightly off when time label changed size.

[Jeremy Wootten (jeremywootten)]

Some code style comments.

Does this bug occur more with long videos? What length recommended for testing?

review: Needs Fixing (code format)

[lp:~markodolar/audience/preview-fixes]

739. By MarkoD on 2017-04-09

fixed code style

[MarkoD (markodolar)]

Yes, video has to be long more than an hour. Then when you watch after 59:59, time label on left changes size and popover starts pointing just a couple pixels off.

I noticed that when trying to fix the first bug - when you place mouse on progress scale and if you don't move it after popover becomes visible it stays in the same position it was last time.

I fixed the code style, ready for another review.

[Jeremy Wootten (jeremywootten)]

I have not tried it on a very long video but notice the following using a 30 minute .vob file.

There is still a defect near the beginning of the video; when moving the cursor backwards along the progress bar so that the preview stays open, the preview will not go beyond a certain point before the play back point. To preview near the beginning you have to close and reopen the preview by moving the cursor away from the progress track and repositioning nearer the beginning.

If commencing the preview near the beginning (before the playback point) and then moving the cursor forward along the track so that the preview stays open, the preview lags behind the cursor.

review: Needs Fixing (function)

[MarkoD (markodolar)]

I noticed that also and wasn't able to fix it. I suspect that it might be problem with Gtk Popover.

I tried creating and experimenting with another popover and it behaved just like existing preview. When hitting the edge of the window, it didn't update pointing arrow correctly, it just stayed centred. It does look like there is some part inside of arrow that moves properly.

But, if i called hide() then show() after every popover pointing_to update, arrow followed correctly. Not really a good solution tho, also i think that popover somehow started to move up...

Maybe i can try if it works with newer Gtk version. Do you know if i can do this on loki or would be easier to compile and test on some other distro that has newer libraries?

[Jeremy Wootten (jeremywootten)]

If the hide()/show() method causes flickering it would probably not be acceptable. It should preferably only be used when necessary (near the edges).

Another work around might be to tweak the start and end points of the progress track so that there is room for the popover with the arrow centred when pointing to the start and end of the video.

It does sound like a possible Gtk issue. I would not recommend changing the Gtk libraries on Loki (at least on a production machine) as it might cause other stuff to stop working.

[Jeremy Wootten (jeremywootten)]

Try this - it seems to fix it:

=== modified file 'src/Widgets/Player/PreviewPopover.vala'
--- src/Widgets/Player/PreviewPopover.vala 2017-04-08 09:58:00 +0000
+++ src/Widgets/Player/PreviewPopover.vala 2017-04-10 11:44:18 +0000
@@ -131,11 +131,13 @@
         });
     }

- public void update_pointing(int parent_width) {
+ public void update_pointing (int x) {
         if (visible) {
             var pointing = pointing_to;
- pointing.x = (int)(req_progress * parent_width);
- set_pointing_to ((Gdk.Rectangle)pointing);
+ pointing_to.x = x;
+ pointing.x = x;
+ pointing.width = int.min (10, x - 20);
+ set_pointing_to (pointing);
         }
     }

=== modified file 'src/Widgets/Player/TimeWidget.vala'
--- src/Widgets/Player/TimeWidget.vala 2017-04-08 09:40:27 +0000
+++ src/Widgets/Player/TimeWidget.vala 2017-04-10 11:46:00 +0000
@@ -90,12 +90,8 @@
         });

         scale.motion_notify_event.connect ((event) => {
- var pointing = preview_popover.pointing_to;
- pointing.x = (int)event.x;
- pointing.width = 0;
- preview_popover.set_pointing_to ((Gdk.Rectangle)pointing);
             preview_popover.set_preview_progress (((double)event.x)/((double)event.window.get_width ()), !main_playback.playing);
-
+ preview_popover.update_pointing ((int)event.x);
             return false;
         });

[Jeremy Wootten (jeremywootten)]

Ignore "+ pointing_to.x = x;" in update_pointing (). That should have been taken out.

[lp:~markodolar/audience/preview-fixes]

740. By MarkoD on 2017-04-10

workaround for gtk popover not updating arrow position when hitting the edge

[MarkoD (markodolar)]

Wow, nice, how did you came up with that :)

What do you think about last commit? I solved it little differently, your solution moved arrow little to fast on start. What it looks like is you just have to change pointing_to.width and Gtk fully updates the arrow.

I also renamed update_pointing() to realign_pointing() to be more clear then created another update_pointing() to handle all pointing changes.

[Jeremy Wootten (jeremywootten)]

Yes, your commit works fine :-) and is neater than my solution (I did not spend any time optimising it). I got one hang/crash during testing but it was not reproducible and may not be anything to do with this branch (there are some outstanding crasher bugs reported).

review: Approve (code, function)

=== modified file 'src/Widgets/Player/PreviewPopover.vala'
--- src/Widgets/Player/PreviewPopover.vala	2016-11-02 23:59:41 +0000
+++ src/Widgets/Player/PreviewPopover.vala	2017-04-10 17:09:38 +0000
@@ -39,6 +39,8 @@
     uint show_timer_id = 0;
     uint hide_timer_id = 0;
     uint idle_id = 0;
+    double req_progress = -1;
+    bool req_loop = false;
 
     public PreviewPopover (ClutterGst.Playback main_playback) {
         opacity = GLOBAL_OPACITY;
@@ -101,8 +103,11 @@
         cancel_loop_timer ();
     }
 
-    public void set_preview_progress (double progress, bool loop = false, bool force = false) {
-        if (idle_id > 0) {
+    public void set_preview_progress (double progress, bool loop = false) {
+        req_progress = progress;
+        req_loop = loop;
+        
+        if (!visible || idle_id > 0) {
             return;
         }
 
@@ -116,7 +121,7 @@
             playback.playing = loop;
             if (loop) {
                 loop_timer_id = Timeout.add_seconds (5, () => {
-                    set_preview_progress (progress, true, true);
+                    set_preview_progress (progress, true);
                     loop_timer_id = 0;
                     return false;
                 });
@@ -126,6 +131,27 @@
         });
     }
 
+    public void update_pointing (int x) {
+        var pointing = pointing_to;
+        pointing.x = x;
+
+        // changing the width properly updates arrow position when popover hits the edge
+        if (pointing.width == 0) {
+            pointing.width = 2;
+            pointing.x -= 1;
+        } else {
+            pointing.width = 0;
+        }
+
+        set_pointing_to (pointing);
+    }
+
+    public void realign_pointing (int parent_width) {
+        if (visible) {
+            update_pointing ((int)(req_progress * parent_width));
+        }
+    }
+
     public void schedule_show () {
         if (show_timer_id > 0) {
             return;
@@ -133,10 +159,13 @@
         cancel_timer (ref hide_timer_id);
 
         show_timer_id = Timeout.add (300, () => {
-                show_all ();
-                show_timer_id = 0;
-                return false;
-            });
+            show_all ();
+            if (req_progress >= 0) {
+                set_preview_progress (req_progress, req_loop);
+            }
+            show_timer_id = 0;
+            return false;
+        });
     }
 
     public void schedule_hide () {

=== modified file 'src/Widgets/Player/TimeWidget.vala'
--- src/Widgets/Player/TimeWidget.vala	2016-11-02 23:59:41 +0000
+++ src/Widgets/Player/TimeWidget.vala	2017-04-10 17:09:38 +0000
@@ -27,7 +27,6 @@
     private Audience.Widgets.PreviewPopover preview_popover;
     private bool released = true;
     private uint timeout_id = 0;
-    private int original = 0;
 
     public TimeWidget (ClutterGst.Playback main_playback) {
         this.main_playback = main_playback;
@@ -36,6 +35,7 @@
         halign = Gtk.Align.CENTER;
         progression_label = new Gtk.Label ("");
         time_label = new Gtk.Label ("");
+        scale = new Gtk.Scale.with_range (Gtk.Orientation.HORIZONTAL, 0, 1, 0.1);
 
         main_playback.notify["progress"].connect (progress_callback);
 
@@ -49,11 +49,10 @@
             sensitive = (main_playback.duration != 0);
             if (sensitive) {
                 preview_popover = new Audience.Widgets.PreviewPopover (main_playback);
-                preview_popover.relative_to = this;
+                preview_popover.relative_to = scale;
             }
         });
 
-        scale = new Gtk.Scale.with_range (Gtk.Orientation.HORIZONTAL, 0, 1, 0.1);
         scale.expand = true;
         scale.draw_value = false;
         scale.can_focus = false;
@@ -90,18 +89,9 @@
             return false;
         });
 
-        // XXX: Store the original size because the popover doesn't update his x=0 position when resizing.
         scale.motion_notify_event.connect ((event) => {
-            if (original == 0)
-                original = event.window.get_width ();
-
-            if (preview_popover.visible) {
-                var pointing = preview_popover.pointing_to;
-                var distance = original - event.window.get_width ();
-                pointing.x = (int)(event.x) - event.window.get_width ()/2 - distance/2;
-                preview_popover.set_pointing_to ((Gdk.Rectangle)pointing);
-                preview_popover.set_preview_progress (((double)event.x)/((double)event.window.get_width ()), !main_playback.playing);
-            }
+            preview_popover.update_pointing ((int) event.x);
+            preview_popover.set_preview_progress (event.x / ((double) event.window.get_width ()), !main_playback.playing);
             return false;
         });
 
@@ -111,6 +101,11 @@
             return false;
         });
 
+        scale.size_allocate.connect ((alloc_rect) => {
+            if (preview_popover != null)
+                preview_popover.realign_pointing (alloc_rect.width);
+        });
+
         add (progression_label);
         add (scale);
         add (time_label);