> On July 17, 2013, 5:22 p.m., Yuri Zelikov wrote: > > I think Kune use alternative implementation of History and that's the > > reason. > > Ali Lown wrote: > @Vicente: Can you confirm is this is indeed the reason. (And so, whether > this patch would cause problems?) > > Vicente J. Ruiz Jurado wrote: > These changes of History come from this review: > https://reviews.apache.org/r/2913/ > that allow to intercept the history changes in applications that > integrates wave client (like kune) and allow make other uses of history with > different hashtags (not only waverefs). > > I think that this is needed to mantain to allow wave client third party > integrations. > > I'll propose similar changes in other parts of the Webclient (for > instance to allow alternatives to Window class, alerts/prompts, etc).
Hmm. I can understand making it easier for other applications, and am happy to support a single-level of indirection here for that purpose. (The HistorySupport class, which can be changed as third-party implementations wish). I don't feel that the three-level of indirection system used here (HistorySupport -> HistoryProvider -> HistoryProviderDefault), which whilst might make it less effort for third-parties to add their changes before shipping (why is this a concern to us here?), is worth the cognitive effort of remembering about these 3 distinct concepts. I would not be a fan of this pattern being added elsewhere to the codebase. [This code-base is meant to be a reference implementation of Wave in a system, for third-parties to use to build their additions to. It is much easier for a new third-party to develop against a small code-base they can quickly understand.] [If some third-party has such an improved history system, why are they not sending it back upstream (If they don't want to/can't, I don't see why running rebase is so difficult), rather than changing upstream to make it easy for them? :)] - Ali ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/12682/#review23302 ----------------------------------------------------------- On July 17, 2013, 3:19 p.m., Ali Lown wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/12682/ > ----------------------------------------------------------- > > (Updated July 17, 2013, 3:19 p.m.) > > > Review request for wave, Bruno Gonzalez, Vicente J. Ruiz Jurado, and Yuri > Zelikov. > > > Repository: wave-git > > > Description > ------- > > Whilst perusing the code for other things to delete, I happened to spot these > files. > At which point I wondered why we had three classes doing a single-line > function call (to GWT). > > As such, I compressed it all into the HistorySupport class. (Which still > seems excessive, but I will leave it there since it is still easy to change > the single implementation [presuming that was the reasoning behind using > three classes in the first place]). > > > Diffs > ----- > > src/org/waveprotocol/box/webclient/client/HistoryProvider.java f207b83 > src/org/waveprotocol/box/webclient/client/HistoryProviderDefault.java > a2dae4a > src/org/waveprotocol/box/webclient/client/HistorySupport.java 93fe852 > src/org/waveprotocol/box/webclient/client/WebClient.java d3e3b49 > > Diff: https://reviews.apache.org/r/12682/diff/ > > > Testing > ------- > > Builds and passes test suite. > The composition of all 7 of these 'related' (but independent) patches is > verified to still work as a wave server. > > > Thanks, > > Ali Lown > >
