From: Alexandre Montplaisir Date: Wed, 4 May 2016 21:23:12 +0000 (-0400) Subject: analysis.lami: Replace OnDemandAnalysisException with CoreException X-Git-Url: http://git.efficios.com/?p=deliverable%2Ftracecompass.git;a=commitdiff_plain;h=46f0c09c9daf5225c930a368c2230511a6de57a5 analysis.lami: Replace OnDemandAnalysisException with CoreException We may want to pass additional information to the exceptions thrown when a LAMI analysis does not complete succesfully, like what type of error should be displayed. CoreException and IStatus are made exactly for that, and they hook nicely into ErrorMessage to display, for example, a command-line stderr output as additional Details in the dialog. That way, a message like "no results returned" won't look like a scary error. Bug: 493941 Change-Id: Id3e6711ac410c3d993b4928a350dc1bdbcf89f5a Signed-off-by: Alexandre Montplaisir Reviewed-on: https://git.eclipse.org/r/72242 Reviewed-by: Hudson CI Reviewed-by: Bernd Hufmann Tested-by: Bernd Hufmann Reviewed-by: Matthew Khouzam --- diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiAnalysisStub.java b/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiAnalysisStub.java index 18aa5dcaef..490b0bab77 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiAnalysisStub.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiAnalysisStub.java @@ -20,12 +20,12 @@ import java.util.Collections; import java.util.List; import java.util.stream.Collectors; +import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.module.LamiAnalysis; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.module.LamiChartModel; -import org.eclipse.tracecompass.tmf.core.analysis.ondemand.OnDemandAnalysisException; import org.eclipse.tracecompass.tmf.core.trace.ITmfTrace; import com.google.common.collect.ImmutableMultimap; @@ -65,8 +65,7 @@ public class LamiAnalysisStub extends LamiAnalysis { @Override protected String getResultsFromCommand(List command, IProgressMonitor monitor) - throws OnDemandAnalysisException { - + throws CoreException { return readLamiFile(fResultFilename); } diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiJsonParserTest.java b/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiJsonParserTest.java index 6f6acc5cab..0f50cd5e28 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiJsonParserTest.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.core.tests/src/org/eclipse/tracecompass/analysis/lami/core/tests/LamiJsonParserTest.java @@ -19,6 +19,7 @@ import static org.junit.Assert.assertTrue; import java.util.List; import java.util.Map; +import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.aspect.LamiTableEntryAspect; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.module.LamiResultTable; @@ -30,7 +31,6 @@ import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.La import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.LamiSize; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.LamiSystemCall; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.LamiTimeRange; -import org.eclipse.tracecompass.tmf.core.analysis.ondemand.OnDemandAnalysisException; import org.eclipse.tracecompass.tmf.core.timestamp.ITmfTimestamp; import org.eclipse.tracecompass.tmf.core.timestamp.TmfTimeRange; import org.eclipse.tracecompass.tmf.core.timestamp.TmfTimestamp; @@ -124,10 +124,10 @@ public class LamiJsonParserTest { /** * Test the results parsing. * - * @throws OnDemandAnalysisException when execute() fails. + * @throws CoreException when execute() fails. */ @Test - public void testResults() throws OnDemandAnalysisException { + public void testResults() throws CoreException { LamiAnalysisStub analysis = new LamiAnalysisStub("test-metadata.json", "test-results.json"); List resultTables = analysis.execute(fTrace, null, "", new NullProgressMonitor()); @@ -213,10 +213,10 @@ public class LamiJsonParserTest { /** * Test the error parsing of the results. * - * @throws OnDemandAnalysisException when execute() fails. + * @throws CoreException when execute() fails. */ - @Test (expected = OnDemandAnalysisException.class) - public void testResultsError() throws OnDemandAnalysisException { + @Test (expected = CoreException.class) + public void testResultsError() throws CoreException { LamiAnalysisStub analysis = new LamiAnalysisStub("test-metadata.json", "test-error.json"); analysis.execute(fTrace, null, "", new NullProgressMonitor()); diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/LamiAnalysis.java b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/LamiAnalysis.java index 5cd62122c6..200c4e15fc 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/LamiAnalysis.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/LamiAnalysis.java @@ -30,7 +30,11 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.eclipse.core.runtime.CoreException; 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.jdt.annotation.NonNull; import org.eclipse.jdt.annotation.Nullable; import org.eclipse.tracecompass.internal.analysis.lami.core.Activator; @@ -54,7 +58,6 @@ import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.La import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.LamiData.DataType; import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.types.LamiTimeRange; import org.eclipse.tracecompass.tmf.core.analysis.ondemand.IOnDemandAnalysis; -import org.eclipse.tracecompass.tmf.core.analysis.ondemand.OnDemandAnalysisException; import org.eclipse.tracecompass.tmf.core.timestamp.TmfTimeRange; import org.eclipse.tracecompass.tmf.core.trace.ITmfTrace; import org.json.JSONArray; @@ -487,12 +490,12 @@ public class LamiAnalysis implements IOnDemandAnalysis { * @param monitor * The progress monitor used to report progress * @return The script's output, formatted into {@link LamiTableEntry}'s. - * @throws OnDemandAnalysisException + * @throws CoreException * If execution did not terminate normally */ @Override public List execute(ITmfTrace trace, @Nullable TmfTimeRange timeRange, - String extraParams, IProgressMonitor monitor) throws OnDemandAnalysisException { + String extraParams, IProgressMonitor monitor) throws CoreException { /* Should have been called already, but in case it was not */ initialize(); @@ -511,7 +514,8 @@ public class LamiAnalysis implements IOnDemandAnalysis { String output = getResultsFromCommand(command, monitor); if (output.isEmpty()) { - throw new OnDemandAnalysisException(Messages.LamiAnalysis_NoResults); + IStatus status = new Status(IStatus.INFO, Activator.instance().getPluginId(), Messages.LamiAnalysis_NoResults); + throw new CoreException(status); } /* @@ -585,7 +589,8 @@ public class LamiAnalysis implements IOnDemandAnalysis { * No results were reported. This may be normal, but warn the * user why a report won't be created. */ - throw new OnDemandAnalysisException(Messages.LamiAnalysis_NoResults); + IStatus status = new Status(IStatus.INFO, Activator.instance().getPluginId(), Messages.LamiAnalysis_NoResults); + throw new CoreException(status); } for (int i = 0; i < results.length(); i++) { @@ -652,7 +657,8 @@ public class LamiAnalysis implements IOnDemandAnalysis { } } catch (JSONException e) { - throw new OnDemandAnalysisException(e.getMessage()); + IStatus status = new Status(IStatus.ERROR, Activator.instance().getPluginId(), e.getMessage(), e); + throw new CoreException(status); } return resultsBuilder.build(); @@ -707,13 +713,13 @@ public class LamiAnalysis implements IOnDemandAnalysis { * @param monitor * The progress monitor * @return The analysis results - * @throws OnDemandAnalysisException + * @throws CoreException * If the command ended abnormally, and normal results were not * returned */ @VisibleForTesting protected String getResultsFromCommand(List command, IProgressMonitor monitor) - throws OnDemandAnalysisException { + throws CoreException { final int scale = 1000; double workedSoFar = 0.0; @@ -792,7 +798,8 @@ public class LamiAnalysis implements IOnDemandAnalysis { if (monitor.isCanceled()) { /* We were interrupted by the canceller thread. */ - throw new OnDemandAnalysisException(null); + IStatus status = new Status(IStatus.CANCEL, Activator.instance().getPluginId(), null); + throw new CoreException(status); } if (ret != 0) { @@ -801,8 +808,21 @@ public class LamiAnalysis implements IOnDemandAnalysis { * gather the stderr and report it to the user. */ BufferedReader br = new BufferedReader(new InputStreamReader(p.getErrorStream())); - String stdErrOutput = br.lines().collect(Collectors.joining("\n")); //$NON-NLS-1$ - throw new OnDemandAnalysisException(stdErrOutput); + List stdErrOutput = br.lines().collect(Collectors.toList()); + + MultiStatus status = new MultiStatus(Activator.instance().getPluginId(), + IStatus.ERROR, Messages.LamiAnalysis_ErrorDuringExecution, null); + for (String str : stdErrOutput) { + status.add(new Status(IStatus.ERROR, Activator.instance().getPluginId(), str)); + } + if (stdErrOutput.isEmpty()) { + /* + * At least say "no output", so an error message actually + * shows up. + */ + status.add(new Status(IStatus.ERROR, Activator.instance().getPluginId(), Messages.LamiAnalysis_ErrorNoOutput)); + } + throw new CoreException(status); } /* External script ended successfully, all is fine! */ @@ -810,7 +830,8 @@ public class LamiAnalysis implements IOnDemandAnalysis { return checkNotNull(resultsStr); } catch (IOException | InterruptedException e) { - throw new OnDemandAnalysisException(Messages.LamiAnalysis_ExecutionInterrupted); + IStatus status = new Status(IStatus.ERROR, Activator.instance().getPluginId(), Messages.LamiAnalysis_ExecutionInterrupted, e); + throw new CoreException(status); } finally { if (cancellerRunnable != null) { diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/Messages.java b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/Messages.java index 1ca275ae41..8909b02c02 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/Messages.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/Messages.java @@ -27,6 +27,8 @@ public class Messages extends NLS { public static String LamiAnalysis_MainTaskName; public static String LamiAnalysis_NoResults; + public static String LamiAnalysis_ErrorDuringExecution; + public static String LamiAnalysis_ErrorNoOutput; public static String LamiAnalysis_ExecutionInterrupted; public static String LamiAnalysis_ExtendedTableNamePrefix; diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/messages.properties b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/messages.properties index c91b0a7491..2b04f445a7 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/messages.properties +++ b/analysis/org.eclipse.tracecompass.analysis.lami.core/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/core/module/messages.properties @@ -11,6 +11,8 @@ LamiAnalysis_DefaultDynamicTableName = Dynamic Table LamiAnalysis_MainTaskName = Invoking external analysis script LamiAnalysis_NoResults = No results were returned. +LamiAnalysis_ErrorDuringExecution = Error during execution of the script. +LamiAnalysis_ErrorNoOutput = (No output) LamiAnalysis_ExecutionInterrupted = Execution was interrupted. LamiAnalysis_ExtendedTableNamePrefix = Extended diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/Messages.java b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/Messages.java index d1811eb7c3..58456de249 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/Messages.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/Messages.java @@ -30,8 +30,12 @@ public class Messages extends NLS { public static String ParameterDialog_ExternalParametersDescription; public static String ParameterDialog_StringValidatorMessage; public static String ParameterDialog_ReportNameSuffix; - public static String ParameterDialog_Error; - public static String ParameterDialog_ErrorMessage; + + public static String ErrorDialog_Info; + public static String ErrorDialog_InfoMessage; + public static String ErrorDialog_Error; + public static String ErrorDialog_ErrorMessage; + public static String AddAnalysisDialog_Name; public static String AddAnalysisDialog_Command; public static String AddAnalysisDialog_NameEmptyErrorMessage; diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/RunAnalysisHandler.java b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/RunAnalysisHandler.java index e99c7cd605..14d19edc4b 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/RunAnalysisHandler.java +++ b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/RunAnalysisHandler.java @@ -16,14 +16,15 @@ import java.util.List; import org.eclipse.core.commands.AbstractHandler; import org.eclipse.core.commands.ExecutionEvent; import org.eclipse.core.commands.ExecutionException; +import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.core.runtime.IStatus; import org.eclipse.core.runtime.NullProgressMonitor; import org.eclipse.core.runtime.Status; import org.eclipse.core.runtime.jobs.Job; import org.eclipse.jdt.annotation.Nullable; +import org.eclipse.jface.dialogs.ErrorDialog; import org.eclipse.jface.dialogs.IInputValidator; -import org.eclipse.jface.dialogs.MessageDialog; import org.eclipse.jface.viewers.ISelection; import org.eclipse.jface.viewers.IStructuredSelection; import org.eclipse.jface.window.Window; @@ -35,7 +36,6 @@ import org.eclipse.tracecompass.internal.provisional.analysis.lami.core.module.L import org.eclipse.tracecompass.internal.provisional.analysis.lami.ui.views.LamiReportViewFactory; import org.eclipse.tracecompass.tmf.core.analysis.ondemand.IOnDemandAnalysis; import org.eclipse.tracecompass.tmf.core.analysis.ondemand.IOnDemandAnalysisReport; -import org.eclipse.tracecompass.tmf.core.analysis.ondemand.OnDemandAnalysisException; import org.eclipse.tracecompass.tmf.core.timestamp.TmfTimeRange; import org.eclipse.tracecompass.tmf.core.trace.ITmfTrace; import org.eclipse.tracecompass.tmf.core.trace.TmfTraceManager; @@ -137,22 +137,37 @@ public class RunAnalysisHandler extends AbstractHandler { }); return Status.OK_STATUS; - } catch (OnDemandAnalysisException e) { - String errMsg = e.getMessage(); - - if (errMsg != null) { - /* The analysis execution yielded an error */ - Display.getDefault().asyncExec(() -> { - MessageDialog.openError(shell, - /* Dialog title */ - Messages.ParameterDialog_Error, - /* Dialog message */ - Messages.ParameterDialog_ErrorMessage + ":\n\n" + //$NON-NLS-1$ - errMsg); - }); + } catch (CoreException e) { + /* + * The analysis execution did not complete normally, we will + * report it to the user. + */ + IStatus status = e.getStatus(); + + /* Don't display a dialog if it was simply cancelled by the user */ + if (status.matches(IStatus.CANCEL)) { + return status; + } + + String dialogTitle; + String dialogMessage; + if (status.matches(IStatus.ERROR)) { + dialogTitle = Messages.ErrorDialog_Error; + dialogMessage = Messages.ErrorDialog_ErrorMessage; + } else { + dialogTitle = Messages.ErrorDialog_Info; + dialogMessage = Messages.ErrorDialog_InfoMessage; } - return Status.CANCEL_STATUS; + Display.getDefault().asyncExec(() -> { + ErrorDialog.openError(shell, dialogTitle, dialogMessage, status); + }); + + /* + * We showed our own error message, no need for the Job to + * show another one. + */ + return Status.OK_STATUS; } } }; diff --git a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/messages.properties b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/messages.properties index 2af7ae7195..94cdb8b6fe 100644 --- a/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/messages.properties +++ b/analysis/org.eclipse.tracecompass.analysis.lami.ui/src/org/eclipse/tracecompass/internal/provisional/analysis/lami/ui/handler/messages.properties @@ -14,8 +14,11 @@ ParameterDialog_ExternalParameters = External Analysis Parameters ParameterDialog_ExternalParametersDescription = Extra parameters to pass to the external analysis. Leave empty for none. ParameterDialog_StringValidatorMessage = Allowed characters are letters, numbers, ',' and '-' ParameterDialog_ReportNameSuffix = Report -ParameterDialog_Error = Error running external script -ParameterDialog_ErrorMessage = The script terminated abnormally + +ErrorDialog_Info = External script completed +ErrorDialog_InfoMessage = The script finished executing. +ErrorDialog_Error = Error running external script +ErrorDialog_ErrorMessage = The script terminated abnormally. AddAnalysisDialog_Title = Add External Analysis AddAnalysisDialog_Name = Name diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/IOnDemandAnalysis.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/IOnDemandAnalysis.java index 2d21f1d4e1..24643f4938 100644 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/IOnDemandAnalysis.java +++ b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/IOnDemandAnalysis.java @@ -9,6 +9,7 @@ package org.eclipse.tracecompass.tmf.core.analysis.ondemand; +import org.eclipse.core.runtime.CoreException; import org.eclipse.core.runtime.IProgressMonitor; import org.eclipse.tracecompass.tmf.core.timestamp.TmfTimeRange; import org.eclipse.tracecompass.tmf.core.trace.ITmfTrace; @@ -93,10 +94,10 @@ public interface IOnDemandAnalysis { * for a default monitor. * @return The results of this analysis. Exact object type is * analysis-dependent, a more specific return type is encouraged. - * @throws OnDemandAnalysisException + * @throws CoreException * If something went wrong with the execution, and expected * results will not be returned */ Object execute(ITmfTrace trace, TmfTimeRange range, String extraParams, - IProgressMonitor monitor) throws OnDemandAnalysisException; + IProgressMonitor monitor) throws CoreException; } diff --git a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/OnDemandAnalysisException.java b/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/OnDemandAnalysisException.java deleted file mode 100644 index 5abd3bb513..0000000000 --- a/tmf/org.eclipse.tracecompass.tmf.core/src/org/eclipse/tracecompass/tmf/core/analysis/ondemand/OnDemandAnalysisException.java +++ /dev/null @@ -1,43 +0,0 @@ -/******************************************************************************* - * Copyright (c) 2016 EfficiOS Inc., Alexandre Montplaisir - * - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the Eclipse Public License v1.0 - * which accompanies this distribution, and is available at - * http://www.eclipse.org/legal/epl-v10.html - *******************************************************************************/ - -package org.eclipse.tracecompass.tmf.core.analysis.ondemand; - -import org.eclipse.jdt.annotation.Nullable; - -/** - * Exceptions resulting from the execution of on-demand analyses. - * - * The suggested behavior is to display the reported messages to the end user, - * so they know why execution did not end normally. - * - * @author Alexandre Montplaisir - * @since 2.0 - */ -public class OnDemandAnalysisException extends Exception { - - private static final long serialVersionUID = 7296987172562152876L; - - /** - * Build a new exception. If the message is not null, it should be reported - * to the user. - * - * @param message - * The message to display, if any - */ - public OnDemandAnalysisException(@Nullable String message) { - super(message); - } - - @Override - public @Nullable String getMessage() { - return super.getMessage(); - } - -}