Ok, I'll try to give a more detailed account of the events that lead to the
bug. For the sake of this discussion, we only need to do three things:
* create song
* create bus - this will be called **Bus-1**
* delete this bus - no undo step will be recorded (which causes problems later)
Why?
1. First let us instrument the function
```
void
bse_container_uncross_undoable (BseContainer *container,
BseItem *child)
{
BseItem *ancestor;
assert_return (BSE_IS_CONTAINER (container));
assert_return (BSE_IS_ITEM (child));
assert_return (child->parent == (BseItem*) container);
/* backup source channels state */
if (BSE_IS_SOURCE (child))
{
printf ("[1] clearing inputs and outputs for child: %s\n",
bse_object_debug_name (child));
bse_source_backup_ochannels_to_undo (BSE_SOURCE (child));
bse_source_clear_ochannels (BSE_SOURCE (child));
bse_source_backup_ichannels_to_undo (BSE_SOURCE (child));
bse_source_clear_ichannels (BSE_SOURCE (child));
}
/* dispose cross references, those backup themselves */
ancestor = BSE_ITEM (container);
do
{
bse_container_uncross_descendant (BSE_CONTAINER (ancestor), child);
ancestor = ancestor->parent;
}
while (ancestor);
}
```
with a printf satement, and repeat the steps. We get this output:
```
[1] clearing inputs and outputs for child: "BseBus::Bus-1"
```
So if we delete the bus **Bus-1**, the first thing that happens is that all
connections from and to the bus get removed in this function.
2. To see how this affects our undo step, lets instrument another related
function:
```
Bse::Error
bse_bus_disconnect (BseBus *self,
BseItem *trackbus)
{
BseSource *osource;
if (BSE_IS_TRACK (trackbus))
osource = bse_track_get_output (BSE_TRACK (trackbus));
else if (BSE_IS_BUS (trackbus))
osource = BSE_SOURCE (trackbus);
else
return Bse::Error::SOURCE_TYPE_INVALID;
if (!osource || !self->summation || !sfi_ring_find (self->inputs, trackbus))
return Bse::Error::SOURCE_PARENT_MISMATCH;
bse_object_unproxy_notifies (trackbus, self, "notify::inputs");
bse_object_unproxy_notifies (self, trackbus, "notify::outputs");
bse_item_cross_unlink (BSE_ITEM (self), BSE_ITEM (trackbus),
bus_uncross_input);
self->inputs = sfi_ring_remove (self->inputs, trackbus);
trackbus_update_outputs (trackbus, NULL, self);
Bse::Error error1 = bse_source_unset_input (self->summation, 0, osource, 0);
Bse::Error error2 = bse_source_unset_input (self->summation, 1, osource, 1);
printf ("[2] disconnecting summation %s output source %s:\n[2] errors: %s and
%s\n",
bse_object_debug_name (self->summation), bse_object_debug_name (osource),
bse_error_blurb (error1), bse_error_blurb (error2));
g_object_notify (G_OBJECT (self), "inputs");
g_object_notify (G_OBJECT (trackbus), "outputs");
return error1 != 0 ? error1 : error2;
}
```
which produces the output
```
[2] disconnecting summation "BseSummation::Summation" output source
"BseBus::Bus-1":
[2] errors: Input/Output channels not connected and Input/Output channels not
connected
```
(the numbers indicate order, so this happens after [1]). So what happens is
that this function returns an error code, because the connections between
**Summation** output source **Bus-1** don't exist (were removed in [1]).
3. Finally this is what causes the undo step recording to be omitted as we can
find by adding a printf to
```
Error
BusImpl::disconnect_bus (BusIface &busi)
{
BseBus *self = as<BseBus*>();
BusImpl &bus = dynamic_cast<BusImpl&> (busi);
Error error = bse_bus_disconnect (self, busi.as<BseItem*>());
printf ("[3] disconnect_bus returned: error %s\n", bse_error_blurb (error));
if (error == 0)
{
// an undo lambda is needed for wrapping object argument references
UndoDescriptor<BusImpl> bus_descriptor = undo_descriptor (bus);
auto lambda = [bus_descriptor] (BusImpl &self, BseUndoStack *ustack) ->
Error {
return self.connect_bus (self.undo_resolve (bus_descriptor));
};
push_undo (__func__, *this, lambda);
}
return error;
}
```
Which results in this output:
```
[3] disconnect_bus returned: error Input/Output channels not connected
```
So to summarize this, the output when deleting **Bus-1** will be
```
[1] clearing inputs and outputs for child: "BseBus::Bus-1"
[2] disconnecting summation "BseSummation::Summation" output source
"BseBus::Bus-1":
[2] errors: Input/Output channels not connected and Input/Output channels not
connected
[3] disconnect_bus returned: error Input/Output channels not connected
```
which we can summarize as the input/outputs of **Bus-1** are disconnected in
`bse_container_uncross_undoable` [1], due to this `bse_bus_disconnect` [2]
fails and finally the undo step is not recorded in `BusImpl::disconnect_bus`
[3]. The patch that I've proposed fixes this by ignoring the error code
Error::SOURCE_NO_SUCH_CONNECTION in [3]. Note that this doesn't irgnore all
possible errors, just specialcase the case that there was no connection.
> * Why should an undo step be recorded for restoring a connection that doesn't
> exist in the first place?
Because the connection did exist, but was removed in [1].
> * Also, with your code, calling disconnect_bus() repeatedly will record more
> and more undo steps, for a connection that's not existing and could have been
> restored at most only once anyway.
Honestly, I don't know the answer to this. Maybe the fix should go somewhere
else, or if we want to keep this, we could add state to the bus that indicates
whether the bus is connected or not (independantly of whether or not the
channels are connected).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/tim-janik/beast/pull/82#issuecomment-433392380_______________________________________________
beast mailing list
[email protected]
https://mail.gnome.org/mailman/listinfo/beast
