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,82 @@
/*******************************************************************************
* 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 java.util.regex.PatternSyntaxException;

import org.eclipse.cdt.core.IConsoleParser;

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.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;

Check warning on line 48 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/EspIdfErrorParser.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP2

new com.espressif.idf.core.build.EspIdfErrorParser(List) may expose internal representation by storing an externally mutable object into EspIdfErrorParser.reHintsList
Raw output
This code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.
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 = false;
try
{
isRegexMatchesWithProcessedString = Pattern.compile(reHintEntry.getRe()).matcher(paramString).find();
}
catch (PatternSyntaxException e)
{
// catching incompatible Python regex
Logger.log(e);
continue;
}

if (isRegexMatchesWithProcessedString)
{
allMatchesList.add(new ReHintPair(paramString, reHintEntry.getHint()));
return true;
}
}
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
}
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 @@ -156,7 +157,7 @@
ICMakeToolChainFile toolChainFile, String launchMode)
{
super(config, name, toolChain, launchMode);
this.toolChainFile = toolChainFile;

Check warning on line 160 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP2

new com.espressif.idf.core.build.IDFBuildConfiguration(IBuildConfiguration, String, IToolChain, ICMakeToolChainFile, String) may expose internal representation by storing an externally mutable object into IDFBuildConfiguration.toolChainFile
Raw output
This code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.
}

@Override
Expand Down Expand Up @@ -189,7 +190,7 @@
org.eclipse.core.runtime.Path path = new org.eclipse.core.runtime.Path(customBuildDir);
if (!path.toFile().exists())
{
path.toFile().mkdirs();

Check warning on line 193 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Exceptional return value of java.io.File.mkdirs() ignored in com.espressif.idf.core.build.IDFBuildConfiguration.getBuildContainerPath()
Raw output
This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.
}
return path;
}
Expand Down Expand Up @@ -293,7 +294,7 @@
public ICMakeToolChainFile getToolChainFile() throws CoreException
{
ICMakeToolChainManager manager = IDFCorePlugin.getService(ICMakeToolChainManager.class);
this.toolChainFile = manager.getToolChainFileFor(getToolChain());

Check warning on line 297 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP

com.espressif.idf.core.build.IDFBuildConfiguration.getToolChainFile() may expose internal representation by returning IDFBuildConfiguration.toolChainFile
Raw output
Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.
return toolChainFile;
}

Expand All @@ -308,7 +309,7 @@
@Override
public IProject[] build(int kind, Map<String, String> args, IConsole console, IProgressMonitor monitor)
throws CoreException
{

Check warning on line 312 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP2

com.espressif.idf.core.build.IDFBuildConfiguration.build(int, Map, IConsole, IProgressMonitor) may expose internal representation by storing an externally mutable object into IDFBuildConfiguration.monitor
Raw output
This code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.
this.monitor = monitor;
isProgressSet = false;

Expand All @@ -329,7 +330,7 @@
// create build directory
Path buildDir = getBuildDirectory();
if (!buildDir.toFile().exists())
{

Check warning on line 333 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Exceptional return value of java.io.File.mkdir() ignored in com.espressif.idf.core.build.IDFBuildConfiguration.build(int, Map, IConsole, IProgressMonitor)
Raw output
This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.
buildDir.toFile().mkdir();
}

Expand Down Expand Up @@ -419,8 +420,14 @@
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 @@
{
Thread.sleep(100);
}
Stream.of(consoleParsers).forEach(IConsoleParser::shutdown);
return rc;
}
catch (InterruptedException e)
Expand All @@ -520,15 +528,15 @@

public ReaderThread(CBuildConfiguration config, InputStream in, IConsoleParser[] consoleParsers)
{
this.config = config;

Check warning on line 531 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in new com.espressif.idf.core.build.IDFBuildConfiguration$ReaderThread(CBuildConfiguration, InputStream, IConsoleParser[]): new java.io.InputStreamReader(InputStream)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
this.in = new BufferedReader(new InputStreamReader(in));
this.out = null;
this.consoleParsers = consoleParsers;
}

public ReaderThread(InputStream in, OutputStream out)
{

Check warning on line 538 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in new com.espressif.idf.core.build.IDFBuildConfiguration$ReaderThread(InputStream, OutputStream): new java.io.InputStreamReader(InputStream)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
this.in = new BufferedReader(new InputStreamReader(in));

Check warning on line 539 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in new com.espressif.idf.core.build.IDFBuildConfiguration$ReaderThread(InputStream, OutputStream): new java.io.PrintStream(OutputStream)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
this.out = new PrintStream(out);
this.consoleParsers = null;
this.config = null;
Expand Down Expand Up @@ -778,7 +786,7 @@
File jsonDiskFile = jsonIPath.toFile();

IFolder folder = getIDFComponentsFolder();
CommandEntry[] sourceFileInfos = null;

Check warning on line 789 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.core.build.IDFBuildConfiguration.linkBuildComponents(): new java.io.FileReader(File)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
try (Reader in = new FileReader(jsonDiskFile))
{
Gson gson = new Gson();
Expand Down Expand Up @@ -816,7 +824,7 @@
IFolder folder = project.getFolder(getComponentsPath());
File file = folder.getLocation().toFile();
if (!file.exists())
{

Check warning on line 827 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/build/IDFBuildConfiguration.java

View workflow job for this annotation

GitHub Actions / spotbugs

RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Exceptional return value of java.io.File.mkdirs() ignored in com.espressif.idf.core.build.IDFBuildConfiguration.getIDFComponentsFolder()
Raw output
This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.
folder.getLocation().toFile().mkdirs();
IProgressMonitor monitor = new NullProgressMonitor();
folder.getProject().refreshLocal(IResource.DEPTH_INFINITE, monitor);
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 All @@ -26,8 +27,8 @@

public ParitionSizeHandler(IProject project, ConsoleOutputStream infoStream, IConsole console)
{
this.project = project;

Check warning on line 30 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP2

new com.espressif.idf.core.util.ParitionSizeHandler(IProject, ConsoleOutputStream, IConsole) may expose internal representation by storing an externally mutable object into ParitionSizeHandler.project
Raw output
This code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.
this.infoStream = infoStream;

Check warning on line 31 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java

View workflow job for this annotation

GitHub Actions / spotbugs

EI_EXPOSE_REP2

new com.espressif.idf.core.util.ParitionSizeHandler(IProject, ConsoleOutputStream, IConsole) may expose internal representation by storing an externally mutable object into ParitionSizeHandler.infoStream
Raw output
This code stores a reference to an externally mutable object into the internal representation of the object.  If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.
this.console = console;
}

Expand Down Expand Up @@ -56,17 +57,15 @@
+ "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());

Check warning on line 60 in bundles/com.espressif.idf.core/src/com/espressif/idf/core/util/ParitionSizeHandler.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.core.util.ParitionSizeHandler.getPartitionTable(): new String(byte[])
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
}
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 @@
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 @@
@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 @@ -141,7 +142,7 @@
JSONParser parser = new JSONParser();
try
{
JSONObject jsonObj = (JSONObject) parser.parse(new FileReader(idf_json_file));

Check warning on line 145 in bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.ui.InitializeToolsStartup.earlyStartup(): new java.io.FileReader(File)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

Check warning on line 145 in bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java

View workflow job for this annotation

GitHub Actions / spotbugs

OBL_UNSATISFIED_OBLIGATION

com.espressif.idf.ui.InitializeToolsStartup.earlyStartup() may fail to clean up java.io.Reader
Raw output
This method may fail to clean up (close, dispose of) a stream, database object, or other resource requiring an explicit cleanup operation.

In general, if a method opens a stream or other resource, the method should use a try/finally block to ensure that the stream or resource is cleaned up before the method returns.

This bug pattern is essentially the same as the OS_OPEN_STREAM and ODR_OPEN_DATABASE_RESOURCE bug patterns, but is based on a different (and hopefully better) static analysis technique. We are interested is getting feedback about the usefulness of this bug pattern. For sending feedback, check:

 * contributing guideline [https://github.com/spotbugs/spotbugs/blob/master/.github/CONTRIBUTING.md]
 * mailinglist [https://github.com/spotbugs/discuss/issues?q=]

In particular, the false-positive suppression heuristics for this bug pattern have not been extensively tuned, so reports about false positives are helpful to us.

See Weimer and Necula, Finding and Preventing Run-Time Error Handling Mistakes (PDF [https://people.eecs.berkeley.edu/~necula/Papers/rte_oopsla04.pdf]), for a description of the analysis technique.
String gitExecutablePath = (String) jsonObj.get(GIT_PATH);
String idfVersionId = (String) jsonObj.get(IDF_VERSIONS_ID);
JSONObject list = (JSONObject) jsonObj.get(IDF_INSTALLED_LIST_KEY);
Expand Down Expand Up @@ -197,13 +198,59 @@
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)
{
JSONParser parser = new JSONParser();
JSONObject jsonObj = null;
try (FileReader reader = new FileReader(idf_json_file))

Check warning on line 253 in bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.ui.InitializeToolsStartup.updateEspIdfJsonFile(File, String): new java.io.FileReader(File)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
{
jsonObj = (JSONObject) parser.parse(reader);
String idfVersionId = (String) jsonObj.get(IDF_VERSIONS_ID);
Expand All @@ -227,7 +274,7 @@

if (jsonObj != null)
{
try (FileWriter fileWriter = new FileWriter(idf_json_file))

Check warning on line 277 in bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.ui.InitializeToolsStartup.updateEspIdfJsonFile(File, String): new java.io.FileWriter(File)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
{
fileWriter.write(jsonObj.toJSONString());
fileWriter.flush();
Expand All @@ -245,7 +292,7 @@
{
// read esp-idf.json file
JSONParser parser = new JSONParser();
try (FileReader reader = new FileReader(idf_json_file))

Check warning on line 295 in bundles/com.espressif.idf.ui/src/com/espressif/idf/ui/InitializeToolsStartup.java

View workflow job for this annotation

GitHub Actions / spotbugs

DM_DEFAULT_ENCODING

Found reliance on default encoding in com.espressif.idf.ui.InitializeToolsStartup.checkForUpdatedVersion(File): new java.io.FileReader(File)
Raw output
Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.
{
JSONObject jsonObj = (JSONObject) parser.parse(reader);
String idfVersionId = (String) jsonObj.get(IDF_VERSIONS_ID);
Expand Down
Loading
Loading