Hi Michael, I just send you a separate email with the blip counting code that was used in the prototype version of this feature in Google Wave.
On Thu, Nov 25, 2010 at 5:09 PM, Michael MacFadden <[email protected]> wrote: > I have made significant progress on the thread toggle indicator. I > have it working more or less in a prototypical form. I am now trying > to clean it up a bit. I have a few questions. I apologize ahead of > time for the verbosity: > > 1) > > What is the difference between the InlineThreadViewBuilder and the > InlineConversationViewBuilder. Are both of these concepts still in > use? I have been focusing on updating the behavior for the Inline > Thread constructs. At this point I haven’t seen an Inline > Conversation being rendered anywhere. If the Inline Conversation > isn’t being used, or it the behavior of the toggle is different for > them, it will affect where I finish off this implementation. The InlineConversation is for rendering other conversations anchored at a blip, which is how private replies are rendered (i.e., the object that is rendered there is a Conversation rather than a ConversationThread). The WIAB client doesn't have any mechanism for creating private replies at this stage, but when Undercurrent was running in the Google Wave setup (briefly), private replies were available, so that code is somewhat reliable and working. I started a patch to enable creating private replies in Undercurrent (through a menu dropdown), but then I thought that might be better suited as a starter project to onboard a new developer. In theory, plugging them in should Just Work. > 2) > > Calculating the total blips and/or unread blips involves quite a bit > of tree traversal. Since what we want to display is the total number > of blips in the subtree, a recursive walking of the tree structure > below a thread is required to compute the sum total. As you can > imagine, the simplest recursion approach (which I have implemented for > the POC) simply does this for every node in the tree. Basically, the > various subtrees get visited multiple times > > The same recursive walking needs to happen when add / delete blips > occur in the live rendering, except with the added bonus of having to > recalulate the totals upwards towards the root of the tree. This just > adds to the sub tree traversal. Basically the current approach I have > taken is not efficient at all. > > It seems like the model itself could keep track of its cardinality and > update it when blips are added and removed. The model could percolate > the cardinality change upward to the parent as well. This would > increase the performance of this bit. This becomes even more > frustrating when. For an initial prototype, don't worry about making it efficient. Getting the whole feature done in one patch is too much code. Better to do it in steps: the UI and control flow with simple counting, then optimize the counting logic in subsequent patches. Ideally, you could put the use of blip counts behind a ClientFlag that was off by default, so that people can opt-in to use the feature while it's still inefficient or in development. That said, I think that waves in WIAB are going to be small enough for a while that inefficient counting won't be a problem. > I believe that it was recommended that the counts of blips and / or > unread blips be stored as DOM attributes accessible by the view. From > here you could take a more pragmatic increment / decrement strategy in > the live renderer. However, I would ask the question why add this > state information at the view level and not at the model level. I > think it would be appropriate to ask the model “how many blips > (recursively) do you contain. It would be up to the model to answer > that question in an efficient way. Sometimes it's not clear whether a feature is really part of the model or part of the presentation logic. Something we experimented with was writing a 'presentation model' that bound together the conversation model and the supplement model into a single model, where conversation state and per-user state were exposed in a single place. e.g., blip.getReplyThreads() and blip.isUnread() on the same object. We didn't end up using that model because it introduced performance regressions that we never tracked down, and not everyone was happy with yet another abstraction layer and set of wrapper objects, so there was not much motivation to fix it. However, I do think that approach is the best answer for something like blip counts. Let the conversation model expose the core state and operations of conversation structure, and other derivable things (like blip counts, blips written by X, participants who have contributed, blip snippet summaries, etc) are provided by something else so that the core model isn't burdened with potential inefficiencies with providing fancy behaviour. But it's all shades of gray. That was one of the reasons for the WaveViewManager concept (I briefly described it in that other email I sent you): it added on all the webclient-specific state and behaviour associated with waves in addition to what is provided by the core models. > It does get a little more complicated for getting a tally of unread > blips since this information is not in the model directly, but in the > supplemented model. I suppose the supplemented model could keep track > of this state in much the same way as I am suggesting for the core > model. Exactly. Although total counts could be argued as something that the model should provide (even though it's already calculable and maintainable from state + events it already exposes), total/unread counts is less clear, because the unread information lives elsewhere. Also, the ability to initialize that state from external information (like server-supplied state), rather than re-computing it from scratch, is a flow that is fairly specific to the web client architecture, and is unlikely to be used by anything else that uses conversations (robots, indexing, message gateways, etc). So until more use cases arise, I think the pragmatic option is to put the responsibility on the presentation/UI code to manage that information. > Thoughts on this would be much appreciated. > > 3) > > Currently an ObservalbeSupplementedWave is passed to the liver > rendering stack in the StageTwo class, but it is not passed to the > FullDomRenderer. I believe the FullDomRenderer will need this object > if it is to calculate the number of unread blips on initial > rendering. Just looking to verify this. If this is the case it is > fairly straight forward to add it to the create method in the > FullDomWaveRendererImpl and pass it through. Yes, both the static and the dynamic renderers will need to be provided with a supplement model. The Stage Two flow is to instantiate the models (conversation, supplement, ...) and then the renderers (static and live), so that should fit in fine. -Dave -- You received this message because you are subscribed to the Google Groups "Wave Protocol" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/wave-protocol?hl=en.
