[Bug 23274] Implement Hold interface support

[bugzilla-daemon at freedesktop.org]

http://bugs.freedesktop.org/show_bug.cgi?id=23274


Simon McVittie <simon.mcvittie at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|patch                       |




--- Comment #6 from Simon McVittie <simon.mcvittie at collabora.co.uk>  2009-08-18 08:32:21 PST ---
Code looks fine. One documentation adjustment needed, and optionally one API
tweak:

I think it would be useful to change the docstring for localHoldState like
this:

>- * Return the channel local hold state.
>+ * Return whether the local user has placed this call on hold.
>  *
>  * This method requires StreamedMediaChannel::FeatureHoldState to be enabled.
>  *
>  * \return The channel local hold state.
>  * \sa requestLocalHold()
>  */
> LocalHoldState StreamedMediaChannel::holdState() const
> LocalHoldState StreamedMediaChannel::localHoldState() const

Similarly, localHoldStateReason could say "the reason why localHoldState()
changed to its current value", or something, as its one-liner.

Otherwise, the docstring is no more helpful than the method name! I know it's
very easy to document methods like "ExampleWindow::accountCreationWidget():
return the account creation widget", but that sort of thing looks rather as
though the author has grudgingly documented things in order to comply with
mandatory coding standards, rather than thinking about how to make the docs
useful (for some reason, in my mind this conjures up images of javadoc, but I'm
sure there are similar offenders in every language).

Even in cases where the method's functionality is entirely obvious, you can
often make the docs seem less mechanically-generated by turning it into a
phrase like "return the window's account creation widget", or better, "return a
widget that can be used to create accounts". Bonus points if your text can
suggest concisely why the library user might want to call this function.

(In reply to comment #3)
> The D-Bus API says GetHoldState, but yeah, I changed it to localHoldState and
> localHoldStateReason. I also changed the Feature to FeatureLocalHoldState and
> the request method to requestLocalHold.

I think FeatureLocalHoldState is a good change too, yes.

I'm not so sure about requestLocalHold - what other hold state could we
possibly be requesting? "Local" is somewhat redundant here, since whether the
other guy has put the call on hold is clearly not our decision - all we can
possibly hope to change by our request is whether *we* have the call on hold.

Keep it as-is if you value consistency over conciseness, though.

Please reinstate the patch keyword when this is ready for review again.


-- 
Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.