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

IEP-1018 Automation of hints viewer #809

Merged
merged 15 commits into from
Sep 13, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ public class IDFCorePreferenceConstants
{
// CMake CCache preferences
public static final String CMAKE_CCACHE_STATUS = "cmakeCCacheStatus"; //$NON-NLS-1$
public static final String AUTOMATE_BUILD_HINTS_STATUS = "automateHintsStatus"; //$NON-NLS-1$
public static final boolean CMAKE_CCACHE_DEFAULT_STATUS = true;

public static final boolean AUTOMATE_BUILD_HINTS_DEFAULT_STATUS = true;
/**
* Returns the node in the preference in the given context.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
/*******************************************************************************
* Copyright 2022-2023 Espressif Systems (Shanghai) PTE LTD. All rights reserved.
* Use is subject to license terms.
*******************************************************************************/
package com.espressif.idf.core.build;

import java.util.ArrayList;
import java.util.List;
import java.util.regex.Pattern;

import org.eclipse.cdt.core.IConsoleParser;

import com.espressif.idf.core.resources.OpenDialogListenerSupport;
import com.espressif.idf.core.resources.PopupDialog;
import com.espressif.idf.core.util.StringUtil;

/**
* The EspIdfErrorParser class implements the IConsoleParser interface to parse console output for ESP-IDF errors and
* extract relevant hints. It processes each line of the console output and checks against a list of regular expression
* patterns to identify errors and their corresponding hints.
*
* This class maintains a list of regular expression hint pairs (ReHintPair) that are used to match and identify errors,
* and associate them with appropriate hints. It accumulates matched pairs in the 'allMatchesList' for further
* processing.
*
* The processLine method is responsible for processing each input line by iterating through the list of regular
* expression hint pairs. It uses regular expressions to determine if the line matches any error pattern. If a match is
* found, the corresponding hint is associated with the error message and added to the 'allMatchesList'.
*
* The shutdown method is used to trigger the completion of parsing. It notifies listeners registred in the UI plugin
* that the list of available hints has changed and providing the accumulated error hint pairs. The 'allMatchesList' is
* then cleared to prepare for the next parsing session.
*
*
* @author Denys Almazov ([email protected])
*
*/
public class EspIdfErrorParser implements IConsoleParser
{

private List<ReHintPair> reHintsList;
private List<ReHintPair> allMatchesList;

public EspIdfErrorParser(List<ReHintPair> reHintPairs)
{
this.reHintsList = reHintPairs;
this.allMatchesList = new ArrayList<>();
}
Comment on lines +43 to +47
Copy link

Choose a reason for hiding this comment

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

The constructor EspIdfErrorParser(List<ReHintPair> reHintPairs) does not check if the passed list reHintPairs is null. This can lead to a NullPointerException in the processLine method when it tries to iterate over reHintsList. It's a good practice to validate constructor parameters to avoid potential runtime exceptions.

-    public EspIdfErrorParser(List<ReHintPair> reHintPairs)
-    {
-        this.reHintsList = reHintPairs;
-        this.allMatchesList = new ArrayList<>();
-    }
+    public EspIdfErrorParser(List<ReHintPair> reHintPairs)
+    {
+        if (reHintPairs == null) {
+            throw new IllegalArgumentException("reHintPairs cannot be null");
+        }
+        this.reHintsList = reHintPairs;
+        this.allMatchesList = new ArrayList<>();
+    }

Comment on lines +44 to +47
Copy link

Choose a reason for hiding this comment

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

The constructor EspIdfErrorParser(List<ReHintPair> reHintPairs) does not check if the passed list reHintPairs is null. This could lead to a NullPointerException in the processLine method when it tries to stream over reHintsList. Consider adding a null check in the constructor.

+    if (reHintPairs == null) {
+        throw new IllegalArgumentException("reHintPairs cannot be null");
+    }

public boolean processLine(String paramString)
{
for (ReHintPair reHintEntry : reHintsList)
{
boolean isRegexMatchesWithProcessedString = Pattern.compile(reHintEntry.getRe()).matcher(paramString)
.find();
if (isRegexMatchesWithProcessedString)
{
allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
return true;
}
}
Copy link

Choose a reason for hiding this comment

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

The regular expression pattern is compiled for every line processed, which can be inefficient if the number of lines or patterns is large. Consider compiling the patterns once during initialization and reusing them.

-    public boolean processLine(String paramString)
-    {
-        for (ReHintPair reHintEntry : reHintsList)
-        {
-            boolean isRegexMatchesWithProcessedString = Pattern.compile(reHintEntry.getRe()).matcher(paramString)
-                    .find();
-            if (isRegexMatchesWithProcessedString)
-            {
-                allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
-                return true;
-            }
-        }
-        return false;
-    }
+    public boolean processLine(String paramString)
+    {
+        for (ReHintPair reHintEntry : reHintsList)
+        {
+            boolean isRegexMatchesWithProcessedString = reHintEntry.getPattern().matcher(paramString).find();
+            if (isRegexMatchesWithProcessedString)
+            {
+                allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
+                return true;
+            }
+        }
+        return false;
+    }

In the above diff, I'm assuming that you will modify the ReHintPair class to store a Pattern object instead of a String for the regex. You would compile the pattern when creating the ReHintPair instance.

return false;
}

public void shutdown()
{
OpenDialogListenerSupport.getSupport().firePropertyChange(PopupDialog.AVAILABLE_HINTS.name(),
StringUtil.EMPTY, new ArrayList<>(allMatchesList));
allMatchesList.clear();
}
sigmaaa marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
import com.espressif.idf.core.internal.CMakeErrorParser;
import com.espressif.idf.core.logging.Logger;
import com.espressif.idf.core.util.DfuCommandsUtil;
import com.espressif.idf.core.util.HintsUtil;
import com.espressif.idf.core.util.IDFUtil;
import com.espressif.idf.core.util.ParitionSizeHandler;
import com.espressif.idf.core.util.StringUtil;
Expand Down Expand Up @@ -419,8 +420,14 @@ private void runCmakeBuildCommand(IConsole console, IProgressMonitor monitor, IP
console.getErrorStream().write(String.format(Messages.CMakeBuildConfiguration_Failure, "")); //$NON-NLS-1$
throw new CmakeBuildException();
}

watchProcess(p, new IConsoleParser[] { epm, new StatusParser() });
boolean buildHintsStatus = Platform.getPreferencesService().getBoolean(IDFCorePlugin.PLUGIN_ID,
IDFCorePreferenceConstants.AUTOMATE_BUILD_HINTS_STATUS,
IDFCorePreferenceConstants.AUTOMATE_BUILD_HINTS_DEFAULT_STATUS, null);
IConsoleParser[] consoleParsers = buildHintsStatus
? new IConsoleParser[] { epm, new StatusParser(),
new EspIdfErrorParser(HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath()))) }
: new IConsoleParser[] { epm, new StatusParser() };
sigmaaa marked this conversation as resolved.
Show resolved Hide resolved
watchProcess(p, consoleParsers);

final String isSkip = System.getProperty("skip.idf.components"); //$NON-NLS-1$
if (!Boolean.parseBoolean(isSkip))
Comment on lines 420 to 433
Copy link

Choose a reason for hiding this comment

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

The code now checks for a preference setting to decide whether to use the EspIdfErrorParser or not. This is a good approach as it allows users to toggle the feature on/off. However, there seems to be no error handling if HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath())) fails (e.g., file not found, parsing error). Consider adding appropriate error handling here.

- new EspIdfErrorParser(HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath())))
+ List<Hint> hints = Collections.emptyList();
+ try {
+     hints = HintsUtil.getReHintsList(new File(HintsUtil.getHintsYmlPath()));
+ } catch (Exception e) {
+     Logger.log(e);
+ }
+ new EspIdfErrorParser(hints)

Expand Down Expand Up @@ -502,6 +509,7 @@ protected int watchProcess(Process process, IConsoleParser[] consoleParsers) thr
{
Thread.sleep(100);
}
Stream.of(consoleParsers).forEach(IConsoleParser::shutdown);
return rc;
}
catch (InterruptedException e)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.espressif.idf.core.resources;

public enum PopupDialog
{
LOW_PARTITION_SIZE, AVAILABLE_HINTS
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import org.eclipse.core.runtime.Path;

import com.espressif.idf.core.resources.OpenDialogListenerSupport;
import com.espressif.idf.core.resources.PopupDialog;

public class ParitionSizeHandler
{
Expand Down Expand Up @@ -56,17 +57,15 @@ private String getPartitionTable() throws IOException, CoreException
+ "partition-table.bin"); //$NON-NLS-1$

Process process = startProcess(commands);
String partitionTableContent = new String(process.getInputStream().readAllBytes());
return partitionTableContent;
return new String(process.getInputStream().readAllBytes());
}
sigmaaa marked this conversation as resolved.
Show resolved Hide resolved

private Process startProcess(List<String> commands) throws IOException
{
infoStream.write(String.join(" ", commands) + '\n'); //$NON-NLS-1$ //$NON-NLS-2$
infoStream.write(String.join(" ", commands) + '\n'); //$NON-NLS-1$
Path workingDir = (Path) project.getLocation();
ProcessBuilder processBuilder = new ProcessBuilder(commands).directory(workingDir.toFile());
Process process = processBuilder.start();
return process;
return processBuilder.start();
}

private void startIdfSizeProcess() throws IOException, CoreException
Expand Down Expand Up @@ -100,7 +99,8 @@ private void checkRemainingSize(IPath path) throws IOException, CoreException
double remainSize = (maxSize - imageSize) / (maxSize);
if (remainSize < 0.3)
{
OpenDialogListenerSupport.getSupport().firePropertyChange(null, maxSize, remainSize * maxSize);
OpenDialogListenerSupport.getSupport().firePropertyChange(PopupDialog.LOW_PARTITION_SIZE.name(),
maxSize, remainSize * maxSize);
break;
}
}
Expand Down
3 changes: 2 additions & 1 deletion bundles/com.espressif.idf.ui/OSGI-INF/l10n/bundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,5 @@ command.label.PartitionTableEditor = ESP-IDF: Partition Table Editor
command.name.PartitionTableEditor = ESP-IDF: Partition Table Editor
command.label.nsvTableEditor = ESP-IDF: NVS Table Editor
command.name.nvsTableEditor = ESP-IDF: NVS Table Editor
command.tooltip.nvsTableEditor = NVS Editor can help you to easily edit NVS CSV, generate encrypted and non-encrypted partitions through GUI, without interacting directly with the csv files.
command.tooltip.nvsTableEditor = NVS Editor can help you to easily edit NVS CSV, generate encrypted and non-encrypted partitions through GUI, without interacting directly with the csv files.
build_hints.name = Build Hints
9 changes: 9 additions & 0 deletions bundles/com.espressif.idf.ui/plugin.xml
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,15 @@
id="com.espressif.idf.ui.category"
name="%views_category.name">
</category>
<view
allowMultiple="false"
category="com.espressif.idf.ui.category"
class="com.espressif.idf.ui.dialogs.BuildView"
icon="icons/Hints_View.png"
id="com.espressif.idf.ui.views.buildhints"
name="%build_hints.name"
restorable="true">
</view>
</extension>
<extension
point="org.eclipse.ui.intro">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
package com.espressif.idf.ui;

import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
import java.io.File;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.net.URL;
import java.text.MessageFormat;
import java.util.List;

import org.eclipse.cdt.cmake.core.internal.Activator;
import org.eclipse.core.resources.ResourcesPlugin;
Expand All @@ -25,6 +25,8 @@
import org.eclipse.swt.widgets.MessageBox;
import org.eclipse.swt.widgets.Shell;
import org.eclipse.ui.IStartup;
import org.eclipse.ui.PartInitException;
import org.eclipse.ui.PlatformUI;
import org.json.simple.JSONObject;
import org.json.simple.parser.JSONParser;
import org.json.simple.parser.ParseException;
Expand All @@ -33,11 +35,14 @@

import com.espressif.idf.core.IDFEnvironmentVariables;
import com.espressif.idf.core.build.Messages;
import com.espressif.idf.core.build.ReHintPair;
import com.espressif.idf.core.logging.Logger;
import com.espressif.idf.core.resources.OpenDialogListenerSupport;
import com.espressif.idf.core.resources.PopupDialog;
import com.espressif.idf.core.resources.ResourceChangeListener;
import com.espressif.idf.core.toolchain.ESPToolChainManager;
import com.espressif.idf.core.util.StringUtil;
import com.espressif.idf.ui.dialogs.BuildView;
import com.espressif.idf.ui.dialogs.MessageLinkDialog;
import com.espressif.idf.ui.update.ExportIDFTools;
import com.espressif.idf.ui.update.InstallToolsHandler;
Expand All @@ -46,6 +51,8 @@
public class InitializeToolsStartup implements IStartup
{

private static final String BUILDHINTS_ID = "com.espressif.idf.ui.views.buildhints"; //$NON-NLS-1$

Comment on lines +54 to +55
Copy link

Choose a reason for hiding this comment

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

The constant BUILDHINTS_ID is defined but there are no comments explaining its purpose. It would be beneficial to add a comment describing what this constant represents in the context of your application.

/**
* esp-idf.json is file created by the installer
*/
Expand All @@ -65,24 +72,18 @@ public class InitializeToolsStartup implements IStartup
@Override
public void earlyStartup()
{
OpenDialogListenerSupport.getSupport().addPropertyChangeListener(new PropertyChangeListener()
{

@Override
public void propertyChange(PropertyChangeEvent evt)
OpenDialogListenerSupport.getSupport().addPropertyChangeListener(evt -> {
PopupDialog popupDialog = PopupDialog.valueOf(evt.getPropertyName());
switch (popupDialog)
{
Display.getDefault().asyncExec(new Runnable()
{
@Override
public void run()
{
MessageLinkDialog.openWarning(Display.getDefault().getActiveShell(),
Messages.IncreasePartitionSizeTitle,
MessageFormat.format(Messages.IncreasePartitionSizeMessage, evt.getNewValue(),
evt.getOldValue(), DOC_URL));
}
});

case LOW_PARTITION_SIZE:
openLowPartitionSizeDialog(evt);
break;
case AVAILABLE_HINTS:
openAvailableHintsDialog(evt);
break;
default:
break;
}
});
ILaunchBarListener launchBarListener = new LaunchBarListener();
Expand Down Expand Up @@ -197,6 +198,52 @@ else if (isInstallerConfigSet())
Logger.log(e);
}
}

@SuppressWarnings("unchecked")
private void openAvailableHintsDialog(PropertyChangeEvent evt)
{
Display.getDefault().asyncExec(() ->
{
List<ReHintPair> erroHintPairs = (List<ReHintPair>) evt.getNewValue();
// if list is empty we don't want to change focus from the console output
if (erroHintPairs.isEmpty())
{
updateValuesInBuildView(erroHintPairs);
return;
}
try
{
PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage()
.showView(BUILDHINTS_ID);
}
catch (PartInitException e)
{
Comment on lines +219 to +220
Copy link

Choose a reason for hiding this comment

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

Exception logging is good, but it might be more helpful to provide some context or additional information about what operation was being attempted when the exception occurred. This can make debugging easier if an error occurs.

Logger.log(e);
}
updateValuesInBuildView(erroHintPairs);
}
);

}
Comment on lines +202 to +227
Copy link

Choose a reason for hiding this comment

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

This method opens a dialog with available hints. However, it seems like it's doing more than just opening a dialog. It's also updating the values in the BuildView. Consider renaming this method to better reflect its functionality or splitting it into two methods: one for opening the dialog and another for updating the BuildView.


private void updateValuesInBuildView(List<ReHintPair> erroHintPairs)
{
BuildView view = ((BuildView) PlatformUI.getWorkbench().getActiveWorkbenchWindow().getActivePage()
.findView(BUILDHINTS_ID));
if (view != null)
{
view.updateReHintsPairs(erroHintPairs);
}
}
Comment on lines +229 to +237
Copy link

Choose a reason for hiding this comment

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

This method updates the values in the BuildView. However, it doesn't check whether the erroHintPairs parameter is null before using it. This could lead to a NullPointerException if the method is called with a null argument. Consider adding a null check at the beginning of this method.

+    if (erroHintPairs == null) {
+        throw new IllegalArgumentException("erroHintPairs cannot be null");
+    }


private void openLowPartitionSizeDialog(PropertyChangeEvent evt)
{
Display.getDefault().asyncExec(() ->
MessageLinkDialog.openWarning(Display.getDefault().getActiveShell(),
Messages.IncreasePartitionSizeTitle, MessageFormat.format(Messages.IncreasePartitionSizeMessage,
evt.getNewValue(), evt.getOldValue(), DOC_URL))
);
}

@SuppressWarnings("unchecked")
private void updateEspIdfJsonFile(File idf_json_file, String newIdfPathToUpdate)
Expand Down
Loading
Loading