RFR: 7167: Agent Plugin [v6]

[Joshua Matsuoka]

On Wed, 2 Jun 2021 15:09:53 GMT, Henrik Dafgård <hdafgard at openjdk.org> wrote:

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

Yes, I missed these when externalizing the rest of the strings, fixed now

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

Fixed this to show an exception dialog, thanks

> 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?

Yes, it makes sense to show an error dialog here, I've fixed it accordingly

> 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?

Added, thanks

> 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?

I fixed the icons previously but missed the TODOs, fixed now

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

Done, thanks

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

Agreed, fixed

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

Opened a bug for this https://bugs.openjdk.java.net/browse/JMC-7298 , good call

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

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