From 511067d371ca6fcd4b66aedae188134ed6d17440 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20L=C3=A4ubrich?= Date: Fri, 20 Dec 2024 12:06:29 +0100 Subject: [PATCH] Refresh targets in the background A target refresh operation is currently run in the UI thread as it probably in the past where considered a fast operation. At least with P2 Updatesites this is no longer the case and we can'T really make any valid assumptions on how other implementation might perform. This now schedule the work into a background job and also cleanup the code to reflect current code base. In addition to that, if a job is already running, the user is asked to cancel it before perform other actions. Fix https://github.com/eclipse-pde/eclipse.pde/issues/1523 --- repository/.project | 11 ++ .../internal/ui/shared/target/Messages.java | 11 +- .../shared/target/TargetLocationsGroup.java | 117 +++++++++++------- .../ui/shared/target/UpdateTargetJob.java | 99 +++++++++------ .../ui/shared/target/messages.properties | 7 +- 5 files changed, 153 insertions(+), 92 deletions(-) create mode 100644 repository/.project diff --git a/repository/.project b/repository/.project new file mode 100644 index 0000000000..4bcf987e90 --- /dev/null +++ b/repository/.project @@ -0,0 +1,11 @@ + + + repository + + + + + + + + diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/Messages.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/Messages.java index a245d5dbb5..7232be9ec5 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/Messages.java +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/Messages.java @@ -18,6 +18,9 @@ public class Messages extends NLS { private static final String BUNDLE_NAME = "org.eclipse.pde.internal.ui.shared.target.messages"; //$NON-NLS-1$ + public static String UpdateTargetJob_RefreshingTarget; + public static String UpdateTargetJob_RefreshJobName; + public static String AddBundleContainerSelectionPage_1; public static String AddBundleContainerSelectionPage_10; public static String AddBundleContainerSelectionPage_2; @@ -148,13 +151,17 @@ public class Messages extends NLS { public static String TargetLocationsGroup_refresh; public static String TargetLocationsGroup_1; + public static String TargetLocationsGroup_group_refresh; + + public static String TargetLocationsGroup_operation_in_progress; + + public static String TargetLocationsGroup_operation_running; + public static String TargetLocationsGroup_TargetUpdateErrorDialog; public static String TargetStatus_NoActiveTargetPlatformStatus; public static String TargetStatus_TargetStatusDefaultString; public static String TargetStatus_UnresolvedTarget; public static String TargetStatus_UnresolvedTargetStatus; - public static String UpdateTargetJob_TargetUpdateFailedStatus; - public static String UpdateTargetJob_TargetUpdateSuccessStatus; public static String UpdateTargetJob_UpdateJobName; public static String UpdateTargetJob_UpdatingTarget; public static String EditTargetContainerPage_Add_Title; diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/TargetLocationsGroup.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/TargetLocationsGroup.java index ff7b0fee57..6cd976870e 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/TargetLocationsGroup.java +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/TargetLocationsGroup.java @@ -17,22 +17,19 @@ import static org.eclipse.swt.events.SelectionListener.widgetSelectedAdapter; -import java.util.Collections; -import java.util.List; import java.util.Objects; import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.ICoreRunnable; import org.eclipse.core.runtime.IStatus; import org.eclipse.core.runtime.ListenerList; -import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.core.runtime.jobs.IJobChangeEvent; -import org.eclipse.core.runtime.jobs.IJobFunction; import org.eclipse.core.runtime.jobs.Job; import org.eclipse.core.runtime.jobs.JobChangeAdapter; import org.eclipse.jface.action.Action; import org.eclipse.jface.action.MenuManager; import org.eclipse.jface.dialogs.ErrorDialog; +import org.eclipse.jface.dialogs.MessageDialog; import org.eclipse.jface.viewers.AbstractTreeViewer; import org.eclipse.jface.viewers.ITreeSelection; import org.eclipse.jface.viewers.TreePath; @@ -121,6 +118,7 @@ static DeleteButtonState computeState(boolean canRemove, boolean canEnable, bool private final ListenerList fChangeListeners = new ListenerList<>(); private final ListenerList fReloadListeners = new ListenerList<>(); private static final TargetLocationHandlerAdapter ADAPTER = new TargetLocationHandlerAdapter(); + private Job targetLocationJob; /** * Creates this part using the form toolkit and adds it to the given @@ -483,47 +481,60 @@ private void handleUpdate() { fUpdateButton.setEnabled(false); return; } - List updateActions = Collections - .singletonList(monitor -> log(ADAPTER.update(fTarget, selection.getPaths(), monitor))); - JobChangeAdapter listener = new JobChangeAdapter() { - @Override - public void done(final IJobChangeEvent event) { - UIJob job = UIJob.create(Messages.UpdateTargetJob_UpdateJobName, monitor -> { - IStatus result = event.getJob().getResult(); - if (!result.isOK()) { - if (!fTreeViewer.getControl().isDisposed()) { - ErrorDialog.openError(fTreeViewer.getTree().getShell(), - Messages.TargetLocationsGroup_TargetUpdateErrorDialog, result.getMessage(), result); - } - } else if (result.getCode() != ITargetLocationHandler.STATUS_CODE_NO_CHANGE) { - // Update was successful and changed the target, if - // dialog/editor still open, update it - if (!fTreeViewer.getControl().isDisposed()) { - contentsChanged(true); - fTreeViewer.refresh(true); - updateButtons(); - } + if (checkJob()) { + JobChangeAdapter listener = new JobChangeAdapter() { + @Override + public void done(final IJobChangeEvent event) { + UIJob job = UIJob.create(Messages.UpdateTargetJob_UpdateJobName, monitor -> { + targetLocationJob = null; + IStatus result = event.getJob().getResult(); + log(result); + if (!result.isOK()) { + if (!fTreeViewer.getControl().isDisposed()) { + ErrorDialog.openError(fTreeViewer.getTree().getShell(), + Messages.TargetLocationsGroup_TargetUpdateErrorDialog, result.getMessage(), + result); + } + } else if (result.getCode() != ITargetLocationHandler.STATUS_CODE_NO_CHANGE) { + // Update was successful and changed the target, if + // dialog/editor still open, update it + if (!fTreeViewer.getControl().isDisposed()) { + contentsChanged(true); + fTreeViewer.refresh(true); + updateButtons(); + } - // If the target is the current platform, run a load - // job for the user - try { - ITargetPlatformService service = PDECore.getDefault() - .acquireService(ITargetPlatformService.class); - if (service != null) { - ITargetHandle currentTarget = service.getWorkspaceTargetHandle(); - if (fTarget.getHandle().equals(currentTarget)) - LoadTargetDefinitionJob.load(fTarget); + // If the target is the current platform, run a load + // job for the user + try { + ITargetPlatformService service = PDECore.getDefault() + .acquireService(ITargetPlatformService.class); + if (service != null) { + ITargetHandle currentTarget = service.getWorkspaceTargetHandle(); + if (fTarget.getHandle().equals(currentTarget)) + LoadTargetDefinitionJob.load(fTarget); + } + } catch (CoreException e) { + // do nothing if we could not set the current + // target. } - } catch (CoreException e) { - // do nothing if we could not set the current - // target. } - } - }); - job.schedule(); - } - }; - UpdateTargetJob.update(updateActions, listener); + }); + job.schedule(); + } + }; + targetLocationJob = UpdateTargetJob.update(fTarget, ADAPTER, selection.getPaths(), listener, + targetLocationJob); + } + } + + private boolean checkJob() { + Job job = targetLocationJob; + if (job == null) { + return true; + } + return MessageDialog.openQuestion(fTreeViewer.getControl().getShell(), + Messages.TargetLocationsGroup_operation_in_progress, Messages.TargetLocationsGroup_operation_running); } private void updateButtons() { @@ -562,16 +573,28 @@ private void updateButtons() { case ENABLE -> fRemoveButton.setText(Messages.BundleContainerTable_Btn_Text_Enable); case TOGGLE -> fRemoveButton.setText(Messages.BundleContainerTable_Btn_Text_Toggle); default -> fRemoveButton.setText(Messages.BundleContainerTable_Btn_Text_Remove); - } + } fRemoveButton.setEnabled(state != DeleteButtonState.NONE); fRemoveButton.setData(BUTTON_STATE, state); } private void handleReload() { - log(ADAPTER.reload(fTarget, fTarget.getTargetLocations(), new NullProgressMonitor())); - Job job = UIJob.create("Refreshing...", (ICoreRunnable) monitor -> contentsReload()); //$NON-NLS-1$ - job.schedule(); - + if (checkJob()) { + targetLocationJob = UpdateTargetJob.refresh(fTarget, ADAPTER, new JobChangeAdapter() { + @Override + public void done(IJobChangeEvent event) { + IStatus result = event.getResult(); + log(result); + if (result.isOK()) { + Job job = UIJob.create(Messages.TargetLocationsGroup_group_refresh, (ICoreRunnable) monitor -> { + targetLocationJob = null; + contentsReload(); + }); + job.schedule(); + } + } + }, targetLocationJob); + } } private void toggleCollapse() { diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/UpdateTargetJob.java b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/UpdateTargetJob.java index 94734e9f58..e327ca56d0 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/UpdateTargetJob.java +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/UpdateTargetJob.java @@ -16,21 +16,18 @@ *******************************************************************************/ package org.eclipse.pde.internal.ui.shared.target; -import java.util.List; import java.util.Map; import java.util.Set; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; -import org.eclipse.core.runtime.MultiStatus; -import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.SubMonitor; import org.eclipse.core.runtime.jobs.IJobChangeListener; import org.eclipse.core.runtime.jobs.IJobFunction; import org.eclipse.core.runtime.jobs.Job; +import org.eclipse.jface.viewers.TreePath; import org.eclipse.pde.core.target.ITargetDefinition; import org.eclipse.pde.core.target.ITargetLocation; -import org.eclipse.pde.internal.core.PDECore; import org.eclipse.pde.ui.target.ITargetLocationHandler; /** @@ -44,22 +41,24 @@ */ public class UpdateTargetJob extends Job { - public static final String JOB_FAMILY_ID = "UpdateTargetJob"; //$NON-NLS-1$ + private final IJobFunction action; - private final List toUpdate; + private boolean update; + + private Job cancelJob; /** - * Schedules a new update job that will update all target locations in the - * provided map. A target's selected children can be added as a set to the - * values of the map so that only certain portions of the target location - * get updated. + * Schedules a new update job that will update all selected children of the + * target location get updated. *

- * If all calls to {@link IJobFunction#run(IProgressMonitor)} return an OK - * status with {@link ITargetLocationHandler#STATUS_CODE_NO_CHANGE}, the - * returned status will also have that status code, indicating that no - * changes were made to the target. + * If no changes are performed an OK status with + * {@link ITargetLocationHandler#STATUS_CODE_NO_CHANGE}, the returned status + * will also have that status code, indicating that no changes were made to + * the target. *

* + * @param treePaths + * * @param updateActions * maps {@link ITargetLocation}s to the {@link Set} of selected * children items that should be updated. The sets may be empty, @@ -67,49 +66,67 @@ public class UpdateTargetJob extends Job { * @param listener * job change listener that will be added to the created job, can * be null + * @param cancelJob + * @return the job created and scheduled + */ + public static Job update(ITargetDefinition definition, ITargetLocationHandler handler, TreePath[] treePaths, + IJobChangeListener listener, Job cancelJob) { + Job job = new UpdateTargetJob(cancelJob, monitor -> handler.update(definition, treePaths, monitor), true); + job.setUser(true); + if (listener != null) { + job.addJobChangeListener(listener); + } + job.schedule(); + return job; + } + + /** + * Schedules a new update job that will refresh all target locations in the + * provided target. + * + * @param definition + * the target definition to refresh + * @param listener + * job change listener that will be added to the created job, can + * be null + * @param cancelJob + * @return the job created and scheduled */ - public static void update(List updateActions, IJobChangeListener listener) { - Job.getJobManager().cancel(JOB_FAMILY_ID); - Job job = new UpdateTargetJob(updateActions); + public static Job refresh(ITargetDefinition definition, ITargetLocationHandler handler, + IJobChangeListener listener, Job cancelJob) { + Job job = new UpdateTargetJob(cancelJob, + monitor -> handler.reload(definition, definition.getTargetLocations(), monitor), + false); job.setUser(true); if (listener != null) { job.addJobChangeListener(listener); } job.schedule(); + return job; } /** * Use {@link #update(ITargetDefinition, Map, IJobChangeListener)} instead */ - private UpdateTargetJob(List updateActions) { - super(Messages.UpdateTargetJob_UpdateJobName); - this.toUpdate = updateActions; + private UpdateTargetJob(Job cancelJob, IJobFunction updateActions, boolean update) { + super(update ? Messages.UpdateTargetJob_UpdateJobName : Messages.UpdateTargetJob_RefreshJobName); + this.cancelJob = cancelJob; + this.action = updateActions; + this.update = update; } @Override protected IStatus run(IProgressMonitor monitor) { - SubMonitor progress = SubMonitor.convert(monitor, Messages.UpdateTargetJob_UpdatingTarget, - toUpdate.size() * 100); - MultiStatus errors = new MultiStatus(PDECore.PLUGIN_ID, 0, Messages.UpdateTargetJob_TargetUpdateFailedStatus, - null); - boolean noChange = true; - for (IJobFunction action : toUpdate) { - IStatus result = action.run(progress.split(100)); - if (result.isOK() && result.getCode() != ITargetLocationHandler.STATUS_CODE_NO_CHANGE) { - noChange = false; - } else if (!result.isOK()) { - noChange = false; - errors.add(result); + SubMonitor progress = SubMonitor.convert(monitor, + update ? Messages.UpdateTargetJob_UpdatingTarget : Messages.UpdateTargetJob_RefreshingTarget, 100); + if (cancelJob != null) { + cancelJob.cancel(); + try { + cancelJob.join(); + } catch (InterruptedException e) { + // we tried our best } } - progress.done(); - if (noChange) { - return new Status(IStatus.OK, PDECore.PLUGIN_ID, ITargetLocationHandler.STATUS_CODE_NO_CHANGE, - Messages.UpdateTargetJob_TargetUpdateSuccessStatus, null); - } else if (!errors.isOK()) { - return errors; - } else { - return Status.OK_STATUS; - } + return action.run(progress); } } diff --git a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/messages.properties b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/messages.properties index 671541550e..474e128aa7 100644 --- a/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/messages.properties +++ b/ui/org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target/messages.properties @@ -138,15 +138,18 @@ TargetContentsGroup_resolveCancelled=Resolve Cancelled TargetLocationsGroup_update=Looks for newer versions of the target contents and updates them TargetLocationsGroup_refresh=Clears the cached target data and resolves the target, looking for newer versions of any artifacts TargetLocationsGroup_1=&Show location content +TargetLocationsGroup_group_refresh=Refreshing... +TargetLocationsGroup_operation_in_progress=Target operation in progress +TargetLocationsGroup_operation_running=A target editor operation is currently running, do you want to cancel it? TargetLocationsGroup_TargetUpdateErrorDialog=Problems Updating Target Definition TargetStatus_NoActiveTargetPlatformStatus=No active target platform TargetStatus_TargetStatusDefaultString=Target Platform TargetStatus_UnresolvedTarget=Unresolved ''{0}'' TargetStatus_UnresolvedTargetStatus=Target platform is not loaded -UpdateTargetJob_TargetUpdateFailedStatus=The target definition did not update successfully -UpdateTargetJob_TargetUpdateSuccessStatus=Target definition update completed successfully UpdateTargetJob_UpdateJobName=Update Target Definition +UpdateTargetJob_RefreshJobName=Refresh Target Definition UpdateTargetJob_UpdatingTarget=Updating Target +UpdateTargetJob_RefreshingTarget=Refreshing Target EditTargetContainerPage_Add_Title=Create target reference EditTargetContainerPage_Edit_Title=Edit target reference EditTargetContainerPage_Message=Please enter a URI to the target to be referenced