Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix improperly disposed projects and only respond to selection changes for the project that the selection model belongs to #10

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Fix improperly disposed projects and only respond to selection changes for the project that the selection model belongs to #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f203b54
Dispose of nested presenter
matthewhorridge Jun 3, 2024
e672618
Make ClientProjectComponent implement dispose so that disposal is in …
matthewhorridge Jun 3, 2024
3817438
Make SelectionModel dispose of place changed handlers. Ignore place …
matthewhorridge Jun 3, 2024
9529f43
Make PortletWidgetMapper implement HasDispose
matthewhorridge Jun 3, 2024
1c99414
Dispose of WidgetMapper
matthewhorridge Jun 3, 2024
8c8528e
More logging. Clean up some handler removal.
matthewhorridge Jun 3, 2024
34178e4
Added some logging in the DispatchServiceManager that monitors whethe…
matthewhorridge Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions ...t/src/main/java/edu/stanford/bmir/protege/web/client/dispatch/DispatchServiceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.google.gwt.place.shared.Place;
import com.google.gwt.place.shared.PlaceController;
import com.google.gwt.user.client.Timer;
import com.google.gwt.user.client.Window;
import com.google.gwt.user.client.rpc.AsyncCallback;
import com.google.gwt.user.client.rpc.InvocationException;
import com.google.web.bindery.event.shared.EventBus;
Expand All @@ -31,6 +32,7 @@
import java.util.*;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.logging.Level;
import java.util.logging.Logger;

import static com.google.common.base.Preconditions.checkNotNull;
Expand Down Expand Up @@ -126,6 +128,7 @@ public <A extends Action<R>, R extends Result> void execute(A action, final Disp
callback.handleSubmittedForExecution();
if(action instanceof HasProjectId) {
ProjectId projectId = ((HasProjectId) action).getProjectId();
checkMakingACallForCurrentProject(projectId);
ResultCache resultCache = getResultCache(projectId, eventBus);
Optional<R> result = resultCache.getCachedResult(action);
if (result.isPresent()) {
Expand All @@ -145,6 +148,24 @@ public <A extends Action<R>, R extends Result> void execute(A action, final Disp
// }
}

private void checkMakingACallForCurrentProject(ProjectId projectId) {
Place place = placeController.getWhere();
if(place instanceof HasProjectId) {
ProjectId placeProjectId = ((HasProjectId) place).getProjectId();
if(!placeProjectId.equals(projectId)) {
logCurrentStack();
}
}
}

private static void logCurrentStack() {
try {
throw new RuntimeException("Mismatch");
} catch (RuntimeException e) {
logger.log(Level.WARNING, "Mismatch of project Ids. If not deliberate this may indicated a bug somewhere.", e);
}
}

@SuppressWarnings("unchecked")
private <A extends Action<R>, R extends Result> void execAction(A action, DispatchServiceCallback<R> callback) {
requestCount++;
Expand Down
6 changes: 6 additions & 0 deletions ...rc/main/java/edu/stanford/bmir/protege/web/client/editor/ClassEditorPortletPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,10 @@ public void startPortlet(PortletUi portletUi, WebProtegeEventBus eventBus) {
protected void handleReloadRequest() {
editorPresenter.handleReloadRequest();
}

@Override
public void dispose() {
editorPresenter.dispose();
super.dispose();
}
}
11 changes: 10 additions & 1 deletion ...ent/src/main/java/edu/stanford/bmir/protege/web/client/inject/ClientProjectComponent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import edu.stanford.bmir.protege.web.client.project.ProjectPresenter;
import edu.stanford.bmir.protege.web.client.projectsettings.ProjectSettingsPresenter;
import edu.stanford.bmir.protege.web.client.search.EntitySearchSettingsPresenter;
import edu.stanford.bmir.protege.web.client.selection.SelectionModel;
import edu.stanford.bmir.protege.web.client.sharing.SharingSettingsPresenter;
import edu.stanford.bmir.protege.web.client.tag.ProjectTagsPresenter;
import edu.stanford.bmir.protege.web.shared.HasDispose;
import edu.stanford.bmir.protege.web.shared.inject.ProjectSingleton;
import edu.stanford.bmir.protege.web.shared.project.ProjectId;

Expand All @@ -28,7 +30,7 @@
}
)
@ProjectSingleton
public interface ClientProjectComponent {
public interface ClientProjectComponent extends HasDispose {

ProjectId getProjectId();

Expand All @@ -51,4 +53,11 @@ public interface ClientProjectComponent {
EntitySearchSettingsPresenter getEntitySearchSettingsPresenter();

PerspectivesManagerPresenter getPerspectivesManagerPresenter();

SelectionModel getProjectSelectionModel();

default void dispose() {
getProjectPresenter().dispose();
getProjectSelectionModel().dispose();
}
}
1 change: 1 addition & 0 deletions ...lient/src/main/java/edu/stanford/bmir/protege/web/client/perspective/PerspectiveImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ public void onResize() {

@Override
public void dispose() {
widgetMapper.dispose();
}


Expand Down
10 changes: 8 additions & 2 deletions .../src/main/java/edu/stanford/bmir/protege/web/client/perspective/PerspectivePresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.google.gwt.user.client.ui.AcceptsOneWidget;
import com.google.gwt.user.client.ui.Label;
import com.google.web.bindery.event.shared.EventBus;
import com.google.web.bindery.event.shared.HandlerRegistration;
import edu.stanford.bmir.protege.web.client.dispatch.DispatchServiceManager;
import edu.stanford.bmir.protege.web.client.form.LanguageMapCurrentLocaleMapper;
import edu.stanford.bmir.protege.web.client.library.msgbox.MessageBox;
Expand Down Expand Up @@ -66,6 +67,8 @@ public class PerspectivePresenter implements HasDispose {

private final LanguageMapCurrentLocaleMapper localeMapper;

private HandlerRegistration placeChangedHandlerRegistration;


@Inject
public PerspectivePresenter(final PerspectiveView perspectiveView,
Expand All @@ -91,8 +94,8 @@ public PerspectivePresenter(final PerspectiveView perspectiveView,

public void start(AcceptsOneWidget container, EventBus eventBus, ProjectViewPlace place) {
GWT.log("[PerspectivePresenter] Starting at place " + place);
eventBus.addHandler(PlaceChangeEvent.TYPE, event -> {
if(event.getNewPlace() instanceof ProjectViewPlace) {
placeChangedHandlerRegistration = eventBus.addHandler(PlaceChangeEvent.TYPE, event -> {
if (event.getNewPlace() instanceof ProjectViewPlace) {
displayPerspective(((ProjectViewPlace) event.getNewPlace()).getPerspectiveId());
}
});
Expand Down Expand Up @@ -213,6 +216,9 @@ private void savePerspectiveLayout(PerspectiveId perspectiveId, Optional<Node> n

@Override
public void dispose() {
if(placeChangedHandlerRegistration != null) {
placeChangedHandlerRegistration.removeHandler();
}
for(Perspective tab : perspectiveCache.values()) {
tab.dispose();
}
Expand Down
30 changes: 22 additions & 8 deletions ...t/src/main/java/edu/stanford/bmir/protege/web/client/perspective/PortletWidgetMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.google.gwt.user.client.ui.IsWidget;
import com.google.gwt.user.client.ui.Label;
import edu.stanford.bmir.protege.web.client.portlet.*;
import edu.stanford.bmir.protege.web.shared.HasDispose;
import edu.stanford.bmir.protege.web.shared.PortletId;
import edu.stanford.bmir.protege.web.shared.event.WebProtegeEventBus;
import edu.stanford.protege.widgetmap.client.HasFixedPrimaryAxisSize;
Expand All @@ -13,14 +14,14 @@
import edu.stanford.protege.widgetmap.shared.node.NodeProperties;
import edu.stanford.protege.widgetmap.shared.node.TerminalNode;
import edu.stanford.protege.widgetmap.shared.node.TerminalNodeId;
import org.slf4j.LoggerFactory;

import javax.annotation.Nonnull;
import javax.inject.Inject;
import javax.inject.Provider;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.*;
import java.util.function.Consumer;
import java.util.logging.Logger;

import static com.google.common.base.Preconditions.checkNotNull;

Expand All @@ -29,7 +30,9 @@
* Stanford Center for Biomedical Informatics Research
* 17/02/16
*/
public class PortletWidgetMapper implements WidgetMapper {
public class PortletWidgetMapper implements WidgetMapper, HasDispose {

private final Logger logger = Logger.getLogger("PortletWidgetMapper");

private static final String DROP_ZONE = "drop-zone";

Expand All @@ -45,6 +48,8 @@ public class PortletWidgetMapper implements WidgetMapper {

private Consumer<TerminalNode> nodePropertiesChangedHandler = node -> {};

private List<HasDispose> disposables = new ArrayList<>();

@Inject
public PortletWidgetMapper(@Nonnull PortletFactory portletFactory,
Provider<PortletUi> portletUiProvider,
Expand All @@ -69,19 +74,19 @@ public void setNodePropertiesChangedHandler(Consumer<TerminalNode> nodePropertie

@Override
public IsWidget getWidget(TerminalNode terminalNode) {
GWT.log("[PortletWidgetMapper] Getting widget for TerminalNode: " + terminalNode);
logger.info("logger.infoGetting widget for TerminalNode: " + terminalNode);
Copy link
Collaborator

@alexsilaghi alexsilaghi Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] I think we should replace the log.info with perhaps log.debug ? leave this kind of logs only for debugging purposes since it generates more traffic through the network.

ViewHolder cachedViewHolder = nodeId2ViewHolderMap.get(terminalNode.getNodeId());
if (cachedViewHolder != null) {
GWT.log("[PortletWidgetMapper] Using cached view: " + terminalNode);
logger.info("logger.infoUsing cached view: " + terminalNode);
return cachedViewHolder;
}
String portletClass = terminalNode.getNodeProperties().getPropertyValue("portlet", "");
GWT.log("[PortletWidgetMapper] Instantiate portlet: " + portletClass);
logger.info("logger.infoInstantiate portlet: " + portletClass);
ViewHolder viewHolder;
if (!portletClass.isEmpty()) {
Optional<WebProtegePortletComponents> thePortlet = portletFactory.createPortlet(new PortletId(portletClass));
if (thePortlet.isPresent()) {
GWT.log("[PortletWidgetMapper] Created portlet from auto-generated factory");
logger.info("logger.infoCreated portlet from auto-generated factory");
WebProtegePortletComponents portletComponents = thePortlet.get();
WebProtegePortletPresenter portletPresenter = portletComponents.getPresenter();
viewHolder = createViewHolder(terminalNode,
Expand Down Expand Up @@ -115,8 +120,10 @@ private ViewHolder createViewHolder(@Nonnull TerminalNode node,
nodePropertiesChangedHandler.accept(node);
});
WebProtegeEventBus eventBus = eventBusProvider.get();
disposables.add(eventBus);
portletUi.setTitle(portlet.getPortletDescriptor().getTitle());
WebProtegePortletPresenter portletPresenter = portlet.getPresenter();
disposables.add(portletPresenter);
portletPresenter.start(portletUi, eventBus);
ViewHolder viewHolder;
if (portletPresenter instanceof HasFixedPrimaryAxisSize) {
Expand All @@ -127,10 +134,17 @@ private ViewHolder createViewHolder(@Nonnull TerminalNode node,
}
viewHolder.addStyleName(DROP_ZONE);
viewHolder.addCloseHandler(event -> {
disposables.remove(eventBus);
eventBus.dispose();
disposables.remove(portletPresenter);
portletPresenter.dispose();
nodeId2ViewHolderMap.remove(node.getNodeId());
});
return viewHolder;
}

@Override
public void dispose() {
disposables.forEach(HasDispose::dispose);
}
}
22 changes: 13 additions & 9 deletions ...nt/src/main/java/edu/stanford/bmir/protege/web/client/place/WebProtegeActivityMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import edu.stanford.bmir.protege.web.client.projectsettings.ProjectSettingsActivity;
import edu.stanford.bmir.protege.web.client.search.EntitySearchSettingsActivity;
import edu.stanford.bmir.protege.web.client.search.EntitySearchSettingsPresenter;
import edu.stanford.bmir.protege.web.client.selection.SelectionModel;
import edu.stanford.bmir.protege.web.client.sharing.SharingSettingsActivity;
import edu.stanford.bmir.protege.web.client.sharing.SharingSettingsPresenter;
import edu.stanford.bmir.protege.web.client.signup.SignUpPresenter;
Expand All @@ -44,6 +45,7 @@
* 12/02/16
*/
public class WebProtegeActivityMapper implements ActivityMapper {

Logger logger = Logger.getLogger("WebProtegeActivityMapper");

private final ClientApplicationComponent applicationComponent;
Expand Down Expand Up @@ -86,9 +88,9 @@ public WebProtegeActivityMapper(LoggedInUserProvider loggedInUserProvider,
}

public void start() {
GWT.log("[WebProtegeActivityMapper] Started activity mapper.");
logger.info("Started activity mapper.");
eventBus.addHandler(UserLoggedOutEvent.ON_USER_LOGGED_OUT, event -> {
GWT.log("[WebProtegeActivityMapper] User logged out. Going to the Login Place.");
logger.info("User logged out. Going to the Login Place.");
LoginPlace loginPlace;
Place currentPlace = placeController.getWhere();
if (!(currentPlace instanceof LoginPlace)) {
Expand All @@ -102,16 +104,18 @@ public void start() {
}

private ClientProjectComponent getClientProjectComponentForProjectAndLoggedInUser(@Nonnull ProjectId projectId) {
logger.info("Getting project component for " + projectId + ". The current project is " + currentClientProjectComponent);
if(currentClientProjectComponent.isPresent()) {
ClientProjectComponent projectComponent = currentClientProjectComponent.get();
if(isProjectComponentForProject(projectComponent, projectId) && isLastUserSameAsLoggedInUser()) {
return projectComponent;
}
projectComponent.getProjectPresenter().dispose();
logger.info("Disposing of project component for " + projectComponent.getProjectId());
projectComponent.dispose();
}
ClientProjectComponent nextProjectComponent = instantiateClientProjectComponent(projectId);
// Reset project component and user
GWT.log("[WebProtegeActivityMapper] Instantiating new project component");
logger.info("Instantiating new project component");
lastUser = Optional.of(loggedInUserProvider.getCurrentUserId());
currentClientProjectComponent = Optional.of(nextProjectComponent);
return nextProjectComponent;
Expand All @@ -131,10 +135,10 @@ private ClientProjectComponent instantiateClientProjectComponent(@Nonnull Projec
}

public Activity getActivity(final Place place) {
GWT.log("[WebProtegeActivityMapper] Map place: " + place);
logger.info("Map place: " + place);
if (shouldRedirectToLogin(place)) {
GWT.log("[WebProtegeActivityMapper] User is not logged in. Redirecting to login.");
logger.info("[WebProtegeActivityMapper] User is not logged in. Redirecting to login.");
logger.info("User is not logged in. Redirecting to login.");
logger.info("User is not logged in. Redirecting to login.");
loginPresenter.setNextPlace(place);
Scheduler.get().scheduleFinally(() -> placeController.goTo(new LoginPlace(place)));
return new LoginActivity(loginPresenter);
Expand Down Expand Up @@ -168,7 +172,7 @@ public Activity getActivity(final Place place) {
}
if (place instanceof LoginPlace) {
if (!loggedInUserProvider.getCurrentUserId().isGuest()) {
logger.info("[WebProtegeActivityMapper] Schedule to project list after login.");
logger.info("Schedule to project list after login.");

Scheduler.get().scheduleFinally(() -> placeController.goTo(new ProjectListPlace()));
}
Expand All @@ -193,7 +197,7 @@ public Activity getActivity(final Place place) {
}

if (place instanceof ProjectListPlace) {
logger.info("[WebProtegeActivityMapper] Route to project list activity");
logger.info("Route to project list activity");
return new ProjectListActivity(projectManagerPresenter);
}

Expand Down
9 changes: 9 additions & 0 deletions ...java/edu/stanford/bmir/protege/web/client/portlet/AbstractWebProtegePortletPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import edu.stanford.bmir.protege.web.shared.lang.DisplayNameSettingsChangedEvent;
import edu.stanford.bmir.protege.web.shared.place.ProjectViewPlace;
import edu.stanford.bmir.protege.web.shared.project.ProjectId;
import edu.stanford.protege.widgetmap.client.view.HasViewTitle;
import org.semanticweb.owlapi.model.OWLEntity;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.util.Optional;
import java.util.logging.Logger;

import static com.google.common.base.MoreObjects.toStringHelper;
import static com.google.common.base.Preconditions.checkNotNull;
Expand All @@ -30,6 +32,8 @@

public abstract class AbstractWebProtegePortletPresenter implements WebProtegePortletPresenter, EntityDisplay {

private static final Logger logger = Logger.getLogger("AbstractWebProtegePortletPresenter");

private final SelectionModel selectionModel;

private final ProjectId projectId;
Expand All @@ -51,6 +55,8 @@ public abstract class AbstractWebProtegePortletPresenter implements WebProtegePo
@Nullable
private Place lastPlace = null;

private boolean disposed = false;

public AbstractWebProtegePortletPresenter(@Nonnull SelectionModel selectionModel,
@Nonnull ProjectId projectId,
@Nonnull DisplayNameRenderer displayNameRenderer,
Expand All @@ -60,6 +66,7 @@ public AbstractWebProtegePortletPresenter(@Nonnull SelectionModel selectionModel
this.projectId = checkNotNull(projectId);
this.displayNameRenderer = displayNameRenderer;
selectionModelHandlerRegistration = selectionModel.addSelectionChangedHandler(e -> {
logger.info("Handling selection changed in " + projectId + " (disposed=" + disposed + ") in " + portletUi.map(HasViewTitle::getViewTitle).orElse("(not set)"));
if (trackSelection) {
handleBeforeSetEntity(e.getPreviousSelection());
handleAfterSetEntity(e.getLastSelection());
Expand Down Expand Up @@ -201,6 +208,8 @@ public String toString() {

@Override
public void dispose() {
disposed = true;
logger.info("Disposing of portlet " + getClass().getSimpleName() + " in project " + projectId);
selectionModelHandlerRegistration.removeHandler();
portletUi.ifPresent(HasDispose::dispose);
}
Expand Down
1 change: 1 addition & 0 deletions ...i-client/src/main/java/edu/stanford/bmir/protege/web/client/project/ProjectPresenter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ public void dispose() {
linkBarPresenter.dispose();
perspectivePresenter.dispose();
eventPollingManager.stop();
eventBus.dispose();
}

@Override
Expand Down
Loading
Loading