[pitivi] timeline: Introduce Remove button in marker popover

[Alexandru Băluț <alexbalut src gnome org>]

commit 2c9f2e53a5c3a86d195f84d80943fda2f1a8111a
Author: Alexandru Băluț <alexandru balut gmail com>
Date:   Mon Dec 13 00:57:18 2021 +0100

    timeline: Introduce Remove button in marker popover
    
    Right-clicking on the timeline to seek can result in clip markers being
    removed.
    
    A Remove button has been added to the marker edit popover instead.

 data/ui/markerpopover.ui       | 67 ++++++++++++++++++++++++++++++++
 pitivi/dialogs/prefs.py        |  5 ++-
 pitivi/timeline/markers.py     | 87 +++++++++++++++++++++++-------------------
 tests/test_timeline_markers.py | 32 ++++++++++++----
 4 files changed, 141 insertions(+), 50 deletions(-)
---
diff --git a/data/ui/markerpopover.ui b/data/ui/markerpopover.ui
new file mode 100644
index 000000000..31eccaef2
--- /dev/null
+++ b/data/ui/markerpopover.ui
@@ -0,0 +1,67 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Generated with glade 3.38.2 -->
+<interface>
+  <requires lib="gtk+" version="3.24"/>
+  <template class="MarkerPopover" parent="GtkPopover">
+    <property name="can-focus">False</property>
+    <child>
+      <object class="GtkBox">
+        <property name="visible">True</property>
+        <property name="can-focus">False</property>
+        <property name="margin-start">10</property>
+        <property name="margin-end">10</property>
+        <property name="margin-top">10</property>
+        <property name="margin-bottom">10</property>
+        <property name="orientation">vertical</property>
+        <property name="spacing">10</property>
+        <child>
+          <object class="GtkTextView" id="comment_textview">
+            <property name="width-request">300</property>
+            <property name="height-request">100</property>
+            <property name="visible">True</property>
+            <property name="can-focus">True</property>
+            <property name="wrap-mode">word</property>
+            <property name="accepts-tab">False</property>
+          </object>
+          <packing>
+            <property name="expand">False</property>
+            <property name="fill">True</property>
+            <property name="position">0</property>
+          </packing>
+        </child>
+        <child>
+          <object class="GtkButtonBox">
+            <property name="visible">True</property>
+            <property name="can-focus">False</property>
+            <property name="homogeneous">True</property>
+            <property name="layout-style">start</property>
+            <child>
+              <object class="GtkButton" id="remove_button">
+                <property name="label">gtk-remove</property>
+                <property name="visible">True</property>
+                <property name="can-focus">True</property>
+                <property name="receives-default">True</property>
+                <property name="use-stock">True</property>
+                <signal name="clicked" handler="remove_button_clicked_cb" swapped="no"/>
+                <style>
+                  <class name="destructive-action"/>
+                  <class name="text-button"/>
+                </style>
+              </object>
+              <packing>
+                <property name="expand">False</property>
+                <property name="fill">True</property>
+                <property name="position">0</property>
+              </packing>
+            </child>
+          </object>
+          <packing>
+            <property name="expand">True</property>
+            <property name="fill">True</property>
+            <property name="position">1</property>
+          </packing>
+        </child>
+      </object>
+    </child>
+  </template>
+</interface>
diff --git a/pitivi/dialogs/prefs.py b/pitivi/dialogs/prefs.py
index 64234d592..defb42f41 100644
--- a/pitivi/dialogs/prefs.py
+++ b/pitivi/dialogs/prefs.py
@@ -18,6 +18,7 @@
 import os
 from gettext import gettext as _
 from threading import Timer
+from typing import Optional
 
 from gi.repository import Gdk
 from gi.repository import Gio
@@ -725,9 +726,9 @@ class PluginPreferencesRow(Gtk.ListBoxRow):
     switch = Gtk.Template.Child()
 
     def __init__(self, item):
-        super().__init__()
+        Gtk.ListBoxRow.__init__(self)
         self.plugin_info = item.plugin_info
-        self.switch_handler_id = None
+        self.switch_handler_id: Optional[int] = None
 
         self._title_label.set_text(self.plugin_info.get_name())
 
diff --git a/pitivi/timeline/markers.py b/pitivi/timeline/markers.py
index 37d9243ef..674e5c3fe 100644
--- a/pitivi/timeline/markers.py
+++ b/pitivi/timeline/markers.py
@@ -15,15 +15,16 @@
 # You should have received a copy of the GNU Lesser General Public
 # License along with this program; if not, see <http://www.gnu.org/licenses/>.
 """Markers display and management."""
+import os
 from typing import Optional
 
 from gi.repository import Gdk
 from gi.repository import GES
 from gi.repository import Gtk
 
+from pitivi.configure import get_ui_dir
 from pitivi.utils.loggable import Loggable
 from pitivi.utils.timeline import Zoomable
-from pitivi.utils.ui import SPACING
 
 TIMELINE_MARKER_SIZE = 10
 CLIP_MARKER_HEIGHT = 12
@@ -75,14 +76,15 @@ class Marker(Gtk.EventBox, Loggable):
         return self.ges_marker.props.position
 
     @property
-    def comment(self):
+    def comment(self) -> str:
         """Returns a comment from ges_marker."""
-        return self.ges_marker.get_string("comment")
+        return self.ges_marker.get_string("comment") or ""
 
     @comment.setter
-    def comment(self, text):
+    def comment(self, text: str):
         if text == self.comment:
             return
+
         self.ges_marker.set_string("comment", text)
 
     @property
@@ -121,9 +123,9 @@ class MarkersBox(Gtk.EventBox, Zoomable, Loggable):
         self._offset = 0
         self.props.height_request = TIMELINE_MARKER_SIZE
 
-        self.__markers_container = None
-        self.marker_moving = None
-        self.marker_new = None
+        self.__markers_container: Optional[GES.MarkerList] = None
+        self.marker_moving: Optional[Marker] = None
+        self.marker_new: Optional[Marker] = None
 
         self.add_events(Gdk.EventMask.POINTER_MOTION_MASK |
                         Gdk.EventMask.BUTTON_PRESS_MASK |
@@ -169,7 +171,7 @@ class MarkersBox(Gtk.EventBox, Zoomable, Loggable):
 
     @property
     def markers_container(self):
-        """Gets the GESMarkerContainer."""
+        """Gets the GES.MarkerList."""
         return self.__markers_container
 
     @markers_container.setter
@@ -229,50 +231,47 @@ class MarkersBox(Gtk.EventBox, Zoomable, Loggable):
         if not self.markers_container:
             return False
 
-        event_widget = Gtk.get_event_widget(event)
-        button = event.button
-        if button == Gdk.BUTTON_PRIMARY:
+        if event.button == Gdk.BUTTON_PRIMARY:
+            event_widget = Gtk.get_event_widget(event)
             if isinstance(event_widget, Marker):
                 if event.type == Gdk.EventType.BUTTON_PRESS:
                     self.marker_moving = event_widget
                     self.marker_moving.selected = True
                     self.app.action_log.begin("Move marker", toplevel=True)
+                    return True
 
-                elif event.type == Gdk.EventType.DOUBLE_BUTTON_PRESS:
+                if event.type == Gdk.EventType.DOUBLE_BUTTON_PRESS:
                     self.marker_moving = None
                     self.app.action_log.rollback()
-                    marker_popover = MarkerPopover(self.app, event_widget)
+                    marker_popover = MarkerPopover(self.app, event_widget, self.__markers_container)
                     marker_popover.popup()
+                    return True
 
             else:
                 position = self.pixel_to_ns(event.x + self.offset)
                 with self.app.action_log.started("Added marker", toplevel=True):
                     self.__markers_container.add(position)
                     self.marker_new.selected = True
-        return True
+                return True
+
+        return False
 
     def do_button_release_event(self, event):
         if not self.markers_container:
             return False
 
-        button = event.button
-        event_widget = Gtk.get_event_widget(event)
-        if button == Gdk.BUTTON_PRIMARY:
+        if event.button == Gdk.BUTTON_PRIMARY:
             if self.marker_moving:
                 self.marker_moving.selected = False
                 self.marker_moving = None
                 self.app.action_log.commit("Move marker")
                 return True
-            elif self.marker_new:
+
+            if self.marker_new:
                 self.marker_new.selected = False
                 self.marker_new = None
                 return True
 
-        elif button == Gdk.BUTTON_SECONDARY and isinstance(event_widget, Marker):
-            with self.app.action_log.started("Removed marker", toplevel=True):
-                self.__markers_container.remove(event_widget.ges_marker)
-                return True
-
         return False
 
     def do_motion_notify_event(self, event):
@@ -321,35 +320,43 @@ class MarkersBox(Gtk.EventBox, Zoomable, Loggable):
         self.layout.move(ges_marker.ui, x, 0)
 
 
+@Gtk.Template(filename=os.path.join(get_ui_dir(), "markerpopover.ui"))
 class MarkerPopover(Gtk.Popover):
-    """A popover to edit a marker's metadata."""
+    """A popover to edit a marker's metadata or to remove the marker."""
 
-    def __init__(self, app, marker):
-        Gtk.Popover.__init__(self)
+    __gtype_name__ = "MarkerPopover"
 
-        self.app = app
-
-        self.text_view = Gtk.TextView()
-        self.text_view.set_size_request(100, -1)
+    comment_textview = Gtk.Template.Child()
+    remove_button = Gtk.Template.Child()
 
-        self.marker = marker
+    def __init__(self, app, marker: Marker, markers_container: GES.MarkerList):
+        Gtk.Popover.__init__(self)
 
-        vbox = Gtk.Box(orientation=Gtk.Orientation.VERTICAL)
-        vbox.props.margin = SPACING
-        vbox.pack_start(self.text_view, False, True, 0)
-        self.add(vbox)
+        self.app = app
+        self.marker: Optional[Marker] = marker
+        self.markers_container: GES.MarkerList = markers_container
 
-        text = self.marker.comment
-        if text:
-            text_buffer = self.text_view.get_buffer()
-            text_buffer.set_text(text)
+        self.comment_textview.get_buffer().set_text(self.marker.comment)
 
         self.set_position(Gtk.PositionType.TOP)
         self.set_relative_to(self.marker)
         self.show_all()
 
+    @Gtk.Template.Callback()
+    def remove_button_clicked_cb(self, event):
+        with self.app.action_log.started("Removed marker", toplevel=True):
+            self.markers_container.remove(self.marker.ges_marker)
+
+        self.marker = None
+
+        self.hide()
+
     def do_closed(self):
-        buffer = self.text_view.get_buffer()
+        if not self.marker:
+            # The user clicked the Remove button so no need to update the text.
+            return
+
+        buffer = self.comment_textview.get_buffer()
         if buffer.props.text != self.marker.comment:
             with self.app.action_log.started("marker comment", toplevel=True):
                 self.marker.comment = buffer.props.text
diff --git a/tests/test_timeline_markers.py b/tests/test_timeline_markers.py
index 87d3ef95d..14dcda735 100644
--- a/tests/test_timeline_markers.py
+++ b/tests/test_timeline_markers.py
@@ -65,14 +65,27 @@ class TestMarkers(common.TestCase):
         event = mock.Mock(spec=Gdk.EventButton)
         event.x = x
         event.y = 1
-        event.button = Gdk.BUTTON_SECONDARY
+        event.button = Gdk.BUTTON_PRIMARY
 
         with mock.patch.object(Gtk, "get_event_widget") as get_event_widget:
             get_event_widget.return_value = marker.ui
-            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
-            marker_box.do_button_press_event(event)
-            event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_RELEASE)
-            marker_box.do_button_release_event(event)
+
+            def popup(markerpopover):
+                # The popover is becoming visible, so we simulate a click.
+                markerpopover.remove_button.clicked()
+
+            original_popover_menu = Gtk.Popover.popup
+            Gtk.Popover.popup = popup
+            try:
+                event.type = Gdk.EventType.BUTTON_PRESS
+                event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
+                marker_box.do_button_press_event(event)
+
+                event.type = Gdk.EventType.DOUBLE_BUTTON_PRESS
+                event.guiEvent = Gdk.Event.new(Gdk.EventType.DOUBLE_BUTTON_PRESS)
+                marker_box.do_button_press_event(event)
+            finally:
+                Gtk.Popover.popup = original_popover_menu
 
         self.assert_markers(markers, [])
 
@@ -122,24 +135,27 @@ class TestMarkers(common.TestCase):
         event = mock.Mock(spec=Gdk.EventButton)
         event.x = x
         event.y = 1
-        event.type = Gdk.EventType.BUTTON_PRESS
         event.button = Gdk.BUTTON_PRIMARY
 
         with mock.patch.object(Gtk, "get_event_widget") as get_event_widget:
             get_event_widget.return_value = marker.ui
 
             def popup(markerpopover):
-                text_buffer = markerpopover.text_view.get_buffer()
+                # The popover is becoming visible, so we simulate the user entering text.
+                text_buffer = markerpopover.comment_textview.get_buffer()
                 text_buffer.set_text("com")
                 text_buffer.set_text("comment")
                 markerpopover.popdown()
+
             original_popover_menu = Gtk.Popover.popup
             Gtk.Popover.popup = popup
             try:
+                event.type = Gdk.EventType.BUTTON_PRESS
                 event.guiEvent = Gdk.Event.new(Gdk.EventType.BUTTON_PRESS)
                 marker_box.do_button_press_event(event)
-                event.guiEvent = Gdk.Event.new(Gdk.EventType.DOUBLE_BUTTON_PRESS)
+
                 event.type = Gdk.EventType.DOUBLE_BUTTON_PRESS
+                event.guiEvent = Gdk.Event.new(Gdk.EventType.DOUBLE_BUTTON_PRESS)
                 marker_box.do_button_press_event(event)
             finally:
                 Gtk.Popover.popup = original_popover_menu