RFR: 7167: Agent Plugin [v6]

[Henrik Dafgård]

On Wed, 28 Apr 2021 17:57:17 GMT, Joshua Matsuoka <jmatsuoka at openjdk.org> wrote:

>> This PR adds the Agent Plugin to JMC. This is a plugin for controlling the jmc agent and provides a way to attach it and add probes to any JVMs in the JVM browser, as well as providing a preset manager for creating/modifying probe presets.
>> 
>> There is still a small amount of cleanup to be done so I'll mark it as a draft for now but it would be great to get a review :)
>
> Joshua Matsuoka has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix rcp feature to use new agent plugin name

I'd like to see another pass at which strings should be externalized and which shouldn't, there are a few that look like they need non-nls comments. There are also a few too many TODOs and references that should be tracked in JIRA with issue numbers in the code.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/editor/AgentEditorAction.java line 44:

> 42: 	private static final String MESSAGE_REFRESH = "Refresh";
> 43: 	private static final String MESSAGE_LOAD_PRESET = "Load a preset...";
> 44: 	private static final String MESSAGE_SAVE_AS_PRESET = "Save as a preset...";

These look like they should be localized.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/editor/AgentEditorUi.java line 136:

> 134: 				} catch (IOException | SAXException e) {
> 135: 					// TODO: display error dialog
> 136: 					e.printStackTrace();

This should be fixed before integration.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/editor/AgentEditorUi.java line 184:

> 182: 			presetRepository.addPreset(preset);
> 183: 		} catch (IOException | SAXException e) {
> 184: 			e.printStackTrace();

Do we want to log or display an error dialog here?

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/editor/AgentEditorUi.java line 255:

> 253: 
> 254: 							IPreset preset = (IPreset) element;
> 255: 							return preset.getFileName() + " - " + preset.getEvents().length + " " + MESSAGE_EVENTS;

Non-nls here?

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/editor/AgentEditorUi.java line 260:

> 258: 						@Override
> 259: 						public Image getImage(Object element) {
> 260: 							return AgentPlugin.getDefault().getImage(AgentPlugin.ICON_AGENT); // TODO: replace the icon in the future

This seems like it can be tracked by an issue in JIRA?

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/model/Preset.java line 516:

> 514: 		} catch (IOException e) {
> 515: 			// TODO: log exception
> 516: 			return false;

This should also be done before integration.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/manager/wizards/PresetManagerPage.java line 117:

> 115: 					@Override
> 116: 					public Image getImage(Object element) {
> 117: 						return AgentPlugin.getDefault().getImage(AgentPlugin.ICON_AGENT); // TODO: replace the icon in the future

Same as the other icon, we should track this with an issue in JIRA.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/utils/ValidationResult.java line 47:

> 45: 	private SAXParseException fatalError;
> 46: 
> 47: 	/* package-private */ ValidationResult(ArrayList<SAXParseException> warnings, ArrayList<SAXParseException> errors,

This isn't really a pattern we've used in JMC so it's a bit odd seeing it only here.

application/org.openjdk.jmc.console.agent/src/main/java/org/openjdk/jmc/console/agent/wizards/BaseWizardPage.java line 116:

> 114: 		Text text = new Text(parent, SWT.BORDER | SWT.MULTI | SWT.V_SCROLL);
> 115: 		// FIXME: Multi line Text field (SWT.MULTI) does not support Text.setMessage
> 116: 		// https://bugs.eclipse.org/bugs/show_bug.cgi?id=328832

We should have a JIRA issue tracking the Eclipse bug.

-------------

PR: https://git.openjdk.java.net/jmc/pull/226