Thanks.Hi Philippe Thanks for the preliminary look at this work. It looks very interesting.
Until now, Mixer channel names were "Ch 1", "Ch 2", ... whatever they are inputs or outputs.On Tue, May 14, 2013 at 08:19:56PM +0200, Philippe Carriere wrote: > I worked last week in order to provide additional "per output setting" > tabs in MatrixMixer, something which might make things easier when large > number of Mixer inputs/outputs are involved. My understanding of this is that you present a series of tabs - one for each output - which are populated with sliders representing the send level from each input to that output. If this is correct then it sounds like a worthwhile addition. > I also tried to clarify the naming of channels. In what way?
Oh, sorry, I should have use the "generic dice_eap", not the Saffire Pro (which rather refer to older Saffire Pro). So I mean I was just able to test for the generic DICE EAP case (with Saffire Pro 40 as the testing device).> Now I know RME devices also use MatrixMixer and I am not able to test > for them, only know it works fine for Saffire Pro. A number of devices use the MatrixMixer object. Those I can spot immediately are all the Saffires (not just the "pro"), all the RMEs and the generic DICE mixer.
It's true that I could have made the corresponding changes ...> Could you have a test with this patch ? Probably, it would at least > require some modification in rme.py as in generic_dice_eap.py > (essentially eliminating the scrollarea at this level, scrollarea being > created further). Sure. Note that when making API changes like this the usual convention in the FOSS world is to update all the code which uses the thing you're changing as part of the patch. Obviously you can't necessarily test the changes, but in most cases the alternations needed are very mechanical in nature. In situations where there are fundamental changes in the behaviour, it is sometimes appropriate to retain the old behaviour as an option or activate the new with an option.
I sent you additional patches via another email.Of course at this early stage, there would be no reason to do any of this in your personal repo because the changes needed may change as you further develop the patch. But once the way forward is known you should at least propose the changes for the other users of the MatrixMixer widget to give the maintainers of that code something to evaluate. But see below for further thoughts in relation to this. > I will have some further modifications after these ones for automatically > refreshing the routing in matrixmixer when needed, but first I await for > your answer. I will only be able to test this later in the week. In the meantime, it would be helpful to me if you can clarify what it is that you're adding here.
Yes, of course; these patches are only draft ones extracted from my own git repo. Before submitting, I usually re-operate to produce more patches each focusing on a specific modification.A high-level comment: when you come to merge this work later it would be good to split the patch up into logically separated patches. It seems that there are a number of changes in this patch which could quite easily be considered separately. For example, the ability to name the I/Os does not depend on the "per output setting" code as such. This produces a clearer history in the repo. It also means that reasonably trivial changes (such as the editable labels) can go in quickly, making it easier to evaluate the more complex issues.
I have no possibilty for testing right now: it looks like I omitted to erase the CR ...Some general comments about the patch follow. > diff --git a/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py b/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py > index 9c06f4b..1bed732 100644 > --- a/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py > +++ b/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py > @@ -1,6 +1,7 @@ > # coding=utf8 > # > # Copyright (C) 2009 by Arnold Krille > +# Copyright (C) 2013 by Philippe Carriere > # > # This file is part of FFADO > # FFADO = Free Firewire (pro-)audio drivers for linux > @@ -215,34 +216,96 @@ class MixerChannel(QtGui.QWidget): > QtGui.QWidget.__init__(self, parent) > layout = QtGui.QGridLayout(self) > self.number = number > - if name != "": > - name = "\n(%s)" % name Any idea why the CR was being added here in the first place? What is achieved by removing it?
Yes, downsizing so much is probably not a good idea.> self.name = name > self.lbl = QtGui.QLabel(self) > self.lbl.setAlignment(Qt.Qt.AlignCenter) > + > + # Names may be quite long > + font = self.lbl.font() > + font.setPointSize(font.pointSize()/1.5) > + self.lbl.setFont(font) > + > + # Might be shortened more > if (smallFont): > font = self.lbl.font() > - font.setPointSize(font.pointSize()/1.5) > + font.setPointSize(font.pointSize()/2.0) > self.lbl.setFont(font) > - layout.addWidget(self.lbl, 0, 0, 1, 2) > + layout.addWidget(self.lbl, 0, 0, 1, 1) > + I gather here that you're reducing the font size to account for longer names. In theory this isn't a bad idea, but I'm concerned that the resulting labels might be too small for many people. I'll see what the end result is and comment further on this later.
Or let the user choose the downsizing (like he can choose to hide the sources/destinations names).Rather than statically setting the font size like this I wonder whether it might be better to downscale the font size only if necessary. That is, if the name is longer than a certain size then select a smaller font.
Of course.This font size change would be a good candidate for splitting out and treating as a separate patch.
Well, here is a point: as far as file saving of mixer/router/monitoring setting is not yet developed, the user will have to label each time a new session is started.> + # User Out/In naming utility > + self.edit = QtGui.QLineEdit(self) > + self.edit.setAlignment(Qt.Qt.AlignCenter) > + layout.addWidget(self.edit, 1, 0, 1, 1) > + self.connect(self.edit, QtCore.SIGNAL("textChanged(const QString &)"), self.editChannelName) > + This is an interesting addition and could be quite useful. However, it's really only worth doing if there's a way to save the user labels. Otherwise you have to go through and label them all every time. Do you have any thoughts about that?
OK, I will correct this.This is another change which could be split out and considered separately I think. > self.hideChannel(False) > > self.setContextMenuPolicy(Qt.Qt.ActionsContextMenu) > > + # Hide almost completely the channel > action = QtGui.QAction("Make this channel small", self) > action.setCheckable(True) > self.connect(action, QtCore.SIGNAL("triggered(bool)"), self.hideChannel) > self.addAction(action) > The added comment is unnecessary. The code makes the intent clear. > + # Hide label and edit line for this channel and replace it by the channel number or vice versa > + # Then emit a signal for further use I have a slight preference that comments be wrapped at 80 characters, but it's a minor thing in the scheme of things.
Reading your comment, I thought that there is still some adjustments to do (most probably rather concerning the additional patches I sent you) for the single output case: it is intended to work, but possibly python will complain at some points (typically, changing the value of something which does not exist).The rest of the patch deals almost exclusively with the new "per output" tab thing which I'll look at in more detail when I'm testing it. In general I think this is a very worthwhile feature and I'm keen to see this refined and ultimately merged. I have a few other things to bring up for your to ponder. The first is that there are some use cases where the matrix mixer is used with N inputs and a single "output" as an easy way to construct an array of controls (the RME does this for example). When this is done, the option of having sliders is very attractive. I mention this so you can ensure that the new code behaves sensibly even when only one output is specified.
Well, as the per output sections are only additional ones, nothing is intended to be changed for the ones who would exclusively use the matrix view; the only change is about scrollarea but I am even not sure that the modification is really imperatively required (I will test again for this, but, if I remember well, it worked for a while before I modified it).Related to this, I wonder how complex it might be to make the the "per output" tabs optional, selected via a parameter to the constructor? The code seems reasonably clean so this could be easy to do (however I'll admit that there still may be some problems lurking under the surface which may prevent this from being easy). The advantage here is that all existing users of the matrixmixer widget would retain their existing behaviour without any changes required within them. The per output tabs could then be enabled as people have the opportunity to test and modify as required. This then would remove the need for you to worry about updating mixers other than the one you are specifically working on (the Saffire Pro). This was the approach I took when I added the mute and phase inversion control extensions some time ago: by default nothing changed, with the new features only visible and useable when explicitly requested by the caller.
Yes, I saw these additional features: presently no modification are performed. Most probably they will fall to you (recalling that the matrix view whatever remains; so is it really imperative, at least at first, to implement these feature inside the per output view ?)The matrixmixer widget has some other optional functionality in addition to the faders: if suitable dbus paths are supplied it can also implement channel muting and phase inversion. At this stage I suspect the RME is the only device to make use of these features, and as a result it will probably fall to me to implement them in the per-output tabs if deemed important enough. However, I was wondering whether you had any thoughts as to how the inclusion of suitable switch controls might be done in the long run. To facilitate this sort of thing, I wonder whether it might be worth creating a top-level channel strip object, and into that place the fader that you're adding now. In time the "channel strip" container could then be extended with additional controls as required without the need to fundamentally change the structure.
Philippe Carriere <firstname.lastname@example.org>