Makes create wave work with undercurrent in box client.

0 views
Skip to first unread message

hear...@google.com

unread,
Oct 27, 2010, 4:46:56 AM10/27/10
to zdw...@google.com, wave-protocol...@googlegroups.com
Reviewers: zdwang,

Message:
Again, ignore the jar update. I won't submit them, but it's too hard to
mask them out from mercurial.

Demo running on my machine, port 9898. New waves are fast!

Please review this at http://codereview.waveprotocol.org/259001

Affected files:
M src/org/waveprotocol/box/webclient/WebClient.gwt.xml
M src/org/waveprotocol/box/webclient/client/RemoteWaveViewService.java
M src/org/waveprotocol/box/webclient/client/StageTwoProvider.java
M src/org/waveprotocol/box/webclient/client/StagesProvider.java
M src/org/waveprotocol/box/webclient/client/WaveView.java
M src/org/waveprotocol/box/webclient/client/WebClient.java
A
src/org/waveprotocol/box/webclient/waveclient/common/ClientIdGenerator.java
M
src/org/waveprotocol/box/webclient/waveclient/common/WebClientBackend.java
M third_party/runtime/wave-libraries/api-src.jar
M third_party/runtime/wave-libraries/api.jar
M third_party/runtime/wave-libraries/common-src.jar
M third_party/runtime/wave-libraries/common.jar
M third_party/runtime/wave-libraries/communication-src.jar
M third_party/runtime/wave-libraries/communication.jar
M third_party/runtime/wave-libraries/concurrency-src.jar
M third_party/runtime/wave-libraries/concurrency.jar
M third_party/runtime/wave-libraries/federation-src.jar
M third_party/runtime/wave-libraries/federation.jar
M third_party/runtime/wave-libraries/media-src.jar
M third_party/runtime/wave-libraries/media.jar
M third_party/runtime/wave-libraries/proto-src.jar
M third_party/runtime/wave-libraries/proto.jar
M third_party/runtime/wave-libraries/testing-src.jar
M third_party/runtime/wave-libraries/testing.jar
M third_party/runtime/wave-libraries/waveserver-src.jar
M third_party/runtime/wave-libraries/waveserver.jar
A war/static/images/unknown.jpg


David Hearnden

unread,
Oct 27, 2010, 4:58:04 AM10/27/10
to zdw...@google.com, wave-protocol...@googlegroups.com
Codereivew is currently down, but there is one bug I've found.
You can open waves, edit waves, create new waves, over and over, but
as soon as you try and edit a wave that you've previously had open in
that session, you get one op through and that's it. I'll investigate
tomorrow.

zdw...@google.com

unread,
Oct 27, 2010, 2:20:51 PM10/27/10
to hear...@google.com, wave-protocol...@googlegroups.com
Please also remove the jars from the patch and use another patch to
update the jars first.


http://codereview.waveprotocol.org/259001/diff/42001/45001
File
src/org/waveprotocol/box/webclient/client/RemoteWaveViewService.java
(right):

http://codereview.waveprotocol.org/259001/diff/42001/45001#newcode228
Line 228: // TODO: remove after bug 124 is addressed.
please leave a url to the bug.

http://codereview.waveprotocol.org/259001/diff/42001/45001#newcode258
Line 258: // TODO: Remove after bug 125 is fixed.
ditto

http://codereview.waveprotocol.org/259001/diff/42001/47001
File src/org/waveprotocol/box/webclient/client/StageTwoProvider.java
(right):

http://codereview.waveprotocol.org/259001/diff/42001/47001#newcode180
Line 180: // wave panels causes some bugs.
This sounds like a hack. Please add note to investigate further in the
future.

http://codereview.waveprotocol.org/259001/diff/42001/47001#newcode198
Line 198: // Use deferred command to work around bug 126.
please leave url to bug.

http://codereview.waveprotocol.org/259001/diff/42001/48001
File src/org/waveprotocol/box/webclient/client/StagesProvider.java
(right):

http://codereview.waveprotocol.org/259001/diff/42001/48001#newcode116
Line 116: blipQueue.flush();
please add a comment on why flush is necessary. e.g. ensure that all
blips are rendered synchronously before editing.

http://codereview.waveprotocol.org/259001/diff/42001/51001
File src/org/waveprotocol/box/webclient/client/WebClient.java (right):

http://codereview.waveprotocol.org/259001/diff/42001/51001#newcode161
Line 161: History.newItem(newWaveId.serialise(), false);
This is only required after line 167 as 169 will have a history item
installed.

http://codereview.waveprotocol.org/259001/diff/42001/47002
File
src/org/waveprotocol/box/webclient/waveclient/common/WebClientBackend.java
(left):

http://codereview.waveprotocol.org/259001/diff/42001/47002#oldcode105
Line 105: this.idGenerator = new
IdGeneratorImpl(this.userId.getDomain(), new IdGeneratorImpl.Seed() {
I think you forgot to add IdGeneratorImpl to your cl.

http://codereview.waveprotocol.org/259001

hear...@google.com

unread,
Oct 27, 2010, 11:03:11 PM10/27/10
to zdw...@google.com, wave-protocol...@googlegroups.com
Yep, the jar diffs will disappear once I do another push that includes
the undercurrent cc-stack changes. I'll do that before submitting this.


http://codereview.waveprotocol.org/259001/diff/42001/45001
File
src/org/waveprotocol/box/webclient/client/RemoteWaveViewService.java
(right):

http://codereview.waveprotocol.org/259001/diff/42001/45001#newcode228
Line 228: // TODO: remove after bug 124 is addressed.

On 2010/10/27 18:20:51, zdwang wrote:
> please leave a url to the bug.

Done.

http://codereview.waveprotocol.org/259001/diff/42001/45001#newcode258
Line 258: // TODO: Remove after bug 125 is fixed.

On 2010/10/27 18:20:51, zdwang wrote:
> ditto

Done.

http://codereview.waveprotocol.org/259001/diff/42001/47001
File src/org/waveprotocol/box/webclient/client/StageTwoProvider.java
(right):

On 2010/10/27 18:20:51, zdwang wrote:
> This sounds like a hack. Please add note to investigate further in the
future.

Hack removed, problem solved. The problem was that showing a new wave
was not clearing the old wave first (contentPanel.clear()), so there
were id collections in the DOM for wave-level elements (like the frame,
toolbar, etc).

http://codereview.waveprotocol.org/259001/diff/42001/47001#newcode198
Line 198: // Use deferred command to work around bug 126.

On 2010/10/27 18:20:51, zdwang wrote:
> please leave url to bug.

Done.

http://codereview.waveprotocol.org/259001/diff/42001/48001
File src/org/waveprotocol/box/webclient/client/StagesProvider.java
(right):

On 2010/10/27 18:20:51, zdwang wrote:
> please add a comment on why flush is necessary. e.g. ensure that all
blips are
> rendered synchronously before editing.

Done.

http://codereview.waveprotocol.org/259001/diff/42001/51001
File src/org/waveprotocol/box/webclient/client/WebClient.java (right):

http://codereview.waveprotocol.org/259001/diff/42001/51001#newcode161
Line 161: History.newItem(newWaveId.serialise(), false);

On 2010/10/27 18:20:51, zdwang wrote:
> This is only required after line 167 as 169 will have a history item
installed.

I see. I've added an openWave() method that uses the undercurrent flow,
and fixed up the history stuff as follows. Rather than fire the history
token from multiple places, some of which are shared by undercurrent and
the old wave panel, some of which not, I've just gone for the direct
apporach:

A history token is passively added (i.e., newItem(x, false)) when, and
only when, a wave is displayed (as opposed to selected). With
undercurrent, this is done in WebClient.openWave(). With old panel,
this is done in WaveView.setWave(). The two methods are mutually
excluded by the enableWavePanelHarness flag.

That way, we don't need to have separate firing for creating vs opening
waves.

http://codereview.waveprotocol.org/259001/diff/42001/47002
File
src/org/waveprotocol/box/webclient/waveclient/common/WebClientBackend.java
(left):

http://codereview.waveprotocol.org/259001/diff/42001/47002#oldcode105
Line 105: this.idGenerator = new
IdGeneratorImpl(this.userId.getDomain(), new IdGeneratorImpl.Seed() {

On 2010/10/27 18:20:51, zdwang wrote:
> I think you forgot to add IdGeneratorImpl to your cl.

IdGeneratorImpl is in model, and is included in wave-protocol-libraries.
ClientIdGenerator is in this patch, and it uses the
walkabout/undercurrent method for id generator (using the session id
seed rather than system time). I removed the idgenerator from this
class, because it was unused. It was only put in here to be given to
something else via getIdGenerator() below.

http://codereview.waveprotocol.org/259001

zdw...@google.com

unread,
Oct 28, 2010, 1:51:48 AM10/28/10
to hear...@google.com, wave-protocol...@googlegroups.com
LGTM. Please remove the jar etc, before submitting.


http://codereview.waveprotocol.org/259001/diff/42001/51001
File src/org/waveprotocol/box/webclient/client/WebClient.java (right):

http://codereview.waveprotocol.org/259001/diff/42001/51001#newcode161
Line 161: History.newItem(newWaveId.serialise(), false);

Nice.

Reply all
Reply to author
Forward
0 new messages