Bug 41356 - No method to retrieve available message types [patch]
Summary: No method to retrieve available message types [patch]
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-qt (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Dario Freddi
QA Contact: Telepathy bugs list
URL: http://cgit.collabora.com/git/user/dr...
Whiteboard: review+
Keywords: patch
: 31407 (view as bug list)
Depends on:
Blocks: 33115
  Show dependency treegraph
Reported: 2011-09-30 00:46 UTC by David Edmundson
Modified: 2011-12-05 09:38 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Patch (2.18 KB, patch)
2011-09-30 00:46 UTC, David Edmundson
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Edmundson 2011-09-30 00:46:56 UTC
Created attachment 51790 [details] [review]

Currently in TpQt4::TextChannel there's no way to get the supported
message types for a channel, so I don't know whether I should/can
handle "/me " type actions in my client code.
Comment 1 Andre Moreira Magalhaes 2011-09-30 08:05:13 UTC
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" ;)
Comment 2 Olli Salli 2011-10-04 03:56:11 UTC
(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.
Comment 3 Olli Salli 2011-10-07 07:03:04 UTC
*** Bug 31407 has been marked as a duplicate of this bug. ***
Comment 4 Dario Freddi 2011-12-05 08:05:20 UTC
Comments addressed and added unit tests in my repo, branch available-message-types
Comment 5 Dario Freddi 2011-12-05 08:10:47 UTC
Added branch to the URL
Comment 6 Andre Moreira Magalhaes 2011-12-05 09:06:18 UTC
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.
Comment 7 Dario Freddi 2011-12-05 09:24:15 UTC
All fixed, f'pushed to available-message-types rebased on new master and available-message-types-0.8 rebased on 0.8
Comment 8 Andre Moreira Magalhaes 2011-12-05 09:34:31 UTC
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.
Comment 9 Dario Freddi 2011-12-05 09:37:15 UTC
After Andre's ++ on IRC, this change has been merged in master, with merge
commit id . Closing the bug
Comment 10 Dario Freddi 2011-12-05 09:38:29 UTC
Sorry, didn't paste the commit id, which was 60702e3a2c74c9595ce34a4e74ccb0d040962ea2

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.