Auto-complete for build.properties: Image Icons #531

alshamams · 2023-03-24T04:59:19Z

Refs: #508

This PR addresses one of the pending items in the referenced PR. The auto-complete feature attaches most relevant image icons to the properties listing, so that the user is able to associate meaning of the property intuitively.

The work is incomplete. I have taken some assumptions here, and would like to get some reviews before proceeding further.

Classified the headers of the build.properties specification
Identified and attached the most meaningful existing images
Provided a generic image for the default case(when the work completes I expect only very few items under this category.)

Please let me know what you think of the general approach.

github-actions · 2023-03-24T05:24:11Z

Test Results

    240 files ±0     240 suites ±0 56m 51s ⏱️ + 7m 57s
  3 278 tests ±0   3 252 ✔️ - 1 24 💤 ±0 0 ❌ ±0 2 🔥 +1
10 131 runs ±0 10 057 ✔️ - 1 72 💤 ±0 0 ❌ ±0 2 🔥 +1

For more details on these errors, see this check.

Results for commit 854f8d3. ± Comparison against base commit b1a4e1c.

♻️ This comment has been updated with latest results.

alshamams · 2023-03-28T18:47:06Z

Since I haven't heard any reviews pertaining to the previous commit, I am assuming that this is a reasonable approach and has further added images for all the features in build.properties.

HannesWell

Yes this is a reasonable approach, thank you for enhancing the content assist.
So is this PR complete from your POV and ready for review?

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

laeubi · 2023-04-04T10:24:31Z

@HannesWell @alshamams any chance to finish this as of today?

alshamams · 2023-04-04T15:03:22Z

@HannesWell @alshamams any chance to finish this as of today?

@laeubi All outstanding review comments have been addressed now.

HannesWell

I haven't checked each image but in general it works. Just have one remark regarding the swtich statement.
Furthermore can you please format the class. There seem to be some miss-formatting.

In general the content-assist should be improved to provide more meaning full results and descriptions for each entry. At the moment it looks like in the following picture and I'm not sure if each entry is really useful:

But that's something for another PR. :)

HannesWell · 2023-04-04T22:36:59Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+public Image getImage(String element) {
+	ImageDescriptor desc = null;
+	switch (element)
+		{
+		case "src.includes", "src.excludes", "src.additionalRoots", "permissions", "root.", "root", ".permissions.", ".link", "folder.", ".folder.", "link", "source.", "sourceFileExtensions", "bin.includes", "bin.excludes", "javacCustomEncodings.", "javacDefaultEncoding." -> desc = PDEPluginImages.DESC_CATEGORY_OBJ;  //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$//$NON-NLS-9$ //$NON-NLS-10$ //$NON-NLS-11$ //$NON-NLS-12$ //$NON-NLS-13$ //$NON-NLS-14$ //$NON-NLS-15$ //$NON-NLS-16$ //$NON-NLS-17$
+		case ".jar", "jars.compile.order", "jars.extra.classpath" -> desc = PDEPluginImages.DESC_JAR_LIB_OBJ; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+		case "javacProjectSettings" -> desc = PDEPluginImages.DESC_SETTINGS_OBJ; //$NON-NLS-1$
+		case "javacErrors." -> desc = PDEPluginImages.DESC_ERROR_ST_OBJ; //$NON-NLS-1$
+		case "significantVersionDigits", "generatedVersionLength" -> desc = PDEPluginImages.DESC_INFO_ST_OBJ; //$NON-NLS-1$ //$NON-NLS-2$
+		case "javacWarnings." -> desc = PDEPluginImages.DESC_ALERT_OBJ; //$NON-NLS-1$
+		case "jre.compilation.profile" -> desc = PDEPluginImages.DESC_TARGET_ENVIRONMENT; //$NON-NLS-1$
+		case "manifest." -> desc = PDEPluginImages.DESC_FOLDER_OBJ; //$NON-NLS-1$
+		default -> desc = PDEPluginImages.DESC_DEFAULT_OBJ;
+
+			}
+		if (desc != null) {
+			return desc.createImage();
 		}
 		return null;


You can use the switch itself as an expression instead of assigning the value of the desc variable in each case:

Suggested change

public Image getImage(String element) {

ImageDescriptor desc = null;

switch (element)

{

case "src.includes", "src.excludes", "src.additionalRoots", "permissions", "root.", "root", ".permissions.", ".link", "folder.", ".folder.", "link", "source.", "sourceFileExtensions", "bin.includes", "bin.excludes", "javacCustomEncodings.", "javacDefaultEncoding." -> desc = PDEPluginImages.DESC_CATEGORY_OBJ; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ //$NON-NLS-6$ //$NON-NLS-7$ //$NON-NLS-8$//$NON-NLS-9$ //$NON-NLS-10$ //$NON-NLS-11$ //$NON-NLS-12$ //$NON-NLS-13$ //$NON-NLS-14$ //$NON-NLS-15$ //$NON-NLS-16$ //$NON-NLS-17$

case ".jar", "jars.compile.order", "jars.extra.classpath" -> desc = PDEPluginImages.DESC_JAR_LIB_OBJ; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$

case "javacProjectSettings" -> desc = PDEPluginImages.DESC_SETTINGS_OBJ; //$NON-NLS-1$

case "javacErrors." -> desc = PDEPluginImages.DESC_ERROR_ST_OBJ; //$NON-NLS-1$

case "significantVersionDigits", "generatedVersionLength" -> desc = PDEPluginImages.DESC_INFO_ST_OBJ; //$NON-NLS-1$ //$NON-NLS-2$

case "javacWarnings." -> desc = PDEPluginImages.DESC_ALERT_OBJ; //$NON-NLS-1$

case "jre.compilation.profile" -> desc = PDEPluginImages.DESC_TARGET_ENVIRONMENT; //$NON-NLS-1$

case "manifest." -> desc = PDEPluginImages.DESC_FOLDER_OBJ; //$NON-NLS-1$

default -> desc = PDEPluginImages.DESC_DEFAULT_OBJ;

}

if (desc != null) {

return desc.createImage();

}

return null;

public Image getImage(String element) {

@SuppressWarnings("nls")

ImageDescriptor desc = switch (element)

{

case "src.includes", "src.excludes", "src.additionalRoots", "permissions", "root.", "root", ".permissions.", ".link", "folder.", ".folder.", //

"link", "source.", "sourceFileExtensions", "bin.includes", "bin.excludes", "javacCustomEncodings.", "javacDefaultEncoding." -> PDEPluginImages.DESC_CATEGORY_OBJ;

case ".jar", "jars.compile.order", "jars.extra.classpath" -> PDEPluginImages.DESC_JAR_LIB_OBJ;

case "javacProjectSettings" -> PDEPluginImages.DESC_SETTINGS_OBJ;

case "javacErrors." -> PDEPluginImages.DESC_ERROR_ST_OBJ;

case "significantVersionDigits", "generatedVersionLength" -> PDEPluginImages.DESC_INFO_ST_OBJ;

case "javacWarnings." -> PDEPluginImages.DESC_ALERT_OBJ;

case "jre.compilation.profile" -> PDEPluginImages.DESC_TARGET_ENVIRONMENT;

case "manifest." -> PDEPluginImages.DESC_FOLDER_OBJ;

default -> PDEPluginImages.DESC_DEFAULT_OBJ;

};

return desc.createImage();

}

Furthermore in this case I would simply suppress the nls warnings instead of adding $NON-NLS-1$ comments up to level 17 and split the line manually to make it readable.

@HannesWell
I have modified the switch case and suppressed the nls warnings.
After splitting the lines manually though, it reverts back to the original form upon formatting.
Do let me know in case of any more edits.

vogella · 2023-04-14T13:37:45Z

@alshamams could you update the PR with @HannesWell suggestion?

alshamams · 2023-04-17T05:30:22Z

@HannesWell @vogella I have been on holiday, and out sick lately. I will look into it, and will update the PR accordingly at the earliest.

alshamams · 2023-04-18T10:03:38Z

I haven't checked each image but in general it works. Just have one remark regarding the swtich statement. Furthermore can you please format the class. There seem to be some miss-formatting.

In general the content-assist should be improved to provide more meaning full results and descriptions for each entry. At the moment it looks like in the following picture and I'm not sure if each entry is really useful:

But that's something for another PR. :)

@HannesWell
I actually wanted to attach the documented description from the javadoc, which provides full meaning to each of the icons in the list above in the form of help text.
Unfortunately that’s not working, so I’m planning to do this in a follow up PR.

gireeshpunathil

left few non-blocking comments, otherwise LGTM.

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

gireeshpunathil · 2023-04-19T05:49:57Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+			case "jre.compilation.profile" -> PDEPluginImages.DESC_TARGET_ENVIRONMENT;
+			case "manifest." -> PDEPluginImages.DESC_FOLDER_OBJ;
+			default -> PDEPluginImages.DESC_DEFAULT_OBJ;
+


gireeshpunathil · 2023-04-19T05:51:52Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+		ImageDescriptor desc = null;
+		desc = switch (element)
+			{
+			case "src.includes", "src.excludes", "src.additionalRoots", "permissions", "root.", "root", ".permissions.", ".link", "folder.", ".folder.", "link", "source.", "sourceFileExtensions", "bin.includes", "bin.excludes", "javacCustomEncodings.", "javacDefaultEncoding." -> PDEPluginImages.DESC_CATEGORY_OBJ;


is this the result of formatting? if so, ignoring this line alone from formatting and manually breaking the line into 2 or 3 looks like a better thing to do?

Yes, I think that would be best. Will do so. @gireeshpunathil

Yes that line is a bit long. I suggest to put comments // at the end of the manually broken line to prevent the formatter to merge them again.

HannesWell

Thanks, the change looks good.
Just two additional non-blocking remarks about the code style.

This change should be squash-merged and the resulting message should be cleaned up.

HannesWell · 2023-04-20T05:53:46Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+		ImageDescriptor desc = null;
+		desc = switch (element)


Suggested change

ImageDescriptor desc = null;

desc = switch (element)

ImageDescriptor desc = switch (element)

HannesWell · 2023-04-20T05:54:26Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+		if (desc != null) {
+			return desc.createImage();
+		}
+		return null;


Suggested change

if (desc != null) {

return desc.createImage();

}

return null;

return desc != null ? desc.createImage() : null;

HannesWell · 2023-04-20T05:55:32Z

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

+		ImageDescriptor desc = null;
+		desc = switch (element)
+			{
+			case "src.includes", "src.excludes", "src.additionalRoots", "permissions", "root.", "root", ".permissions.", ".link", "folder.", ".folder.", "link", "source.", "sourceFileExtensions", "bin.includes", "bin.excludes", "javacCustomEncodings.", "javacDefaultEncoding." -> PDEPluginImages.DESC_CATEGORY_OBJ;


Yes that line is a bit long. I suggest to put comments // at the end of the manually broken line to prevent the formatter to merge them again.

HannesWell · 2023-05-01T15:20:47Z

@alshamams do you want to address the pending remarks?
If yes can you please just squash and force push your branch. Eventually we will squash it any ways.

alshamams · 2023-05-03T09:02:31Z

@alshamams do you want to address the pending remarks?
If yes can you please just squash and force push your branch. Eventually we will squash it any ways.

Hi @HannesWell,
I have been on sick leave the past week, and I will look into the pending remarks as soon as possible.

The auto-complete feature attaches most relevant image icons to the properties listing, so that the user is able to associate meaning of the property intuitively.

alshamams · 2023-05-03T09:58:37Z

Hi @HannesWell,
I have addressed all the pending remarks, squashed and force pushed the branch. Kindly let me know in case of any edits.

HannesWell

This PR looks good now. Thank you @alshamams!

I have been on sick leave the past week, and I will look into the pending remarks as soon as possible.

No problem, it was just a gentle reminder/a question :)

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java

alshamams force-pushed the autocomplete_image branch from b508815 to d64d2f2 Compare March 28, 2023 15:09

HannesWell reviewed Mar 28, 2023

View reviewed changes

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java Outdated Show resolved Hide resolved

alshamams force-pushed the autocomplete_image branch from d64d2f2 to db01193 Compare April 4, 2023 14:58

laeubi added this to the 4.28 M2 milestone Apr 4, 2023

HannesWell reviewed Apr 4, 2023

View reviewed changes

alshamams force-pushed the autocomplete_image branch from db01193 to 9519630 Compare April 18, 2023 09:15

gireeshpunathil approved these changes Apr 19, 2023

View reviewed changes

HannesWell approved these changes Apr 20, 2023

View reviewed changes

HannesWell force-pushed the autocomplete_image branch from 9519630 to 1363fcb Compare May 1, 2023 15:19

alshamams force-pushed the autocomplete_image branch from 1363fcb to 1b010c2 Compare May 3, 2023 09:26

Auto-complete for build.properties: Image Icons

854f8d3

The auto-complete feature attaches most relevant image icons to the properties listing, so that the user is able to associate meaning of the property intuitively.

alshamams force-pushed the autocomplete_image branch from 1b010c2 to 854f8d3 Compare May 3, 2023 09:43

HannesWell approved these changes May 3, 2023

View reviewed changes

.../org/eclipse/pde/internal/ui/editor/contentassist/BuildPropertiesContentAssistProcessor.java Show resolved Hide resolved

HannesWell merged commit dabb850 into eclipse-pde:master May 3, 2023
11 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto-complete for build.properties: Image Icons #531

Auto-complete for build.properties: Image Icons #531

alshamams commented Mar 24, 2023

github-actions bot commented Mar 24, 2023 •

edited

alshamams commented Mar 28, 2023

HannesWell left a comment

laeubi commented Apr 4, 2023

alshamams commented Apr 4, 2023

HannesWell left a comment •

edited

HannesWell Apr 4, 2023

alshamams Apr 18, 2023

vogella commented Apr 14, 2023

alshamams commented Apr 17, 2023

alshamams commented Apr 18, 2023

gireeshpunathil left a comment

gireeshpunathil Apr 19, 2023

gireeshpunathil Apr 19, 2023

alshamams Apr 19, 2023

HannesWell Apr 20, 2023

HannesWell left a comment

HannesWell Apr 20, 2023

HannesWell Apr 20, 2023

HannesWell Apr 20, 2023

HannesWell commented May 1, 2023

alshamams commented May 3, 2023

alshamams commented May 3, 2023

HannesWell left a comment

	ImageDescriptor desc = null;
	desc = switch (element)
	ImageDescriptor desc = switch (element)

Auto-complete for build.properties: Image Icons #531

Auto-complete for build.properties: Image Icons #531

Conversation

alshamams commented Mar 24, 2023

github-actions bot commented Mar 24, 2023 • edited

Test Results

alshamams commented Mar 28, 2023

HannesWell left a comment

Choose a reason for hiding this comment

laeubi commented Apr 4, 2023

alshamams commented Apr 4, 2023

HannesWell left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogella commented Apr 14, 2023

alshamams commented Apr 17, 2023

alshamams commented Apr 18, 2023

gireeshpunathil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HannesWell commented May 1, 2023

alshamams commented May 3, 2023

alshamams commented May 3, 2023

HannesWell left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 24, 2023 •

edited

HannesWell left a comment •

edited