After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 608682 - markers / regions
markers / regions
Status: RESOLVED OBSOLETE
Product: pitivi
Classification: Other
Component: Timeline
Git
Other Linux
: Normal enhancement
: Git
Assigned To: Pitivi maintainers
Pitivi maintainers
Depends on:
Blocks: 629131
 
 
Reported: 2010-02-01 14:11 UTC by Jean-François Fortin Tam
Modified: 2015-10-20 13:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch submitted by Robin Norwood (14.50 KB, patch)
2010-03-02 23:44 UTC, Brandon Lewis
needs-work Details | Review
Mockp-up about UI modifications for markers (271.33 KB, image/png)
2010-12-22 19:23 UTC, Jean-Philippe Fleury
  Details

Description Jean-François Fortin Tam 2010-02-01 14:11:05 UTC
The ability to add markers and regions, which would appear above the ruler (or maybe below it?) and be used for arbitrary visual reminders for the user (ex: identify a portion of the timeline as "scene 44" or "needs color grading", etc.). They should be label-able, movable, deletable, etc.

Markers would be added with an Insert -> Marker menu item command, or with a shortcut such as "m". When adding a marker, its label should be highlighted in edit mode to allow typing a name directly. Pressing Enter or clicking anywhere on the timeline would exit the label editing mode.

"Region markers" are a set of two markers that are linked together and delimit a region. Double-clicking on the ruler between those markers (or double-clicking one of the two markers) allows selecting the region between them.

I know that emdash had this feature somewhere on the back of his mind, but I don't think it was operationalized in a bug report, and I think it could make a nice addition to the "PiTiVi Love" list. It would be an easy an fun feature for someone to get acquainted with the pitivi timeline/canvas/ruler.
Comment 1 Jean-François Fortin Tam 2010-02-01 14:23:55 UTC
Ah, just remembered that regions would depend on the ability to create regions in the first place; that means that when dragging an empty space onto the timeline canvas, not only should the blue "rectangle selection" box be used, but an additional "region" selection should be made based on the X coordinates of that box, to create a "region". After that, the user could add "region markers" around that region by using "Insert -> Region markers" or hitting "r". If a region is not selected, the menu item should of course be greyed out (insensitive).
Comment 2 Brandon Lewis 2010-03-02 23:44:47 UTC
Created attachment 155085 [details] [review]
Patch submitted by Robin Norwood

This patch is a good start, but I have some critiques.

In general, this code is broadly similar to the Keyframe/Interpolator code, 
and you should have a look in pitivi/ui/curve.py, which implements the UI 
side of the keyframe curve. 

marks.py: a mark is part of the timeline data, and so the file should
be located in pitivi/timeline/

diff --git a/pitivi/marks.py b/pitivi/marks.py
new file mode 100644
index 0000000..80f67bc
--- /dev/null
+++ b/pitivi/marks.py

...

+
+class Mark(Loggable):
+    """
+    Base class for L{TimeMark} and L{RegionMark}.
+
+    @ivar label: Label of this mark.
+    @type label: L{string}
+    """
+
+    def __init__(self, label=None):
+        Loggable.__init__(self)
+        self.label = label

It's probably not the best idea to store color information in this class. For
one thing, this is a PiTiVi core class, and color is a UI notion. There are 
different representations for color and we don't want to assume a particular
one. Finally, are we even sure that marks should have different colors? 

+        self.rgb = (1.0, 0.0, 0.0)
+

This class should be signallable, an emit a signal when the time changes.
See the Keyframe / Interpolator classes in pitivi/timeline/track.py.

+
+class TimeMark(Mark):
+    """
+    A Mark at a specific point in time.
+
+    @ivar time: Time at which the mark occurs (nanoseconds).
+    @type time: L{long}
+    """
+
+    def __init__(self, when, label=None, rgb=None):
+        Mark.__init__(self, label)
+        if not rgb:
+            rgb = (1.0, 0.0, 0.0)
+        self.time = when
+
+

Ditto here, but also emit a signal for duration.

+class RegionMark(Mark):
+    """
+    A Mark for a region on a timeline, with a start time and duration.
+
+    @ivar start: Time at which the marked region starts (nanoseconds).
+    @type start: L{long}
+    @ivar duration: Duration of marked region (nanoseconds).
+    @type duration: L{long}
+    """
+
+    def __init__(self, when, duration):
+        Mark.__init__(self)
+        self.start = when
+        self.duration = duration
+
+
+class MarkList(Loggable, Signallable):
+    """
+    A list of Mark objects attached to a Timeline.
+
+    @ivar timeline: Timeline the marks are for.
+    @type timeline: L{Timeline}
+    @ivar marks: List of Marks on the timeline.
+    @type marks: L{list}
+    """
+
+    def __init__(self, timeline):
+        Loggable.__init__(self)
+        Signallable.__init__(self)

It's not necessary to call Signallable.__init__()

+
+        self.timeline = timeline
+
+        self.marks = []
+

The remaining methods should emit signals whenever a mark is added or removed.
It would probably be better to avoid the use of *args and **kwargs here. There's
only two parameters, and it's probably better to be specific. 

Stylistically, it would be better to have addMark/removeMark take a mark object
an input parameter. If you also want to write a convenience function that creates
the mark object for you, that's fine -- but it should have a different name.

So my suggestion here is rename append to addMark, rename delete to removeMark
or deleteMark. If you want to factor common functionality into a helper routine,
that's fine too (but consider whether the routine should be public or not).


+    def addTimeMark(self, *args, **kwargs):
+        """
+        Add a mark at a specific time.
+
+        @returns: The mark just added.
+        @rtype: L{TimeMark}
+        """
+        mark = TimeMark(*args, **kwargs)
+        self.marks.append(mark)
+
+        return mark
+
+    def addRegionMark(self, *args, **kwargs):
+        """
+        Add a region mark at a specific time.
+
+        @returns: The mark just added.
+        @rtype: L{RegionMark}
+        """
+        mark = RegionMark(*args, **kwargs)
+        self.marks.append(mark)
+
+        return mark
+
+    def append(self, mark):
+        """
+        Add a Mark object.
+
+        @ivar mark: Mark object
+        @type mark: L{Mark}
+        """
+

It's not clear to me why you delete the mark here. If it's an error to add
a mark more than once to the same MarkList, you should throw an exception. 

+        # Delete existing mark, if any.
+        self.delete(mark)
+        self.marks.append(mark)
+
+        return
+
+    def delete(self, mark):
+        """
+        Delete a Mark object, if it exists.
+
+        @ivar mark: Mark object
+        @type mark: L{Mark}
+        """

Here you should just call self.marks.remove(mark). I

+        self.marks = [ m for m in self.marks if m.time != mark.time ]
+
+        return
+
+    def __iter__(self):
+        return getattr(self.marks, '__iter__')()

At first glance, the etree stuff is fine (except for afore-mentioned presence
of color information). The only thing missing are unit tests. 

In timeline.py, the marks should be included in the list of edges (see the
TimelineEdges class) so that clips will snap to the marker locations during drag
operations.

On the UI side, the idea is that you connect to the signals of the model and
update the display in response. Since your model doesn't emit any signals, you
obviously aren't doing that here. So the first change I would make is to only
request a re-draw when you receive a signal from the model, as we do for the 
seek position. Notice that the event handlers for the mouse events don't call
queue draw, they simply ask the pipeline to seek to a specific location. Only
when the pipeline position has changed do we update the ruler.


diff --git a/pitivi/ui/ruler.py b/pitivi/ui/ruler.py
index 6819f04..9e95694 100644
--- a/pitivi/ui/ruler.py
+++ b/pitivi/ui/ruler.py
...
+    def addMark(self, mark):
+        if mark.label and len(mark.label) > 1:
+            self.app.current.timeline.marks.append(mark)
+        self.doPixmap()
+        self.queue_draw()
+
+    def deleteMark(self, mark):
+        self.app.current.timeline.marks.delete(mark)
+        self.doPixmap()
+        self.queue_draw()
+
     def do_button_release_event(self, event):
         self.debug("button released at x:%d", event.x)
         self.pressed = False
@@ -166,8 +198,22 @@ class ScaleRuler(gtk.Layout, Zoomable, Loggable):
             # seek at position
             cur = self.pixelToNs(event.x)
             self._doSeek(cur)
+
         return False

The drawing code looks alright for now. Ideally you wouldn't draw it on
the pixpamp, but over it as we do with the timeline position indicator. This
way we don't have to refresh the entire pixmap every time the mark moves. Marks
should be draggable with the mouse, and to do that you'll need to check for hits.
 
         zoomRatio = self.zoomratio
         self.drawFrameBoundaries(allocation)
+        self.drawMarks(allocation, offset)
         self.drawTicks(allocation, offset, spacing, scale)
         self.drawTimes(allocation, offset, spacing, scale, layout)
 
@@ -347,6 +394,25 @@ class ScaleRuler(gtk.Layout, Zoomable, Loggable):
             paintpos += spacing
             seconds += interval
 
+    def drawMarks(self, allocation, offset):
+        if not self.app.current:
+            return
+
+        for mark in self.app.current.timeline.marks:
+            xpos = self.nsToPixel(mark.time) + self.border
+
+            if (xpos < self.pixmap_offset or
+                xpos > self.pixmap_offset + self.pixmap_allocated_width):
+                continue
+
+            xgc = self.bin_window.new_gc()
+            xgc.set_rgb_fg_color(gtk.gdk.Color(*mark.rgb))
+            xgc.set_line_attributes(10, gtk.gdk.LINE_SOLID, gtk.gdk.CAP_BUTT, gtk.gdk.JOIN_MITER)
+            self.pixmap.draw_line(
+                xgc,
+                xpos, 0,
+                xpos, allocation.height)
+
     def drawFrameBoundaries(self, allocation):
         ns_per_frame = float(1 / self.frame_rate) * gst.SECOND
         frame_width = self.nsToPixel(ns_per_frame)
@@ -380,3 +446,94 @@ class ScaleRuler(gtk.Layout, Zoomable, Loggable):
         context.stroke()
 
         context.restore()

I haven't actually run the patch, so I don't have much to say about the popup/
tool tip windows yet.

+
+
+class MarkTips(gtk.Window, Loggable):
+    def __init__(self, ruler, mark, x, y):
+        # FIXME: if the next line is used, I can't get the MarkTips window to receive key press events.
+#        gtk.Window.__init__(self, gtk.WINDOW_POPUP)
+        gtk.Window.__init__(self)
+        Loggable.__init__(self)
+
...
Comment 3 Robin Norwood 2010-03-03 03:07:25 UTC
Thanks for the feedback!  I'll look at implementing all of these suggestions.
Comment 4 Jean-Philippe Fleury 2010-12-22 19:23:53 UTC
Created attachment 176895 [details]
Mockp-up about UI modifications for markers

I didn't test the patch, so I don't know what are the UI modifications, but I would like to point the utility and the benefit to add in the playback toolbar buttons to go to previous and next marker. See this attached mock-up.
Comment 5 Thibault Saunier 2015-10-20 13:28:54 UTC
This bug has been migrated to https://phabricator.freedesktop.org/T2451.

Please use the Phabricator interface to report further bugs by creating a task and associating it with Project: Pitivi.

See http://wiki.pitivi.org/wiki/Bug_reporting for details.