My initial inclination is to do nothing (option 0) for the moment and tackle it when we turn on the inline conversation functionality I guess the point is that it is just a "coincedence" that the conversation and thread have some of the same interface methods and it is an implementation detail that they delegate to the same class.
If we do nothing and add the functionality to the collapsible builder, then when we turn on the inline conversation we will have to decided to either update it to take advantage of the new functionality, or do some refactoting, but at least that point we will have a solid use case requiring the change rather than a hypothetical So I am leaning towards, option 0 for now, and we can re-evaluate option 1 later on. ~Michael On Nov 25, 8:00 pm, David Hearnden <[email protected]> wrote: > In Google Wave, nested conversations are not collapsible (for no > particular reason), so making them collapsible was an afterthought in > Undercurrent, which is why the collapse concerns are copied between > those two interfaces. > > Yes, inline threads and inline conversations have different > renderings. Inline conversations have a participant panel in them. > > I don't see how Option 2 could work well, since the inline > conversation and inline thread views have different structure. Option > 3 sounds like the collapsible UI layout and logic would end up being > copy & paste across the two implementations. There's also option 0, > which is do nothing, which is taking the position that the View API > doesn't bother with a dedicated collapsible concept. InlineThreads > and InlineConversations both happen to have collapsibility and blip > counts, but that's a coincidence as far as the View API is concerned. > The implementations, however, have the collapsibility logic factored > out into shared classes, which is not uncommon. > > Option 1 sounds good to me too. Maybe something like a decorator: > > /* A collapsible piece of conversation structure. Parameterized by > the view it contains. */ > interface Collapsible<T extends View> extends View { > void setCollapsed(...); > void setCount(...); > void setRead(...); > T getContents(); > } > > /* A sequence of blips */ > interface ThreadView extends View { ... } > > /* A participant panel and a thread. */ > interface ConversationView extends View { ... } > > /* typedef for convenience. */ > interface InlineThreadView extends Collapsible<ThreadView> {} > > /* typedef for convenience. */ > interface InlineConversationView extends > Collapsible<ConversationView> {} > > That way the view structure still has specific types, like: > interface BlipView ... { > InlineThreadView getReplyBefore(InlineThreadView ref); > } > > but the implementation of collapsibility is in one place, the > CollapsibleView{Builder/DomImpl}, to which the > Inline{Thread,Conversation}View implementations can delegate, just > like they do now. > > Alternatively, those convenience types could be dropped, leaving: > > interface BlipView ... { > Collapsible<ThreadView> getReplyBefore(Collapsible<ThreadView> > ref); > } > > but that seems like it would make the view API harder to use and > navigate. > > I'm not sure that it's necessarily the wrong approach to configure the > collapsible builder to have a numeric and non-numeric mode. It may be > useful later to have other parts of the UI that are not blip trees to > be collapsible using the current bubble rendering. But we don't have > any use cases for that, so adding a more to the collapsible types > should be unnecessary right now. > > So in summary, I don't think option 0 is that bad. The API has a > small amount of redundancy (multiple things have collapsibility and > counts exposed), but the implementations have no redundancy. > Being a purist, option 1 sounds like the ideal though, since both the > API and the implementations have no redundancy. But since the > implementations probably wouldn't change, I'm not sure that the > increase in factorization of the API is worth the cost of moving from > option 0 to option 1 while the commonality between inline threads and > inline conversations is fairly small. I certainly don't object to it > though. > > -David > > On Nov 26, 12:32 pm, Michael MacFadden <[email protected]> > wrote: > > > > > > > > > In regards to the inline conversation construct, it does seem like > > there is a little bit of overlap between the handling of inline > > conversations and inline threads. If you look at the two intrinsic > > interfaces: > > > public interface IntrinsicInlineThreadView extends IntrinsicThreadView > > { > > void setCollapsed(boolean collapsed); > > boolean isCollapsed(); > > > } > > > public interface IntrinsicInlineConversationView { > > void setCollapsed(boolean collapsed); > > boolean isCollapsed(); > > > } > > > Furthermore, both ultimately just delegate to the CollapsibleBuilder > > class. Right now the place to modify the toggle would be in the > > CollapsibleBuilder. For this I will need to add a method such as > > this: > > > public void setTotalBlipCount(int totalBlipCount) > > > Of course this means I will need to also add this to the > > IntrinsicInlineThreadView interface so that this class can delegate to > > the collapsible builder. The interesting thing is that then the > > collapsible builder is going to render this new style indicator, but > > the IntrinsicInlineConversationView interface doesn't have the > > required method to allow a consumer to set the total blip count. > > Basically we have this weird interface association due to the > > delegation of the implementation to the same class. > > > In other words, when I add a method to one interface relative to a new > > capability in the delegated class (CollapsibleBuider) I almost HAVE to > > add it to the other interface. From this perspective it seems like > > the abstractions aren't quite right. I see a few options: > > > 1) > > The inline thread and inline conversation interfaces need another > > super interface that they both implement. > > > 2) > > We collapse these into one interface with multiple implementations. > > > 3) > > Separate the rendering of these into two separate classes. > > > Are the inline conversation and inline thread rendered differently? > > Do they have different capabilities, etc. For the moment we can > > ignore it, but if we turn on the inline conversation feature, then you > > would immediately notice that the indicator for the toggle is always > > going to say 0. > > > As a side note, I think it would be the wrong approach to add some > > sort of configuration flag to the CollapsibleBuilder to tell it > > whether to show the count or not. > > > Thoughts? > > > On Nov 25, 6:33 pm, David Hearnden <[email protected]> wrote: > > > > 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 > > ... > > read more » -- 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.
