GNOME Bugzilla – Bug 608682
markers / regions
Last modified: 2015-10-20 13:28:54 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.
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).
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) + ...
Thanks for the feedback! I'll look at implementing all of these suggestions.
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.
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.