API change proposal for Songbird::join and Songbird::join_gateway

[vilgotf]

Not being able to use ? on Songbird::join and Songbird::join_gateway is bothering me, so I propose that join takes a call as an argument instead of returning a new one.

so

fn join(songbird: Songbird, channel_id: ChannelId, guild_id: GuildId) -> Result<()> {
  let (_, res) = songbird.join(guild_id, channel_id).await;
  res
}

would be

fn join(songbird: Songbird, channel_id: ChannelId, guild_id: GuildId) -> Result<()> {
  let call = songbird.get_or_insert(guild_id);
  songbird.join(channel_id, call).await
}

Thoughts?

[vilgotf]

As a real world example
This:

async fn _join(&self, channel_id: ChannelId) -> Result<()> {
  let call = self.bot.songbird.get_or_insert(self.guild_id.into());
  {
    let mut lock = call.lock().await;
    if !lock.is_deaf() {
      lock.deafen(true).await?
    }
  }
  let (_, result) = self.bot.songbird.join(self.guild_id, channel_id).await;
  result

would turn into this:

async fn _join(&self, channel_id: ChannelId) -> Result<()> {
  let call = self.bot.songbird.get_or_insert(self.guild_id.into());
  {
    let mut lock = call.lock().await;
    if !lock.is_deaf() {
      lock.deafen(true).await?
    }
  }
  self.bot.songbird.join(channel_id, call).await
}

[FelixMcFelix]

I agree that being unable to just use ? to shortcut the return values for these methods is pretty bad. However, I think these suggestions also hurt ergonomics; users are now forced to get a Call before attempting to join a channel, at which point the Songbird manager object isn't actually needed any more from a functionality point of view: it just handles consistent lookup if GuildIDs->Calls, and this API might trick users into trying to make their own standalone Calls.

There might be cause for moving out the logic in Songbird::join/join_gateway into an associated function on Call (namely the risk of deadlock due to lock misuse):

impl Call {
    pub fn join_from_lock(self: &Arc<Mutex<Self>>) -> JoinResult<()> {
        // Safe handling of lock, await one, unlock, await other...
    }
}

...and then pushing users to prefer Songbird::join == Call::join_from_lock > Call::join.

[vilgotf]

Didn't think about new users fallaciously using Call::standalone()... I think a safe method on Call would be equally good although it kinda feels like a hack (why have a safe and an unsafe method?)

[FelixMcFelix]

It's not an unsafe method; there are just caveats to using a Call directly, and we can't assume that every user will have it in an Arc<Mutex<...>>. Join/join_gateway methods which act on the objects themselves cover this niche.

[vilgotf]

Another option is to not return Call