incubator-wave-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Hearnden (JIRA)" <>
Subject [jira] [Commented] (WAVE-263) ConversationThread.isInline is still used in some parts of the codebase
Date Thu, 05 May 2011 00:37:03 GMT


David Hearnden commented on WAVE-263:

I don't think it's too difficult to do it all in one go, but 3) sounds like the right choice
if this is to be done in two steps.

Most call sites use getAllReplyThreads(), already, which helps.  There are only two things
that use the other methods: the robot API, and the client's deprecated ConversationRenderer.
 The latter is distinguishes inline and non-inline in order to put private conversations between
them.  That can safely be changed to having the private conversations follow all replies,
thus removing the distinction.

That then leaves only the robot API, it looks like there's only three places, two of which
just iterates over inline reply threads followed by iterating over reply threads (so should
be replaced with all reply threads), leaving only one real use case over the whole codebase
for getInlineReplyThreads() (in EventDataConverterV22*). The only thing that is different
from getAllReplyThreads is that that method needs/wants to know the inline location of each

So I suggest the following plan moving forward.

*) Make the above changes, removing all calls to getReplyThreads, and leaving only one call
to getInlineReplyThreads; then 
*) Rename getInlineReplyThreads to locateReplyThreads, to indicate that it is not a cheap
method, and have it return a map of thread -> location rather than a list, which is more
appropriate for the one use case that uses that method.

*Note that that robot method has quadratic complexity (O(replies^2)) because it calls getInlineReplyThreads
once for each thread, and getInlineReplyThreads is O(docsize) (which is O(replies)) because
it dynamically crawls the blip for anchor locations every time it is called.

> ConversationThread.isInline is still used in some parts of the codebase
> -----------------------------------------------------------------------
>                 Key: WAVE-263
>                 URL:
>             Project: Wave
>          Issue Type: Bug
>            Reporter: David Hearnden
>            Assignee: Michael MacFadden
>            Priority: Minor
> In Google Wave, there were four types of reply threads:
> inline=false, no content anchor  =&gt; an indented reply thread outside the parent
> inline=false, with content anchor =&gt; corrupt data, but treated anyway as above
> inline=true, no content anchor =&gt; unanchored inline reply, rendered at the end
of the parent blip (inside).
> inline=true, with content anchor =&gt; anchored inline reply, rendered at the content
anchor (inside).
> In Undercurrent / WIAB, this distinction based on the inline attribute in the manifest
is no longer made.  Reply threads are just reply threads.  Any reply thread can have an anchor
in its parent blip's content, and if there is such an anchor, the thread is rendered there,
otherwise it is rendered at the end of the parent blip's content.  The rendering and behaviour
of the two cases is identical (unlike Google Wave).  The only difference is the location where
the thread is rendered.  Essentially, this means that &quot;indented&quot; replies,
as opposed to &quot;inline&quot; replies, are gone.
> All code that is causing divergent behaviour based on ConversationThread.isInline() needs
to be fixed.  In most cases, the expression:
>   ConversationThread.isInline()
> can be replaced with:
>   true
> followed by consequent refactoring and dead-code removal etc.
> ---
> Issue imported from
> Owner:
> Cc:
> Cc: vega113
> Label: Type-Defect
> Label: Priority-Medium
> Stars: 1
> State: open
> Status: New

This message is automatically generated by JIRA.
For more information on JIRA, see:

View raw message