pitivi r1342 - in trunk: . pitivi/ui

[edwardrv svn gnome org]

Author: edwardrv
Date: Fri Oct 17 13:04:21 2008
New Revision: 1342
URL: http://svn.gnome.org/viewvc/pitivi?rev=1342&view=rev

Log:
	* pitivi/ui/complexinterface.py
	* pitivi/ui/complexlayer.py
	* pitivi/ui/complextimeline.py
	* pitivi/ui/timeline.py
	Code review

Modified:
   trunk/ChangeLog
   trunk/pitivi/ui/complexinterface.py
   trunk/pitivi/ui/complexlayer.py
   trunk/pitivi/ui/complextimeline.py
   trunk/pitivi/ui/timeline.py

Modified: trunk/pitivi/ui/complexinterface.py
==============================================================================
--- trunk/pitivi/ui/complexinterface.py	(original)
+++ trunk/pitivi/ui/complexinterface.py	Fri Oct 17 13:04:21 2008
@@ -49,6 +49,15 @@
 # . pixelToNs(pixels)
 # . nsToPixels(time)
 
+# FIXME: this might be poor design. while sharing the adjustment 
+# does provide an easy way of ensuring that all the UI elements are
+# updated, it's a little bit kludgey when it comes to sharing the
+# adjustment among elements which have children. In general, it migh
+# be better to factor this interface out into a separate Transformation
+# class which can handle the conversion between coordinate systems, for
+# both horizontal and vertical coordinates. This interface only handles
+# the horizontal.
+
 class Zoomable:
 
     zoomratio = 0
@@ -100,50 +109,3 @@
 
     def setChildZoomAdjustment(self, adj):
         pass
-
-# ZoomableObject(Zoomable)
-# -----------------------
-# Interface for UI widgets which wrap PiTiVi timeline objects.
-#
-# Methods:
-# . setObject
-# . getObject
-# . startDurationChanged
-# . getPixelPosition
-# . getPixelWidth
-
-class ZoomableObject(Zoomable):
-
-    object = None
-    width = None
-    position = None
-
-    def setTimelineObject(self, object):
-        if self.object:
-            self.object.disconnect(self._start_duration_changed_sigid)
-        self.object = object
-        if object:
-            self.start_duration_changed_sigid = object.connect(
-                "start-duration-changed", self._start_duration_changed_cb)
-
-    def getTimelineObject(self):
-        return self.object
-
-    def _start_duration_changed(self, object, start, duration):
-        self.width = self.nsToPixel(duration)
-        self.position = self.nsToPixel(start)
-        self.startDurationChanged()
-
-    def startDurationChanged(self):
-        """Overriden by subclasses"""
-        pass
-
-    def zoomChanged(self):
-        self._start_duration_changed(self.object, self.object.start,
-            self.object.duration)
-
-    def getPixelPosition(self):
-        return self.position
-
-    def getPixelWidth(self):
-        return self.width

Modified: trunk/pitivi/ui/complexlayer.py
==============================================================================
--- trunk/pitivi/ui/complexlayer.py	(original)
+++ trunk/pitivi/ui/complexlayer.py	Fri Oct 17 13:04:21 2008
@@ -54,6 +54,13 @@
 #       A layer was removed
 #
 
+# FIXME: this code has completely the wrong semantics. It's original intent
+# was to support layer editing (as-in priority), but we can do this in other
+# ways. Currently, it's actually used to implement tracks. In either case,
+# it's a needless level of indirection. Why can't the timeline composition
+# emit track-added, track-removed signals directly?
+
+
 class LayerInfo:
     """ Information on a layer for the complex timeline widgets """
 

Modified: trunk/pitivi/ui/complextimeline.py
==============================================================================
--- trunk/pitivi/ui/complextimeline.py	(original)
+++ trunk/pitivi/ui/complextimeline.py	Fri Oct 17 13:04:21 2008
@@ -46,6 +46,14 @@
 VIDEO_TRACK_HEIGHT = 50
 AUDIO_TRACK_HEIGHT = 20
 
+# FIXME: I like the idea of separating appearnce from implementation using
+# some scheme like this, but I'm not sure this implementation is the way to
+# go. The question is what will be the best way of letting people with good
+# aesthetic sense tweak the user interface so that it has a pleasing
+# appearance. It'd be good to build that support into the UI rather than
+# having to hack it in later. Unfortunately, these "style" objects aren't
+# powerful enough for that use, and are also tricky to use.
+
 # visual styles for sources in the UI
 VIDEO_SOURCE = (
     goocanvas.Rect,
@@ -139,6 +147,12 @@
 # TODO: replace this with custom cursor
 RAZOR_CURSOR = gtk.gdk.Cursor(gtk.gdk.XTERM)
 
+# FIXME: do we want this expressed in pixels or miliseconds?
+# If we express it in miliseconds, then we can have the core handle edge
+# snapping (it's really best implemented in the core). On the other hand, if
+# the dead-band is a constant unit of time, it will be too large at high zoom,
+# and too small at low zoom. So we might want to be able to adjust the
+# deadband from the UI.
 # default number of pixels to use for edge snaping
 DEADBAND = 5
 
@@ -171,12 +185,14 @@
 
     def __init__(self, *args, **kwargs):
         SmartGroup.__init__(self, *args, **kwargs)
+        # FIXME: all of these should be private
         self.widgets = {}
         self.elements = {}
         self.sig_ids = None
         self.comp = None
         self.object_style = None
 
+    # FIXME: this should be set_model(), overriding BaseView
     def set_composition(self, comp):
         if self.sig_ids:
             for sig in self.sig_ids:
@@ -187,6 +203,8 @@
             added = comp.connect("source-added", self._objectAdded)
             removed = comp.connect("source-removed", self._objectRemoved)
             self.sig_ids = (added, removed)
+            # FIXME: this is total crap right here. All tracks should be the
+            # same size. Maybe we have the audio track initially expanded.
             if comp.media_type == MEDIA_TYPE_VIDEO:
                 self.object_style = VIDEO_SOURCE
                 self.height = VIDEO_TRACK_HEIGHT
@@ -195,16 +213,28 @@
                 self.height = AUDIO_TRACK_HEIGHT
 
     def _objectAdded(self, unused_timeline, element):
+        # FIXME: here we assume that the object added is always a
+        # TimelineFileSource
         w = ComplexTimelineObject(element, self.comp, self.object_style)
+        # FIXME: this is crack: here, we're making the item itself draggable
+        # below, we're making the resize handles draggable. 
         make_dragable(self.canvas, w, start=self._start_drag,
             end=self._end_drag, moved=self._move_source_cb)
+        # FIXME: ideally the TimelineFileSource itself would handle this
+        # callback, but we control too much positioning here. We'd have to
+        # make the timeline object's zoomable, as well, and it makes it hard
+        # to do edge snapping, because we actually keep track of the edges
+        # here. Having the timeline objects do edge snapping would mean having
+        # each timeline object maintain a pointer to all the "edges" they'd
+        # have to snap. 
         element.connect("start-duration-changed", self.start_duration_cb, w)
         self.widgets[element] = w
         self.elements[w] = element
-        #element.set_data("widget", w)
         self.start_duration_cb(element, element.start, element.duration, w)
         self.add_child(w)
+        # FIXME: see util.py
         make_selectable(self.canvas, w.bg)
+        # FIXME: see util.py
         make_dragable(self.canvas, w.l_handle, 
             start=self._start_drag, moved=self._trim_source_start_cb,
             cursor=LEFT_SIDE)
@@ -237,6 +267,11 @@
         element.setStartDurationTime(max(self.canvas.snap_obj_to_edit(element,
             self.pixelToNs(pos[0])), 0))
 
+    # FIXME: these two methods should be in the ComplexTimelineObject class at least, or in
+    # their own class possibly. But they're here because they do
+    # edge-snapping. If we move edge-snapping into the core, this won't be a
+    # problem.
+
     def _trim_source_start_cb(self, item, pos):
         element = item.element
         cur_end = element.start + element.duration
@@ -267,6 +302,7 @@
         #FIXME: only for sources
         element.setMediaStartDurationTime(gst.CLOCK_TIME_NONE, new_duration)
 
+    # FIXME: this is part of the zoomable interface I want to get rid of
     def zoomChanged(self):
         """Force resize if zoom ratio changes"""
         for child in self.elements:
@@ -275,6 +311,13 @@
             duration = element.duration
             self.start_duration_cb(self, start, duration, child)
 
+# FIXME: a huge problem with the way I've implemented this is that the
+# property interface in goocanvas is a secondary interface. You're meant to
+# use transformation matrices and bounds. This caused problems for the simple
+# UI, because I wanted to implement expanding containers in the easiest way
+# possible (i.e., using signals) and no signals are sent when you reposition
+# an item with a transformation.
+
 class ComplexTimelineObject(goocanvas.Group):
 
     __gtype_name__ = 'ComplexTimelineObject'
@@ -301,6 +344,8 @@
             self.spacer]
         for thing in self.children:
             self.add_child(thing)
+
+        # FIXME: this is ghetto. 
         self.connect("notify::x", self.do_set_x)
         self.connect("notify::y", self.do_set_y)
         self.connect("notify::width", self.do_set_width)
@@ -360,6 +405,8 @@
         self.r_handle.props.height = height
         self._size_spacer()
 
+# FIXME: this class should be renamed CompositionTracks, or maybe just Tracks.
+
 class CompositionLayers(goocanvas.Canvas, Zoomable):
     """ Souped-up VBox that contains the timeline's CompositionLayer """
 
@@ -428,6 +475,14 @@
 ## nearest edit point. We do this here so we can keep track of edit points
 ## for all layers/tracks.
 
+    # FIXME: move this code into the core. The core should provide some method
+    # for being notified that updates need to happen, though in some cases
+    # we'll probably want this to update automatically. In other cases we'll
+    # want the UI to be able to disable it altogether. But what we're doing
+    # here is duplicating information that already exists in the core. As we
+    # add features to the core, like Critical Points (Keyframes), this code
+    # will have to be updated. Bad.
+
     def update_editpoints(self):
         #FIXME: this might be more efficient if we used a binary sort tree,
         # updated only when the timeline actually changes instead of before
@@ -480,6 +535,12 @@
 
 ## Editing Operations
 
+    # FIXME: here once again we're doing something that would be better done
+    # in the core. As we add different types of objects in the Core, we'll
+    # have to modify this code here (maybe there are different ways of
+    # deleting different objects: you might delete() a source, but unset() a
+    # keyframe)
+
     def deleteSelected(self, unused_action):
         for obj in self._selected_sources:
             if obj.comp:
@@ -488,6 +549,11 @@
         set_selection(self, set())
         return True
 
+
+    # FIXME: the razor is the one toolbar tool that violates the noun-verb
+    # principle. Do I really want to make an exception for this? What about
+    # just double-clicking on the source like jokosher?
+
     def activateRazor(self, unused_action):
         self._cursor = RAZOR_CURSOR
         # we don't want mouse events passing through to the canvas items
@@ -528,6 +594,10 @@
                     self.pixelToNs(x)))
         return True
 
+    # FIXME: this DEFINITELY needs to be in the core. Also, do we always want
+    # to split linked sources? Should the user be forced to un-link linked
+    # sources when they only wisth to split one of them? If not, 
+
     def _splitSource(self, obj, editpoint):
         comp = obj.comp
         element = obj.element
@@ -569,6 +639,10 @@
         new.setStartDurationTime(b_start, b_dur)
         comp.addSource(new, 0, True)
 
+    # FIXME: should be implemented in core, if at all. Another alternative
+    # would be directly suppporting ripple edits in the core, rather than
+    # doing select after + move selection. 
+
     def selectBeforeCurrent(self, unused_action):
         pass
 

Modified: trunk/pitivi/ui/timeline.py
==============================================================================
--- trunk/pitivi/ui/timeline.py	(original)
+++ trunk/pitivi/ui/timeline.py	Fri Oct 17 13:04:21 2008
@@ -27,7 +27,14 @@
 import gtk
 import gst
 
+# FIXME: this file is obsolete. we don't need this layer of indirection,
+# since we no longer need to switch between timelines. The DnD code 
+# should be moved to complex.py, and that file should be renamed
+# timeline.py
+
 import pitivi.instance as instance
+import dnd
+
 from pitivi.timeline.source import TimelineFileSource, TimelineBlankSource
 from pitivi.timeline.objects import MEDIA_TYPE_AUDIO, MEDIA_TYPE_VIDEO
 
@@ -79,12 +86,7 @@
             media_type=MEDIA_TYPE_VIDEO,
             name=filefactory.name)
 
-        # ONLY FOR SIMPLE TIMELINE : if video-only, we link a blank audio object
-        if not filefactory.is_audio:
-            audiobrother = TimelineBlankSource(factory=filefactory,
-                media_type=MEDIA_TYPE_AUDIO, name=filefactory.name)
-            source.setBrother(audiobrother)
-
+        # FIXME: access of instance.PiTiVi
         timeline = instance.PiTiVi.current.timeline
         if pos_ == -1:
             timeline.videocomp.appendSource(source)
@@ -95,7 +97,8 @@
             timeline.videocomp.prependSource(source)
 
     def _dragMotionCb(self, unused_layout, unused_context, x, y, timestamp):
-        #TODO: temporarily add source to timeline, and put it in drag mode
+
+        # FIXME: temporarily add source to timeline, and put it in drag mode
         # so user can see where it will go
         gst.info("SimpleTimeline x:%d , source would go at %d" % (x, 0))
 
@@ -107,12 +110,14 @@
         selection, targetType, timestamp):
         gst.log("SimpleTimeline, targetType:%d, selection.data:%s" % 
             (targetType, selection.data))
+        # FIXME: need to handle other types
         if targetType == dnd.TYPE_PITIVI_FILESOURCE:
             uri = selection.data
         else:
             context.finish(False, False, timestamp)
         self._gotFileFactory(instance.PiTiVi.current.sources[uri], x, y)
         context.finish(True, False, timestamp)
+        # FIXME: access of instance, and playground
         instance.PiTiVi.playground.switchToTimeline()