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 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.
