> On 2012-01-16 21:39:13, Yuri Zelikov wrote: > > Hmm, I think I already implemented something like this for waveinabox.net > > as experimental feature. But then I removed all experimental features. And > > I didn't bother to submit it later... Too bad. Anyway, can you try my > > implementation and let me know which one you prefer? > > https://github.com/vega113/WaveInCloud/commit/6ff2f3b2c0b615cb70fe4fbdbde5022616b79407 > > Ali Lown wrote: > Yours is definitely the cleaner approach in this situation (I hadn't > considered handling the problem like that). > > Push yours instead and close this review. > > Note however for your code: I feel the default title is more confusing > than simply leaving it blank, and the lower limit of 3 on length before > reverting to the default seems pretty arbitrary? > > Ali Lown wrote: > PS. Any other features I am going to attempt to implement which you have > sitting in your repository not push upstream?
I ll make a patch from it and put it for review. I think there two more features that were implemented for waveinabox.net but not in WIAB: 1. A prettier microphone image for the main page. 2. Pure HTML rendering of waves on the server side - for example http://waveinabox.net/render/wave/waveinabox.net/w+JdoVYWRgpuE . But it requires a serious refactoring of the current code to decouple the GWT part form the logic of rendering. I suggested a patch with such refactoring but it wasn't accepted for reasons concerning efficiency and just because it was too huge. But since then Pat Coleman submitted a similar patch (http://codereview.waveprotocol.org/617001/show) that at least partially decouple the tight coupling between GWT and rendering logic, so I guess it worth to make another try. I didn't implement this feature in waveinabox.net cleanly - it would make it imcompatible with WIAB, so I just copied all the wave rendering classes and modified them accordingly. Anyway, if you want to pick on this - let me know :). - Yuri ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3512/#review4410 ----------------------------------------------------------- On 2012-01-16 21:23:00, Ali Lown wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3512/ > ----------------------------------------------------------- > > (Updated 2012-01-16 21:23:00) > > > Review request for wave, Yuri Zelikov and Lennard de Rijk. > > > Summary > ------- > > For waves with a title determined by the existing WaveTitleHandler code, this > renders it in the page's html title tag and in the blue space at the top of > the wave frame. > > Note: I went for extending the WaveSelectionEvent to pass a title along with > it, rather than trying to find the wave from the WaveRef, opening it and then > getting the title, because in future implementations of the backend logic > this is likely to force an IO operation (which is pointless since the callers > to WaveSelectionEvent seem to have the title readily available anyway). > > > Diffs > ----- > > /src/org/waveprotocol/box/webclient/client/HistoryChangeListener.java > 1230205 > /src/org/waveprotocol/box/webclient/client/WebClient.java 1230205 > /src/org/waveprotocol/box/webclient/client/events/WaveSelectionEvent.java > 1230205 > > /src/org/waveprotocol/box/webclient/client/events/WaveSelectionEventHandler.java > 1230205 > /src/org/waveprotocol/box/webclient/search/SearchPresenter.java 1230205 > > Diff: https://reviews.apache.org/r/3512/diff > > > Testing > ------- > > Works on my machine. :) > > > Thanks, > > Ali > >
