[Merge] lp:~phablet-team/messaging-framework/group-own-permissions into lp:messaging-framework

Tiago Salem Herrmann tiago.herrmann at canonical.com
Fri Aug 12 21:34:21 UTC 2016


Review: Needs Fixing

Looks good, just two comments.

Diff comments:

> 
> === modified file 'include/messaging/group_manager.h'
> --- include/messaging/group_manager.h	2016-07-01 13:46:08 +0000
> +++ include/messaging/group_manager.h	2016-08-10 11:47:46 +0000
> @@ -23,12 +23,22 @@
>  #include <messaging/interface.h>
>  #include <messaging/member.h>
>  #include <messaging/members.h>
> +#include <messaging/flags.h>
>  
>  #include <memory>
>  #include <chrono>
>  
>  namespace messaging
>  {
> +// Maintain sequence of 2 multiples if adding new values to preserve flags management
> +enum class GroupPermissions : uint
> +{
> +    CanChangeTitle  = 1 << 1,
> +    CanKick         = 1 << 2,
> +    CanSetAdmin     = 1 << 4,

I think CanSetAdmin should be 1 << 3 (which is 8) and CanDissolve should be 1 << 4 (which is 16). Isn't it?

> +    CanDissolve     = 1 << 8
> +};
> +
>  /// @brief GroupManager models a textual group conversation.
>  class MESSAGING_FW_PUBLIC GroupManager : public Interface, NonCopyable, NonMovable
>  {
> 
> === modified file 'src/messaging/qt/tp/text_channel.cpp'
> --- src/messaging/qt/tp/text_channel.cpp	2016-08-03 15:30:10 +0000
> +++ src/messaging/qt/tp/text_channel.cpp	2016-08-10 11:47:46 +0000
> @@ -347,7 +355,7 @@
>          }
>  
>          room_config_interface->setConfigurationRetrieved(true);
> -        room_config_interface->setCanUpdateConfiguration(true);
> +        room_config_interface->setCanUpdateConfiguration(group_manager->permissions().is_set(GroupPermissions::CanChangeTitle));

I don't think this is quite correct, but on the other hand I don't know the proper way to fix this, since not all properties in the Room Configuration can be updated in this protocol. I think we can keep like this for now. Maybe just add a comment?

>  
>          // FIXME: check what flags we want by default
>          Tp::ChannelGroupFlags groupFlags = Tp::ChannelGroupFlagCanAdd |


-- 
https://code.launchpad.net/~phablet-team/messaging-framework/group-own-permissions/+merge/302212
Your team Ubuntu Phablet Team is subscribed to branch lp:messaging-framework.



More information about the Ubuntu-reviews mailing list