[Bug 23274] Implement Hold interface support

[bugzilla-daemon at freedesktop.org]

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


Andre Moreira Magalhaes <andrunko at gmail.com> changed:

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




--- Comment #7 from Andre Moreira Magalhaes <andrunko at gmail.com>  2009-08-18 08:46:41 PST ---
(In reply to comment #6)
> 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.
Done. Completely agree here, I will try to write more useful docs next time. I
am not good at writing docs but I will try my best :)

> (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.
Done. Reverted to requestHold.



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