From 1d80ac83ffd119d7371e0a2fe45aecdb94b308b8 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Fri, 27 Apr 2012 17:54:03 -0700 Subject: [PATCH] Use an EventBus to manage star icons Image widgets that need to track the starred state of a change register themselves on the EventBus when the widget is attached to the DOM, and automatically unregister when the widget is removed from the DOM. This ensures there are no memory leaks in the browser. Events can be fired on the event bus to update UI widgets with the new status of the star, enabling the UI to immediately reflect what the user has done. Toggle star RPCs are batched and sent in the background, with at most one star RPC pending at any one time. If the user rapidly clicks a lot of search results to be starred, these will batch and send as soon as the first star action is updated. On slower networks this will run more quickly than starring each change individually. In the common case of starring a single change, the batching has no additional overhead If any RPC fails the pending stars are undone in the UI and an error is shown to the user. Any outstanding batch is discarded. Change-Id: I7ea46d2e2ee5c64c0d807677859cfb7d90b8966a --- .../gerrit/client/changes/ChangeCache.java | 8 - .../client/changes/ChangeDetailCache.java | 1 + .../gerrit/client/changes/ChangeScreen.java | 17 +- .../gerrit/client/changes/ChangeTable.java | 4 +- .../gerrit/client/changes/StarCache.java | 139 ----------- .../gerrit/client/changes/StarredChanges.java | 216 ++++++++++++++++++ 6 files changed, 225 insertions(+), 160 deletions(-) delete mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarCache.java create mode 100644 gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarredChanges.java diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeCache.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeCache.java index 27f76f6e83..5a9da1a51d 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeCache.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeCache.java @@ -38,7 +38,6 @@ public class ChangeCache { private Change.Id changeId; private ChangeDetailCache detail; private ListenableValue info; - private StarCache starred; protected ChangeCache(Change.Id chg) { changeId = chg; @@ -61,11 +60,4 @@ public class ChangeCache { } return info; } - - public StarCache getStarCache() { - if (starred == null) { - starred = new StarCache(changeId); - } - return starred; - } } diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDetailCache.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDetailCache.java index bb28e11639..9c19d50ed3 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDetailCache.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeDetailCache.java @@ -65,6 +65,7 @@ public class ChangeDetailCache extends ListenableValue { public static void setChangeDetail(ChangeDetail detail) { Change.Id chgId = detail.getChange().getId(); ChangeCache.get(chgId).getChangeDetailCache().set(detail); + StarredChanges.fireChangeStarEvent(chgId, detail.isStarred()); } private final Change.Id changeId; diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java index b054175bee..15c1150d4c 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java @@ -28,9 +28,9 @@ import com.google.gerrit.common.data.ChangeDetail; import com.google.gerrit.common.data.ChangeInfo; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gwt.event.dom.client.ChangeEvent; import com.google.gwt.event.dom.client.ChangeHandler; import com.google.gwt.event.dom.client.KeyPressEvent; @@ -42,7 +42,6 @@ import com.google.gwt.user.client.ui.DisclosurePanel; import com.google.gwt.user.client.ui.FlowPanel; import com.google.gwt.user.client.ui.Grid; import com.google.gwt.user.client.ui.HorizontalPanel; -import com.google.gwt.user.client.ui.Image; import com.google.gwt.user.client.ui.InlineLabel; import com.google.gwt.user.client.ui.Label; import com.google.gwt.user.client.ui.ListBox; @@ -60,9 +59,7 @@ public class ChangeScreen extends Screen private final Change.Id changeId; private final PatchSet.Id openPatchSetId; private ChangeDetailCache detailCache; - private StarCache starred; - private Image starChange; private ChangeDescriptionBlock descriptionBlock; private ApprovalTable approvals; @@ -155,8 +152,6 @@ public class ChangeScreen extends Screen detailCache = cache.getChangeDetailCache(); detailCache.addValueChangeHandler(this); - starred = cache.getStarCache(); - addStyleName(Gerrit.RESOURCES.css().changeScreen()); keysNavigation = new KeyCommandSet(Gerrit.C.sectionNavigation()); @@ -165,13 +160,13 @@ public class ChangeScreen extends Screen keysNavigation.add(new ExpandCollapseDependencySectionKeyCommand(0, 'd', Util.C.expandCollapseDependencies())); if (Gerrit.isSignedIn()) { - keysAction.add(starred.new KeyCommand(0, 's', Util.C.changeTableStar())); + StarredChanges.Icon star = StarredChanges.createIcon(changeId, false); + star.setStyleName(Gerrit.RESOURCES.css().changeScreenStarIcon()); + setTitleWest(star); + + keysAction.add(StarredChanges.newKeyCommand(star)); keysAction.add(new PublishCommentsKeyCommand(0, 'r', Util.C .keyPublishComments())); - - starChange = starred.createStar(); - starChange.setStyleName(Gerrit.RESOURCES.css().changeScreenStarIcon()); - setTitleWest(starChange); } descriptionBlock = new ChangeDescriptionBlock(); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java index 5a568f1629..08fc7bdf6b 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeTable.java @@ -145,7 +145,7 @@ public class ChangeTable extends NavigationTable { protected void onStarClick(final int row) { final ChangeInfo c = getRowItem(row); if (c != null && Gerrit.isSignedIn()) { - ChangeCache.get(c.getId()).getStarCache().toggleStar(); + ((StarredChanges.Icon) table.getWidget(row, C_STAR)).toggleStar(); } } @@ -198,7 +198,7 @@ public class ChangeTable extends NavigationTable { final String idstr = c.getKey().abbreviate(); table.setWidget(row, C_ARROW, null); if (Gerrit.isSignedIn()) { - table.setWidget(row, C_STAR, cache.getStarCache().createStar()); + table.setWidget(row, C_STAR, StarredChanges.createIcon(c.getId(), c.isStarred())); } table.setWidget(row, C_ID, new TableChangeLink(idstr, c)); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarCache.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarCache.java deleted file mode 100644 index d7624a6fd8..0000000000 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarCache.java +++ /dev/null @@ -1,139 +0,0 @@ -// Copyright (C) 2012 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package com.google.gerrit.client.changes; - -import com.google.gerrit.client.Gerrit; -import com.google.gerrit.client.rpc.GerritCallback; -import com.google.gerrit.client.ui.NeedsSignInKeyCommand; -import com.google.gerrit.common.data.ChangeDetail; -import com.google.gerrit.common.data.ChangeInfo; -import com.google.gerrit.common.data.ToggleStarRequest; -import com.google.gerrit.reviewdb.client.Change; - -import com.google.gwt.event.dom.client.ClickEvent; -import com.google.gwt.event.dom.client.ClickHandler; -import com.google.gwt.event.dom.client.KeyPressEvent; -import com.google.gwt.event.logical.shared.HasValueChangeHandlers; -import com.google.gwt.event.logical.shared.ValueChangeEvent; -import com.google.gwt.event.logical.shared.ValueChangeHandler; -import com.google.gwt.event.shared.GwtEvent; -import com.google.gwt.event.shared.HandlerManager; -import com.google.gwt.event.shared.HandlerRegistration; -import com.google.gwt.resources.client.ImageResource; -import com.google.gwt.user.client.ui.Image; -import com.google.gwtjsonrpc.common.VoidResult; - -public class StarCache implements HasValueChangeHandlers { - public class KeyCommand extends NeedsSignInKeyCommand { - public KeyCommand(int mask, char key, String help) { - super(mask, key, help); - } - - @Override - public void onKeyPress(final KeyPressEvent event) { - StarCache.this.toggleStar(); - } - } - - ChangeCache cache; - - private HandlerManager manager = new HandlerManager(this); - - public StarCache(final Change.Id chg) { - cache = ChangeCache.get(chg); - } - - public boolean get() { - ChangeDetail detail = cache.getChangeDetailCache().get(); - if (detail != null) { - return detail.isStarred(); - } - ChangeInfo info = cache.getChangeInfoCache().get(); - if (info != null) { - return info.isStarred(); - } - return false; - } - - public void set(final boolean s) { - if (Gerrit.isSignedIn() && s != get()) { - final ToggleStarRequest req = new ToggleStarRequest(); - req.toggle(cache.getChangeId(), s); - - Util.LIST_SVC.toggleStars(req, new GerritCallback() { - public void onSuccess(final VoidResult result) { - setStarred(s); - fireEvent(new ValueChangeEvent(s){}); - } - }); - } - } - - private void setStarred(final boolean s) { - ChangeDetail detail = cache.getChangeDetailCache().get(); - if (detail != null) { - detail.setStarred(s); - } - ChangeInfo info = cache.getChangeInfoCache().get(); - if (info != null) { - info.setStarred(s); - } - } - - public void toggleStar() { - set(!get()); - } - - @SuppressWarnings("unchecked") - public Image createStar() { - final Image star = new Image(getResource()); - star.setVisible(Gerrit.isSignedIn()); - - star.addClickHandler(new ClickHandler() { - @Override - public void onClick(final ClickEvent event) { - StarCache.this.toggleStar(); - } - }); - - @SuppressWarnings("rawtypes") - ValueChangeHandler starUpdater = new ValueChangeHandler() { - @Override - public void onValueChange(ValueChangeEvent event) { - star.setResource(StarCache.this.getResource()); - } - }; - - cache.getChangeDetailCache().addValueChangeHandler(starUpdater); - cache.getChangeInfoCache().addValueChangeHandler(starUpdater); - - this.addValueChangeHandler(starUpdater); - - return star; - } - - private ImageResource getResource() { - return get() ? Gerrit.RESOURCES.starFilled() : Gerrit.RESOURCES.starOpen(); - } - - public void fireEvent(GwtEvent event) { - manager.fireEvent(event); - } - - public HandlerRegistration addValueChangeHandler( - ValueChangeHandler handler) { - return manager.addHandler(ValueChangeEvent.getType(), handler); - } -} diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarredChanges.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarredChanges.java new file mode 100644 index 0000000000..8b5aa1cd4a --- /dev/null +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/StarredChanges.java @@ -0,0 +1,216 @@ +// Copyright (C) 2012 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.client.changes; + +import com.google.gerrit.client.Gerrit; +import com.google.gerrit.client.rpc.GerritCallback; +import com.google.gerrit.common.data.ToggleStarRequest; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gwt.event.dom.client.ClickEvent; +import com.google.gwt.event.dom.client.ClickHandler; +import com.google.gwt.event.dom.client.KeyPressEvent; +import com.google.gwt.event.shared.EventBus; +import com.google.gwt.event.shared.SimpleEventBus; +import com.google.gwt.resources.client.ImageResource; +import com.google.gwt.user.client.ui.Image; +import com.google.gwtexpui.globalkey.client.KeyCommand; +import com.google.gwtjsonrpc.common.VoidResult; +import com.google.web.bindery.event.shared.Event; +import com.google.web.bindery.event.shared.HandlerRegistration; + +/** Supports the star icon displayed on changes and tracking the status. */ +public class StarredChanges { + private static final EventBus eventBus = new SimpleEventBus(); + private static final Event.Type TYPE = + new Event.Type(); + + /** Handler that can receive notifications of a change's starred status. */ + public static interface ChangeStarHandler { + public void onChangeStar(ChangeStarEvent event); + } + + /** Event fired when a star changes status. The new status is reported. */ + public static class ChangeStarEvent extends Event { + private boolean starred; + + public ChangeStarEvent(Change.Id source, boolean starred) { + setSource(source); + this.starred = starred; + } + + public boolean isStarred() { + return starred; + } + + @Override + public Type getAssociatedType() { + return TYPE; + } + + @Override + protected void dispatch(ChangeStarHandler handler) { + handler.onChangeStar(this); + } + } + + /** + * Create a star icon for the given change, and current status. Returns null + * if the user is not signed in and cannot support starred changes. + */ + public static Icon createIcon(Change.Id source, boolean starred) { + return Gerrit.isSignedIn() ? new Icon(source, starred) : null; + } + + /** Make a key command that toggles the star for a change. */ + public static KeyCommand newKeyCommand(final Icon icon) { + return new KeyCommand(0, 's', Util.C.changeTableStar()) { + @Override + public void onKeyPress(KeyPressEvent event) { + icon.toggleStar(); + } + }; + } + + /** Add a handler to listen for starred status to change. */ + public static HandlerRegistration addHandler( + Change.Id source, + ChangeStarHandler handler) { + return eventBus.addHandlerToSource(TYPE, source, handler); + } + + /** + * Broadcast the current starred value of a change to UI widgets. This does + * not RPC to the server and does not alter the starred status of a change. + */ + public static void fireChangeStarEvent(Change.Id id, boolean starred) { + eventBus.fireEventFromSource( + new ChangeStarEvent(id, starred), + id); + } + + /** + * Set the starred status of a change. This method broadcasts to all + * interested UI widgets and sends an RPC to the server to record the + * updated status. + */ + public static void toggleStar( + final Change.Id changeId, + final boolean newValue) { + if (next == null) { + next = new ToggleStarRequest(); + } + next.toggle(changeId, newValue); + fireChangeStarEvent(changeId, newValue); + if (!busy) { + start(); + } + } + + private static ToggleStarRequest next; + private static boolean busy; + + private static void start() { + final ToggleStarRequest req = next; + next = null; + busy = true; + + Util.LIST_SVC.toggleStars(req, new GerritCallback() { + @Override + public void onSuccess(VoidResult result) { + if (next != null) { + start(); + } else { + busy = false; + } + } + + @Override + public void onFailure(Throwable caught) { + rollback(req); + if (next != null) { + rollback(next); + next = null; + } + busy = false; + super.onFailure(caught); + } + }); + } + + private static void rollback(ToggleStarRequest req) { + if (req.getAddSet() != null) { + for (Change.Id id : req.getAddSet()) { + fireChangeStarEvent(id, false); + } + } + if (req.getRemoveSet() != null) { + for (Change.Id id : req.getRemoveSet()) { + fireChangeStarEvent(id, true); + } + } + } + + public static class Icon extends Image + implements ChangeStarHandler, ClickHandler { + private final Change.Id changeId; + private boolean starred; + private HandlerRegistration handler; + + Icon(Change.Id changeId, boolean starred) { + super(resource(starred)); + this.changeId = changeId; + this.starred = starred; + addClickHandler(this); + } + + /** + * Toggles the state of the star, as if the user clicked on the image. This + * will broadcast the new star status to all interested UI widgets, and RPC + * to the server to store the changed value. + */ + public void toggleStar() { + StarredChanges.toggleStar(changeId, !starred); + } + + @Override + protected void onLoad() { + handler = StarredChanges.addHandler(changeId, this); + } + + @Override + protected void onUnload() { + handler.removeHandler(); + handler = null; + } + + @Override + public void onChangeStar(ChangeStarEvent event) { + setResource(resource(event.isStarred())); + starred = event.isStarred(); + } + + @Override + public void onClick(ClickEvent event) { + toggleStar(); + } + + private static ImageResource resource(boolean starred) { + return starred ? Gerrit.RESOURCES.starFilled() : Gerrit.RESOURCES.starOpen(); + } + } + + private StarredChanges() { + } +}