Bug 41356

Hey David, here goes some comments regarding the patch. Just some minor tweaks here and there

+
+/** Returns true if the provided message type is supported.
+  *
+  */
+
+bool TextChannel::supportsMessageType(ChannelTextMessageType messageType) const

Should become something like
+
+/** 
+ * Return whether the provided message type is supported.
+ * 
+ * This method requires TextChannel::FeatureMessageCapabilities to be ready.
+ *
+ * \param messageType The message type to check.
+ * \return \c true if supported, \c false otherwise
+ */
+bool TextChannel::supportsMessageType(ChannelTextMessageType messageType) const

Please also add a check for FeatureCapabilities being ready there and warn in case it's not and the method was called.

Also I would like to have a public QSet<ChannelTextMessageType> (or maybe QList?) TextChannel::supportedMessageTypes() const. Note that the method should not return a UIntList. It should also check for FeatureCapabilities and be properly documented.

Also rename the internal private variable holding the supported message types to something like "supportedMessageTypes" instead of "channelTextMessageTypes" ;)

(In reply to comment #1)
> Also I would like to have a public QSet<ChannelTextMessageType> (or maybe
> QList?) TextChannel::supportedMessageTypes() const. 

Such an accessor is probably mostly useful for populating drop down menus, radio button groups or alike used for choosing the type of message to send (having the available options there). supportsMessageType() already provides the answer to "does it contain type X", the most natural use for a QSet, hence QList<ChannelTextMessageType> would be most appropriate here.

QList is fine as the internal impl for both accessors too. As there are only a few message types, it's likely even faster than a QSet even for contains() (linear search from an array vs logarithmic search from a linked structure, where the total size is usually about equal to the logarithm base), and definitely faster to create.

*** Bug 31407 has been marked as a duplicate of this bug. ***

Comments addressed and added unit tests in my repo, branch available-message-types

Added branch to the URL

Here is a review for the Dario's branch:

+ * This method requires TextChannel::FeatureMessageCapabilities to be ready.
+ *
+ * \return The list of supported message types
+ */
+QList<ChannelTextMessageType> TextChannel::supportedMessageTypes() const
+{
+ if (!isReady(FeatureCore)) {
You should be checking for FeatureMessageCapabilities here.

bool TextChannel::supportsMessageType(ChannelTextMessageType messageType) const
{
+ if (!isReady(FeatureCore)) {
Same here

+ QVERIFY(mChan->supportsMessageType(static_cast<ChannelTextMessageType>(0U)));
Please add checks for supported and unsupported, also please use enums instead of "0U" here.

+ Q_FOREACH (uint messageType, messageTypesAsUIntList) {
Use foreach, emit, etc for internal code.

All fixed, f'pushed to available-message-types rebased on new master and available-message-types-0.8 rebased on 0.8

Please merge the branch on top of upstream/master but don't merge the 0.8 branch. Also please close this bug once the changes are merged.

After Andre's ++ on IRC, this change has been merged in master, with merge
commit id . Closing the bug

Sorry, didn't paste the commit id, which was 60702e3a2c74c9595ce34a4e74ccb0d040962ea2