This is being worked on by Paul Lange on his branch at: https://github.com/palango/pitivi/tree/new-layer-controls

Note that the current management of layers is completely screwed and we should get the basics of a proper layer management merged asap. This branch is only missing 1 feature (Add/remove layers) to be mergeable as far as feature is concerned.

Here is a first round of review on that branch:

layercontrol.py:
---------------
    * I would call layercontrol.py layer.py
    * Do not forget to add it to Makefile.am
    * You should rather use ges.TRACK_TYPE_XXX (or your enum) for layer type than a string and use the same map everywhere
    * Various properties that should be declared as private and are not (ex: self.solo_button self._solo_button), could even be local I believe

timeline.py:
-----------
    * Why stop using KW_LABEL_Y_OVERFLOW? If you do not need it, do not import it.A Same for TRACK_SPACING
    * You could set self.height to 21 in __init__ also please add a global constant for that crazy number

    * in _request_size():
        -    playhead_height = self.height + SPACING
        -    if playhead_height >= 0:
        -        self._playhead.props.height = playhead_height
        -    else:
        -        self._playhead.props.height = 0
        + self._playhead.props.height = max (0, self.height + SPACING)
    * def getYOfLayer(self, track_type, layer) is not realy optimal I think you could use the layer priority to get the same resul more efficiently
        * same for getHeightOfLayer
    * You should take care of resizing thumbnails when collapsing/uncollapsing layers, anyway low priority and I think we should get that branch merge without the collapsing option for now.

track.py:
--------
        def getHeight(self):
            track_type = self.track.props.track_type
            return self.app.gui.timeline_ui._controls.getHeightOfTrack(track_type)
        -> _controls is a private property

    height = property(getHeight)

Misc:
-----
 I am not sure about this commit, why is it needed? It looks subotpimal to me:
    1 commit 406b85497d9a21332e28d5e34ad0d80b0da5611a
    2 Author: Paul Lange <palango@gmx.de>
    3 Date:   Thu May 17 19:57:24 2012 -0500
    4X
    5     [WIP] handle initializationh

Hey Thibault,

thanks for the review. I fixed most stuff and but my questions/answers below!

timeline.py:
-----------
    * Why stop using KW_LABEL_Y_OVERFLOW? If you do not need it, do not import
it.A Same for TRACK_SPACING

- It just moved the track a bit down a bit in the canvas which misaligned it 
with the widgets, so I removed it

    * def getYOfLayer(self, track_type, layer) is not realy optimal I think you
could use the layer priority to get the same resul more efficiently
        * same for getHeightOfLayer
        
- I agree that it's not perfectly efficient, but I gives us perfect aligment 
and all necesarry information for folding layers as well which would give a
pretty hard calculation when doing it with the priority only.
        
    * You should take care of resizing thumbnails when collapsing/uncollapsing
layers, anyway low priority and I think we should get that branch merge without
the collapsing option for now.

- That's true, will do that later!

track.py:
--------
        def getHeight(self):
            track_type = self.track.props.track_type
            return
self.app.gui.timeline_ui._controls.getHeightOfTrack(track_type)
        -> _controls is a private property

    height = property(getHeight)
    
- I really need to access that, so I could change it into a public property!?

Misc:
-----
 I am not sure about this commit, why is it needed? It looks subotpimal to me:
    1 commit 406b85497d9a21332e28d5e34ad0d80b0da5611a
    2 Author: Paul Lange <palango@gmx.de>
    3 Date:   Thu May 17 19:57:24 2012 -0500
    4X
    5     [WIP] handle initializationh
    
- what looks supoptimal? It basically waits until all spaces are allocated and 
then aligns the canvas-contents based on that info.

Layer deletion is implemented, so branch is ready for review!

Here are some thoughts and nitpicks on the current state of the branch. 

> "My solution for [the problem of hardcoded layer heights] is that
> I adjust the layer height to the height of the layer control widget"

This sounds like the right approach to me too. A side-effect of this is that currently the height of the video layers is not the same as the height of the audio layers, which looks slightly odd visually. Even if we eventually allow users to resize layer heights to an arbitrary size, it would be nice to have all the layers have the same "default height". Perhaps you could measure the one with the biggest height allocation and use that as the standard "default height"?


- Ideally there should be a visual separation between layers in the layer controls; could be a gtk separator widget, or simply a different background color shading/brightness (if using the separator approach, we could measure that widget's allocated height to calculate the spacing between layers in the canvas UI).

- For the ability to make the container widget "selectable", perhaps you could ask on the GTK mailing list and get help more easily than on IRC.

Also, this might or might not help, but you could take a quick look at jokosher's (bzr branch lp:jokosher) ControlsBox.py which is used by InstrumentViewer.py (that's how they do their selectable track controls widget thing). It seems they use a GTK EventBox. I don't know if this would be the "right" approach or not, but it might give you some ideas.

- Delete button: your blog post from two weeks ago is correct, when we figure out how to make the layer control container widgets selectable we might as well remove that button. Deleting a layer is not something that you do as often as muting/soloing it, and the delete button is not only "dangerous", it's a bit too much visual crowding of that space. We can simply ask the user to use the "Timeline > Delete selected layer" menu instead, or right-click > "Delete layer".

- Layer reordering by drag and drop is missing :)

Drag and drop reordering is done.

Some missing things which shouldn't prevent you from reviewing ;)
- Same layer heights for audio/video
       * I would push that back a bit since it would change the logic quite a bit and doesn't seem to justify the work for me now

- Highlight active control when popup menu is shown
- Menu items for changing layer positioning

I'll look at the last two today or tomorrow

For the record, the initial implementation has been merged to master,

from commit d13a846e27528de8a0fbef80a1b76afc35f384bf
to commit 2fa49d9c5f4e4b554f4a47429d35082d72eb72ea

This bug has been migrated to https://phabricator.freedesktop.org/T2642.

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.