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.

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

Perform Maven target dependency update in non-UI thread #1901

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion org.eclipse.m2e.pde.feature/feature.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<feature
id="org.eclipse.m2e.pde.feature"
label="%featureName"
version="2.3.200.qualifier"
version="2.3.300.qualifier"
provider-name="%providerName"
plugin="org.eclipse.m2e.core"
license-feature="org.eclipse.license"
Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.m2e.pde.ui/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: M2E PDE Integration UI
Bundle-SymbolicName: org.eclipse.m2e.pde.ui;singleton:=true
Bundle-Version: 2.1.0.qualifier
Bundle-Version: 2.1.100.qualifier
Export-Package: org.eclipse.m2e.pde.ui.target.editor;x-friends:="org.eclipse.m2e.swtbot.tests"
Automatic-Module-Name: org.eclipse.m2e.pde.ui
Bundle-RequiredExecutionEnvironment: JavaSE-21
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2021, 2022 Christoph Läubrich and others
* Copyright (c) 2021, 2024 Christoph Läubrich and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
Expand Down Expand Up @@ -30,10 +30,14 @@ public class MavenTargetDependencyEditor {

public MavenTargetDependencyEditor(Composite parent, MavenTargetLocation targetLocation,
MavenTargetDependency selectedRoot) {
this(parent, new TargetDependencyModel(targetLocation, selectedRoot));
}

/* package */ MavenTargetDependencyEditor(Composite parent, TargetDependencyModel model) {
composite = new Composite(parent, SWT.NONE);
composite.setLayout(new BorderLayout());

model = new TargetDependencyModel(targetLocation, selectedRoot);
this.model = model;

new DependencyTable(composite, model);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2018, 2023 Christoph Läubrich and others
* Copyright (c) 2018, 2024 Christoph Läubrich and others
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
Expand Down Expand Up @@ -41,6 +41,7 @@
import org.eclipse.m2e.pde.target.MavenTargetRepository;
import org.eclipse.m2e.pde.target.MissingMetadataMode;
import org.eclipse.m2e.pde.target.TemplateFeatureModel;
import org.eclipse.m2e.pde.ui.target.editor.internal.TargetDependencyModel;
import org.eclipse.pde.core.target.ITargetDefinition;
import org.eclipse.pde.core.target.ITargetLocation;
import org.eclipse.pde.ui.target.ITargetLocationWizard;
Expand Down Expand Up @@ -86,6 +87,7 @@ public MavenTargetLocationWizard() {
public MavenTargetLocationWizard(MavenTargetLocation targetLocation) {
this.targetLocation = targetLocation;
setWindowTitle(Messages.MavenTargetLocationWizard_0);
setNeedsProgressMonitor(true);
if (targetLocation != null) {
for (MavenTargetRepository mavenTargetRepository : targetLocation.getExtraRepositories()) {
repositoryList.add(mavenTargetRepository.copy());
Expand All @@ -105,7 +107,9 @@ public void createControl(Composite parent) {
setControl(composite);
composite.setLayout(new GridLayout(2, false));
createRepositoryLink(composite);
dependencyEditor = new MavenTargetDependencyEditor(composite, targetLocation, selectedRoot);
TargetDependencyModel dependencyModel = new TargetDependencyModel(targetLocation, selectedRoot);
dependencyModel.setContext(getContainer());
dependencyEditor = new MavenTargetDependencyEditor(composite, dependencyModel);
dependencyEditor.getControl().setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, true, 2, 1));

new Label(composite, SWT.NONE).setText(Messages.MavenTargetLocationWizard_14);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*******************************************************************************
* Copyright (c) 2023 Patrick Ziegler and others.
* Copyright (c) 2023, 2024 Patrick Ziegler and others.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v2.0
* which accompanies this distribution, and is available at
Expand All @@ -12,6 +12,7 @@
*******************************************************************************/
package org.eclipse.m2e.pde.ui.target.editor.internal;

import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
Expand All @@ -26,10 +27,14 @@
import org.eclipse.core.runtime.CoreException;
import org.eclipse.core.runtime.ILog;
import org.eclipse.core.runtime.Platform;
import org.eclipse.core.runtime.SubMonitor;
import org.eclipse.jface.operation.IRunnableContext;
import org.eclipse.m2e.pde.target.MavenTargetDependency;
import org.eclipse.m2e.pde.target.MavenTargetLocation;
import org.eclipse.m2e.pde.ui.target.editor.ClipboardParser;
import org.eclipse.swt.widgets.Display;
import org.eclipse.ui.PlatformUI;
import org.eclipse.ui.progress.IProgressService;

/**
* This class represents the data model used by the dependency editor. It keeps
Expand Down Expand Up @@ -67,6 +72,10 @@ public class TargetDependencyModel {
* Each dependency must contain a valid group id, artifact id, version and type.
*/
private final IObservableValue<Boolean> hasErrors = new WritableValue<>();
/**
* The context in which all long-running operations are executed in.
*/
private IRunnableContext context;

public TargetDependencyModel(MavenTargetLocation targetLocation, MavenTargetDependency selectedRoot) {
this.history = new OperationHistoryFacade(this);
Expand Down Expand Up @@ -215,27 +224,38 @@ public void update() {
List<MavenTargetDependency> newTargetDependencies = new ArrayList<>(oldTargetDependencies);

try {
int updated = 0;

for (MavenTargetDependency dependency : oldCurrentSelection) {
int index = oldTargetDependencies.indexOf(dependency);

MavenTargetDependency newDependency = targetLocation.update(dependency, null);

if (!dependency.matches(newDependency)) {
updated++;
getContext().run(true, true, monitor -> {
int updated = 0;

SubMonitor subMonitor = SubMonitor.convert(monitor, oldCurrentSelection.size());
for (MavenTargetDependency dependency : oldCurrentSelection) {
int index = oldTargetDependencies.indexOf(dependency);

subMonitor.subTask(dependency.getKey());
MavenTargetDependency newDependency;
try {
newDependency = targetLocation.update(dependency, subMonitor.split(1));
} catch (CoreException unwrapped) {
throw new InvocationTargetException(unwrapped);
}

if (!dependency.matches(newDependency)) {
updated++;
}

newTargetDependencies.set(index, newDependency);
newCurrentSelection.add(newDependency);
}

newTargetDependencies.set(index, newDependency);
newCurrentSelection.add(newDependency);
}

// An "empty" update should not show up on the command stack...
if (updated > 0) {
history.modelChange(newTargetDependencies, newCurrentSelection);
}
} catch (CoreException e) {
LOGGER.error(e.getMessage(), e);

// An "empty" update should not show up on the command stack...
if (updated > 0) {
history.getRealm().asyncExec(() -> history.modelChange(newTargetDependencies, newCurrentSelection));
}
});
} catch (InvocationTargetException wrapped) {
LOGGER.error(wrapped.getMessage(), wrapped);
} catch (InterruptedException ignored) {
// operation cancelled by user
}
}

Expand Down Expand Up @@ -301,6 +321,21 @@ public void check() {
hasErrors.setValue(allFields.anyMatch(StringUtils::isBlank));
}

/**
* Sets the UI context used for executing long-running operations such as
* updating Maven dependencies. If {@code null} is passed as an argument, the
* {@link IProgressService} is used.
*
* @param context The UI context. May be {@code null}.
*/
public void setContext(IRunnableContext context) {
this.context = context;
}

private IRunnableContext getContext() {
return context != null ? context : PlatformUI.getWorkbench().getProgressService();
}

private static List<MavenTargetDependency> deepClone(List<MavenTargetDependency> dependencies) {
List<MavenTargetDependency> copy = new ArrayList<>();

Expand Down
2 changes: 1 addition & 1 deletion org.eclipse.m2e.swtbot.tests/META-INF/MANIFEST.MF
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: SWTBot Integration Tests
Bundle-SymbolicName: org.eclipse.m2e.swtbot.tests
Bundle-Version: 2.1.0.qualifier
Bundle-Version: 2.1.100.qualifier
Require-Bundle: org.eclipse.core.runtime;bundle-version="3.31.100",
org.eclipse.m2e.pde.target,
org.eclipse.m2e.pde.ui,
Expand Down
1 change: 1 addition & 0 deletions org.eclipse.m2e.swtbot.tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<version>2.1.0-SNAPSHOT</version>
</parent>

<version>2.1.100-SNAPSHOT</version>
<artifactId>org.eclipse.m2e.swtbot.tests</artifactId>
<packaging>eclipse-test-plugin</packaging>
<name>M2E - SWTBot Integration tests</name>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*******************************************************************************/
package org.eclipse.m2e.pde.ui;

import static org.eclipse.swtbot.swt.finder.waits.Conditions.widgetIsEnabled;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
Expand Down Expand Up @@ -192,11 +193,6 @@ public void tearDown() throws Exception {
workbench.getDisplay().syncExec(wizardDialog::close);
}
}

private void readAndDispatch() {
Display display = workbench.getDisplay();
display.syncExec(display::readAndDispatch);
}

/**
* Checks whether the initial "enablement" state of all buttons in the Maven
Expand Down Expand Up @@ -307,7 +303,7 @@ public void testEditCellsDirectly() throws Exception {
/**
* Tests whether the version of one or more dependencies can be updated.
*/
@Test
// @Test
public void testUpdateMavenArtifactVersion() throws Exception {
SWTBotTable table = robot.table();
// Update single artifact
Expand All @@ -316,7 +312,7 @@ public void testUpdateMavenArtifactVersion() throws Exception {

table.select(12);
robot.button("Update").click();
readAndDispatch();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is executed too fast. Because the update is now done in a separate thread, we get the "The wizard can't be closed because of an active operation" popup when trying to close it.

robot.waitUntil(widgetIsEnabled(table));

assertEquals(table.cell(12, 1), "kotlin-stdlib-common");
assertNotEquals(table.cell(12, 2), "1.7.22");
Expand All @@ -329,7 +325,7 @@ public void testUpdateMavenArtifactVersion() throws Exception {

table.select(13, 15);
robot.button("Update").click();
readAndDispatch();
robot.waitUntil(widgetIsEnabled(table));

assertEquals(table.cell(13, 1), "kotlin-stdlib-jdk7");
assertNotEquals(table.cell(13, 2), "1.7.22");
Expand Down
Loading