Re: [tim-janik/beast] BSE: fix undo problems that occur after removing a bus (#82)

[Stefan Westerfeld <notifications github com>]

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, view it on GitHub, or mute the thread.