[Merge] lp:~akulichalexander/telepathy-ofono/telepathy-ofono into lp:telepathy-ofono
Tiago Salem Herrmann
tiago.herrmann at canonical.com
Fri May 22 20:06:43 UTC 2015
Review: Needs Fixing
I left some comments below.
About backward compatibility, this isn't a problem since the service library is still static. No need #ifdef's in my opinion.
I will start testing the changes and will report here if I find any problem.
Thanks.
Diff comments:
> === modified file 'connection.cpp'
> --- connection.cpp 2015-03-31 16:41:22 +0000
> +++ connection.cpp 2015-05-18 13:35:37 +0000
> @@ -407,16 +407,22 @@
>
> Tp::DBusError error;
> bool yours;
> - QVariantMap hints;
> + QVariantMap request;
> +
> uint handle = ensureHandle(senderNormalizedNumber);
> qDebug() << "ensure handle" << senderNormalizedNumber << handle;
>
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")] = TP_QT_IFACE_CHANNEL_TYPE_TEXT;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".InitiatorHandle")] = handle;
> +
> if (initialInviteeHandles.size() > 0) {
> initialInviteeHandles << handle;
> - hints[TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles")] = QVariant::fromValue(initialInviteeHandles);
> - ensureChannel(TP_QT_IFACE_CHANNEL_TYPE_TEXT, Tp::HandleTypeNone, 0, yours, handle, false, hints, &error);
> + request[TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles")] = QVariant::fromValue(initialInviteeHandles);
> + ensureChannel(request, yours, false, &error);
> } else {
> - ensureChannel(TP_QT_IFACE_CHANNEL_TYPE_TEXT, Tp::HandleTypeContact, handle, yours, handle, false, hints, &error);
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandleType")] = Tp::HandleTypeContact;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")] = handle;
> + ensureChannel(request, yours, false, &error);
> }
>
> if(error.isValid()) {
> @@ -640,10 +646,9 @@
> return handles;
> }
>
> -Tp::BaseChannelPtr oFonoConnection::createTextChannel(uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error)
> +Tp::BaseChannelPtr oFonoConnection::createTextChannel(const QVariantMap &request, Tp::DBusError *error)
> {
> - Q_UNUSED(targetHandleType);
> + uint targetHandle = request.value(TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")).toUInt();
>
> if (mSelfPresence.type != Tp::ConnectionPresenceTypeAvailable) {
> error->set(TP_QT_ERROR_NETWORK_ERROR, "No network available");
> @@ -652,14 +657,14 @@
>
> QStringList phoneNumbers;
> bool flash = false;
> - if (hints.contains(TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles"))) {
> - phoneNumbers << inspectHandles(Tp::HandleTypeContact, qdbus_cast<Tp::UIntList>(hints[TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles")]), error);
> + if (request.contains(TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles"))) {
> + phoneNumbers << inspectHandles(Tp::HandleTypeContact, qdbus_cast<Tp::UIntList>(request[TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialInviteeHandles")]), error);
> } else {
> phoneNumbers << mHandles.value(targetHandle);
> }
>
> - if (hints.contains(TP_QT_IFACE_CHANNEL_INTERFACE_SMS + QLatin1String(".Flash"))) {
> - flash = hints[TP_QT_IFACE_CHANNEL_INTERFACE_SMS + QLatin1String(".Flash")].toBool();
> + if (request.contains(TP_QT_IFACE_CHANNEL_INTERFACE_SMS + QLatin1String(".Flash"))) {
> + flash = request[TP_QT_IFACE_CHANNEL_INTERFACE_SMS + QLatin1String(".Flash")].toBool();
> }
>
> oFonoTextChannel *channel = new oFonoTextChannel(this, phoneNumbers, flash);
> @@ -689,16 +694,16 @@
> }
> }
>
> -Tp::BaseChannelPtr oFonoConnection::createCallChannel(uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error)
> +Tp::BaseChannelPtr oFonoConnection::createCallChannel(const QVariantMap &request, Tp::DBusError *error)
> {
> - Q_UNUSED(targetHandleType);
> + uint targetHandle = request.value(TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")).toUInt();
>
> bool success = true;
> QString newPhoneNumber = mHandles.value(targetHandle);
> bool available = (mSelfPresence.type == Tp::ConnectionPresenceTypeAvailable);
> - bool isConference = (hints.contains(TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialChannels")) &&
> - targetHandleType == Tp::HandleTypeNone && targetHandle == 0);
> + bool isConference = (request.contains(TP_QT_IFACE_CHANNEL_INTERFACE_CONFERENCE + QLatin1String(".InitialChannels")) &&
> + !request.contains(TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandleType")) &&
Tests are passing, but indeed this might cause a regression. Just to stay on the safe side, perhaps we can do something like this: http://pastebin.ubuntu.com/11291335/
> + !request.contains(TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")));
>
> if (!available && (isConference || !isEmergencyNumber(newPhoneNumber))) {
> error->set(TP_QT_ERROR_NETWORK_ERROR, "No network available");
> @@ -722,7 +727,7 @@
> return Tp::BaseChannelPtr();
> }
>
> - QDBusObjectPath objpath(hints["ofonoObjPath"].toString());
> + QDBusObjectPath objpath(request["ofonoObjPath"].toString());
>
> if (objpath.path().isEmpty()) {
> objpath = mOfonoVoiceCallManager->dial(newPhoneNumber, "", success);
> @@ -787,13 +792,14 @@
> }
>
>
> -Tp::BaseChannelPtr oFonoConnection::createChannel(const QString& channelType, uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error)
> +Tp::BaseChannelPtr oFonoConnection::createChannel(const QVariantMap &request, Tp::DBusError *error)
> {
> + const QString channelType = request.value(TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")).toString();
> +
> if (channelType == TP_QT_IFACE_CHANNEL_TYPE_TEXT) {
> - return createTextChannel(targetHandleType, targetHandle, hints, error);
> + return createTextChannel(request, error);
> } else if (channelType == TP_QT_IFACE_CHANNEL_TYPE_CALL) {
> - return createCallChannel(targetHandleType, targetHandle, hints, error);
> + return createCallChannel(request, error);
> } else {
> error->set(TP_QT_ERROR_NOT_IMPLEMENTED, "Channel type not available");
> }
> @@ -835,7 +841,15 @@
> Tp::DBusError error;
> bool yours;
> uint handle = newHandle(normalizedNumber);
> - ensureChannel(TP_QT_IFACE_CHANNEL_TYPE_TEXT,Tp::HandleTypeContact, handle, yours, handle, false, QVariantMap(), &error);
> +
> + QVariantMap request;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")] = TP_QT_IFACE_CHANNEL_TYPE_TEXT;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandleType")] = Tp::HandleTypeContact;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")] = handle;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".InitiatorHandle")] = handle;
> +
> + ensureChannel(request, yours, false, &error);
> +
> if(error.isValid()) {
> qWarning() << "Error creating channel for incoming message" << error.name() << error.message();
> return;
> @@ -872,7 +886,14 @@
> QVariantMap hints;
> hints[TP_QT_IFACE_CHANNEL_INTERFACE_SMS + QLatin1String(".Flash")] = flash;
> uint handle = newHandle(normalizedNumber);
> - ensureChannel(TP_QT_IFACE_CHANNEL_TYPE_TEXT,Tp::HandleTypeContact, handle, yours, handle, false, hints, &error);
> +
> + QVariantMap request;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")] = TP_QT_IFACE_CHANNEL_TYPE_TEXT;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandleType")] = Tp::HandleTypeContact;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")] = handle;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".InitiatorHandle")] = handle;
> +
> + ensureChannel(request, yours, false, &error);
> if(error.isValid()) {
> qWarning() << "Error creating channel for incoming message" << error.name() << error.message();
> return;
> @@ -927,24 +948,10 @@
> return newHandle(normalizedNumber);
> }
>
> -Tp::BaseChannelPtr oFonoConnection::ensureChannel(const QString &channelType, uint targetHandleType,
> - uint targetHandle, bool &yours, uint initiatorHandle,
> - bool suppressHandler,
> - const QVariantMap &hints,
> - Tp::DBusError* error)
> +bool oFonoConnection::matchChannel(const Tp::BaseChannelPtr &channel, const QVariantMap &request, Tp::DBusError *error)
I think having this method "matchChannel()" is good, but I think we have to add another check before calling BaseConnection::matchChannel().
This other MR adds a fix for the MMS group chat case: https://code.launchpad.net/~tiagosh/telepathy-ofono/fix-mms-group-chat/+merge/259818
Could you add this check to this MR as well before we approve?
Just to give you some context, on telepathy-ofono we can have multiple MMS group chat channels with targetHandleType = None and targetHandle = 0.
The MMS chat rooms have no persistent ID. They are based on recipients.
> {
> - // we only reuse old text channels
> - if (channelType == TP_QT_IFACE_CHANNEL_TYPE_TEXT) {
> - Q_FOREACH(oFonoTextChannel *channel, mTextChannels) {
> - if (channel->baseChannel()->targetHandleType() == targetHandleType
> - && channel->baseChannel()->targetHandle() == targetHandle) {
> - yours = false;
> - return channel->baseChannel();
> - }
> - }
> - }
> - yours = true;
> - return Tp::BaseConnection::createChannel(channelType, targetHandleType, targetHandle, initiatorHandle, suppressHandler, hints, error);
> + // we only match text channels
> + return (request.value(TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")).toString() == TP_QT_IFACE_CHANNEL_TYPE_TEXT) && BaseConnection::matchChannel(channel, request, error);
> }
>
> void oFonoConnection::onOfonoCallAdded(const QString &call, const QVariantMap &properties)
> @@ -986,9 +993,15 @@
> qDebug() << "initiatorHandle " <<initiatorHandle;
> qDebug() << "handle" << handle;
>
> - QVariantMap hints;
> - hints["ofonoObjPath"] = call;
> - Tp::BaseChannelPtr channel = ensureChannel(TP_QT_IFACE_CHANNEL_TYPE_CALL, Tp::HandleTypeContact, handle, yours, initiatorHandle, false, hints, &error);
> + QVariantMap request;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")] = TP_QT_IFACE_CHANNEL_TYPE_CALL;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandleType")] = Tp::HandleTypeContact;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".TargetHandle")] = handle;
> + request[TP_QT_IFACE_CHANNEL + QLatin1String(".InitiatorHandle")] = initiatorHandle;
> + request["ofonoObjPath"] = call;
> +
> + Tp::BaseChannelPtr channel = ensureChannel(request, yours, false, &error);
> +
> if (error.isValid() || channel.isNull()) {
> qWarning() << "error creating the channel " << error.name() << error.message();
> return;
>
> === modified file 'connection.h'
> --- connection.h 2014-10-09 19:31:56 +0000
> +++ connection.h 2015-05-18 13:35:37 +0000
> @@ -71,8 +71,7 @@
>
> QStringList inspectHandles(uint handleType, const Tp::UIntList& handles, Tp::DBusError *error);
> Tp::UIntList requestHandles(uint handleType, const QStringList& identifiers, Tp::DBusError* error);
> - Tp::BaseChannelPtr createChannel(const QString& channelType, uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error);
> + Tp::BaseChannelPtr createChannel(const QVariantMap &request, Tp::DBusError *error);
> Tp::ContactAttributesMap getContactAttributes(const Tp::UIntList &handles, const QStringList &ifaces, Tp::DBusError *error);
> uint setPresence(const QString& status, const QString& statusMessage, Tp::DBusError *error);
> void connect(Tp::DBusError *error);
> @@ -103,15 +102,11 @@
>
> uint ensureHandle(const QString &phoneNumber);
> oFonoTextChannel* textChannelForMembers(const QStringList &members);
> - Tp::BaseChannelPtr createTextChannel(uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error);
> - Tp::BaseChannelPtr createCallChannel(uint targetHandleType,
> - uint targetHandle, const QVariantMap &hints, Tp::DBusError *error);
> - Tp::BaseChannelPtr ensureChannel(const QString &channelType, uint targetHandleType,
> - uint targetHandle, bool &yours, uint initiatorHandle,
> - bool suppressHandler,
> - const QVariantMap &hints,
> - Tp::DBusError* error);
> + Tp::BaseChannelPtr createTextChannel(const QVariantMap &request, Tp::DBusError *error);
> + Tp::BaseChannelPtr createCallChannel(const QVariantMap &request, Tp::DBusError *error);
> +
> + bool matchChannel(const Tp::BaseChannelPtr &channel, const QVariantMap &request, Tp::DBusError *error);
> +
> QDBusObjectPath sendMMS(const QStringList &numbers, const OutgoingAttachmentList& attachments);
>
>
>
> === modified file 'ofonocallchannel.cpp'
> --- ofonocallchannel.cpp 2015-02-17 17:03:42 +0000
> +++ ofonocallchannel.cpp 2015-05-18 13:35:37 +0000
> @@ -33,7 +33,7 @@
> mMultiparty(false)
>
> {
> - Tp::BaseChannelPtr baseChannel = Tp::BaseChannel::create(mConnection, TP_QT_IFACE_CHANNEL_TYPE_CALL, targetHandle, Tp::HandleTypeContact);
> + Tp::BaseChannelPtr baseChannel = Tp::BaseChannel::create(mConnection, TP_QT_IFACE_CHANNEL_TYPE_CALL, Tp::HandleTypeContact, targetHandle);
> Tp::BaseChannelCallTypePtr callType = Tp::BaseChannelCallType::create(baseChannel.data(),
> true,
> Tp::StreamTransportTypeUnknown,
>
> === modified file 'ofonoconferencecallchannel.cpp'
> --- ofonoconferencecallchannel.cpp 2015-01-26 18:52:02 +0000
> +++ ofonoconferencecallchannel.cpp 2015-05-18 13:35:37 +0000
> @@ -36,7 +36,7 @@
> }
> }
>
> - Tp::BaseChannelPtr baseChannel = Tp::BaseChannel::create(mConnection, TP_QT_IFACE_CHANNEL_TYPE_CALL, 0, Tp::HandleTypeNone);
> + Tp::BaseChannelPtr baseChannel = Tp::BaseChannel::create(mConnection, TP_QT_IFACE_CHANNEL_TYPE_CALL);
> Tp::BaseChannelCallTypePtr callType = Tp::BaseChannelCallType::create(baseChannel.data(),
> true,
> Tp::StreamTransportTypeUnknown,
>
> === modified file 'ofonotextchannel.cpp'
> --- ofonotextchannel.cpp 2014-11-19 21:55:35 +0000
> +++ ofonotextchannel.cpp 2015-05-18 13:35:37 +0000
> @@ -54,13 +54,11 @@
> if (phoneNumbers.size() == 1) {
> baseChannel = Tp::BaseChannel::create(mConnection,
> TP_QT_IFACE_CHANNEL_TYPE_TEXT,
> - mConnection->ensureHandle(mPhoneNumbers[0]),
> - Tp::HandleTypeContact);
> + Tp::HandleTypeContact,
> + mConnection->ensureHandle(mPhoneNumbers[0]));
> } else {
> baseChannel = Tp::BaseChannel::create(mConnection,
> - TP_QT_IFACE_CHANNEL_TYPE_TEXT,
> - 0,
> - Tp::HandleTypeNone);
> + TP_QT_IFACE_CHANNEL_TYPE_TEXT);
> }
> Tp::BaseChannelTextTypePtr textType = Tp::BaseChannelTextType::create(baseChannel.data());
> baseChannel->plugInterface(Tp::AbstractChannelInterfacePtr::dynamicCast(textType));
>
> === modified file 'protocol.cpp'
> --- protocol.cpp 2014-09-23 09:44:37 +0000
> +++ protocol.cpp 2015-05-18 13:35:37 +0000
> @@ -40,7 +40,7 @@
>
> Tp::BaseConnectionPtr Protocol::createConnection(const QVariantMap ¶meters, Tp::DBusError *error) {
> Q_UNUSED(error);
> - Tp::BaseConnectionPtr connection_ptr = Tp::BaseConnection::create<oFonoConnection>(QDBusConnection::sessionBus(), "ofono", name().toLatin1(), parameters);
> + Tp::BaseConnectionPtr connection_ptr = Tp::BaseConnection::create<oFonoConnection>("ofono", name().toLatin1(), parameters);
> connect(
> static_cast<oFonoConnection*>(connection_ptr.data()), &oFonoConnection::activeAudioOutputChanged,
> Tp::memFun(&mAudioModeMediator, &PowerDAudioModeMediator::audioModeChanged)
>
--
https://code.launchpad.net/~akulichalexander/telepathy-ofono/telepathy-ofono/+merge/259379
Your team Ubuntu Phablet Team is subscribed to branch lp:telepathy-ofono.
More information about the Ubuntu-reviews
mailing list