Rename method in SmartImportJob and add getCurrentImportJob

- Rename "getImportJob" to "createOrGetConfiguredImportJob" to better express its behavior and also change its JavaDoc. - Add a new method "getCurrentImportJob" that merely retrieves the current job (without creating/configuring anything). - Extract private method in SmartImportTests - Remove calls to waitForJobs(...) in SmartImportTests and join the SmartImportJob instead. Fixes: eclipse-platform#1427 Fixes: eclipse-platform#1807
fedejeanne · Apr 12, 2024 · e033414 · e033414
1 parent 2e5b0bd
commit e033414
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 30 deletions.
diff --git a/...se.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportRootWizardPage.java b/...se.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportRootWizardPage.java
@@ -180,7 +180,7 @@ private class FolderForProjectsLabelProvider extends CellLabelProvider implement
 		public String getText(Object o) {
 			File file = (File) o;
 			Path filePath = file.toPath();
-			Path rootPath = getWizard().getImportJob().getRoot().toPath();
+			Path rootPath = getWizard().createOrGetConfiguredImportJob().getRoot().toPath();
 			if (filePath.startsWith(rootPath)) {
 				Path parent = rootPath.getParent();
 				if (parent != null) {
@@ -738,9 +738,9 @@ protected void validatePage() {
 
 	@Override
 	public boolean isPageComplete() {
-		return sourceIsValid() && getWizard().getImportJob() != null
-				&& (getWizard().getImportJob().getDirectoriesToImport() == null
-						|| !getWizard().getImportJob().getDirectoriesToImport().isEmpty());
+		return sourceIsValid() && getWizard().createOrGetConfiguredImportJob() != null
+				&& (getWizard().createOrGetConfiguredImportJob().getDirectoriesToImport() == null
+						|| !getWizard().createOrGetConfiguredImportJob().getDirectoriesToImport().isEmpty());
 	}
 
 	private boolean sourceIsValid() {
@@ -785,10 +785,10 @@ public Set<IWorkingSet> getSelectedWorkingSets() {
 	}
 
 	private void proposalsSelectionChanged() {
-		if (getWizard().getImportJob() != null) {
+		if (getWizard().createOrGetConfiguredImportJob() != null) {
 			if (potentialProjects.size() == 1 && potentialProjects.values().iterator().next().isEmpty()) {
-				getWizard().getImportJob().setDirectoriesToImport(null);
-				getWizard().getImportJob().setExcludedDirectories(null);
+				getWizard().createOrGetConfiguredImportJob().setDirectoriesToImport(null);
+				getWizard().createOrGetConfiguredImportJob().setExcludedDirectories(null);
 
 				selectionSummary.setText(NLS.bind(DataTransferMessages.SmartImportProposals_selectionSummary,
 						directoriesToImport.size(), 1));
@@ -797,8 +797,8 @@ private void proposalsSelectionChanged() {
 				for (File directory : this.directoriesToImport) {
 					excludedDirectories.remove(directory);
 				}
-				getWizard().getImportJob().setDirectoriesToImport(directoriesToImport);
-				getWizard().getImportJob().setExcludedDirectories(excludedDirectories);
+				getWizard().createOrGetConfiguredImportJob().setDirectoriesToImport(directoriesToImport);
+				getWizard().createOrGetConfiguredImportJob().setExcludedDirectories(excludedDirectories);
 				selectionSummary.setText(NLS.bind(DataTransferMessages.SmartImportProposals_selectionSummary,
 						directoriesToImport.size(),
 						potentialProjects.size()));
@@ -837,7 +837,7 @@ private void refreshProposals() {
 			TreeItem computingItem = new TreeItem(tree.getTree(), SWT.DEFAULT);
 			computingItem
 					.setText(NLS.bind(DataTransferMessages.SmartImportJob_inspecting, selection.getAbsolutePath()));
-			final SmartImportJob importJob = getWizard().getImportJob();
+			final SmartImportJob importJob = getWizard().createOrGetConfiguredImportJob();
 			refreshProposalsJob = new Job(
 					NLS.bind(DataTransferMessages.SmartImportJob_inspecting, selection.getAbsolutePath())) {
 				@Override
@@ -879,7 +879,7 @@ public void done(IJobChangeEvent event) {
 							IStatus result = event.getResult();
 							if (!control.isDisposed() && result.isOK()) {
 								computingItem.dispose();
-								if (sourceIsValid() && getWizard().getImportJob() == importJob) {
+								if (sourceIsValid() && getWizard().createOrGetConfiguredImportJob() == importJob) {
 									proposalsUpdated();
 								}
 								tree.getTree().setEnabled(potentialProjects.size() > 1);

diff --git a/...rg.eclipse.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportWizard.java b/...rg.eclipse.ui.ide/src/org/eclipse/ui/internal/wizards/datatransfer/SmartImportWizard.java
@@ -241,7 +241,7 @@ public boolean performFinish() {
 			System.arraycopy(previousProposals, 0, newProposals, 1, previousProposals.length);
 			getDialogSettings().put(SmartImportRootWizardPage.IMPORTED_SOURCES, newProposals);
 		}
-		SmartImportJob job = getImportJob();
+		SmartImportJob job = createOrGetConfiguredImportJob();
 		boolean runInBackground = WorkbenchPlugin.getDefault().getPreferenceStore()
 				.getBoolean(IPreferenceConstants.RUN_IN_BACKGROUND);
 		job.setProperty(IProgressConstants.PROPERTY_IN_DIALOG, runInBackground);
@@ -250,12 +250,20 @@ public boolean performFinish() {
 	}
 
 	/**
-	 * Get the import job that will be processed by this wizard. Can be null (if
-	 * provided directory is invalid).
+	 * Get or create the import job that will be processed by this wizard. Can be
+	 * <code>null</code> (if the provided directory is invalid).
 	 *
-	 * @return the import job
+	 * <strong>IMPORTANT:</strong> If there was already a job but it didn't match
+	 * the current configuration of the wizard then a new job will be created and
+	 * returned.
+	 *
+	 * Regardless of whether or not the job was created, some configurations will be
+	 * applied to it.
+	 *
+	 * @return the (newly created) import job
+	 * @see #getCurrentImportJob()
 	 */
-	public SmartImportJob getImportJob() {
+	public SmartImportJob createOrGetConfiguredImportJob() {
 		final File root = this.projectRootPage.getSelectedRoot();
 		if (root == null) {
 			return null;
@@ -270,16 +278,29 @@ public SmartImportJob getImportJob() {
 		} else {
 			return null;
 		}
+
 		if (this.easymportJob == null || !matchesPage(this.easymportJob, this.projectRootPage)) {
 			this.easymportJob = new SmartImportJob(this.directoryToImport, projectRootPage.getSelectedWorkingSets(),
 					projectRootPage.isConfigureProjects(), projectRootPage.isDetectNestedProject());
 		}
-		if (this.easymportJob != null) {
-			// always update working set on request as the job isn't updated on
-			// WS change automatically
-			this.easymportJob.setWorkingSets(projectRootPage.getSelectedWorkingSets());
-			this.easymportJob.setCloseProjectsAfterImport(projectRootPage.isCloseProjectsAfterImport());
-		}
+
+		// always update working set on request as the job isn't updated on
+		// WS change automatically
+		this.easymportJob.setWorkingSets(projectRootPage.getSelectedWorkingSets());
+		this.easymportJob.setCloseProjectsAfterImport(projectRootPage.isCloseProjectsAfterImport());
+
+		return this.easymportJob;
+	}
+
+	/**
+	 * Gets the current import job but it will not create it if it's
+	 * <code>null</code>. If you need to create the job based on the current
+	 * configuration of the wizard then you can call {@link #createOrGetConfiguredImportJob()}
+	 *
+	 * @return The current import job (it might be <code>null</code>).
+	 * @see #createOrGetConfiguredImportJob()
+	 */
+	public SmartImportJob getCurrentImportJob() {
 		return this.easymportJob;
 	}
 

diff --git a/...eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer/SmartImportTests.java b/...eclipse.ui.tests/Eclipse UI Tests/org/eclipse/ui/tests/datatransfer/SmartImportTests.java
@@ -150,10 +150,7 @@ private void proceedSmartImportWizard(SmartImportWizard wizard, Consumer<SmartIm
 			final Button okButton = getFinishButton(dialog.buttonBar);
 			assertNotNull(okButton);
 			processEventsUntil(() -> okButton.isEnabled(), -1);
-			wizard.performFinish();
-			waitForJobs(100, 1000); // give the job framework time to schedule the job
-			wizard.getImportJob().join();
-			waitForJobs(100, 5000); // give some time for asynchronous workspace jobs to complete
+			finishWizard(wizard);
 		} finally {
 			if (!dialog.getShell().isDisposed()) {
 				dialog.close();
@@ -442,10 +439,7 @@ public void testChangedWorkingSets() throws Exception {
 			combo.notifyListeners(SWT.Selection, e);
 			processEvents();
 			processEventsUntil(() -> okButton.isEnabled(), -1);
-			wizard.performFinish();
-			waitForJobs(100, 1000); // give the job framework time to schedule the job
-			wizard.getImportJob().join();
-			waitForJobs(100, 5000); // give some time for asynchronous workspace jobs to complete
+			finishWizard(wizard);
 			assertEquals("WorkingSet2 should be selected", Collections.singleton(workingSet2),
 					page.getSelectedWorkingSets());
 			assertEquals("Projects were not added to working set", 1, workingSet2.getElements().length);
@@ -459,6 +453,11 @@ public void testChangedWorkingSets() throws Exception {
 		}
 	}
 
+	private void finishWizard(SmartImportWizard wizard) throws InterruptedException {
+		wizard.performFinish();
+		wizard.getCurrentImportJob().join();
+	}
+
 	/**
 	 * Bug 559600 - [SmartImport] Label provider throws exception if results contain
 	 * filesystem root