Fixed ClassCastException when adding module file #453

protoism · 2023-02-17T13:27:07Z

Modified AbstractModule.getShortName and AbstractModule.getGWtMaven2ModuleName to return an empty string
when Module is not stored in an IFile

I'm just curing the symptoms, as I'm not clear what the code should do.

But it's better than nothing...

Fixes #452

protoism · 2023-02-22T09:31:20Z

.../com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/AbstractModule.java

   * GWT Maven project will have a short name
   * @return the short name for module
   */
  private String getShortName() {


This method fails when the plugin is scanning classpath for manifest files in dependencies, which are in jar files.
Returning an empty string should be the right thing to do, as there's some maven related logic which makes sense only for manifests contained in the source, and not in dependencies

.../com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/AbstractModule.java

niloc132

From what I can tell, there are two AbstractModule subtypes:

ModuleFile, which always uses an IFile storage (the "correct" case according to the old code)
ModuleJarResource, which is always an IJarEntryResource storage (presumably the "wrong" one, since it is the only other case, and doesn't inherit from IFile).

Each of these respective AbstractModule types limits what type it takes in its constructor, so we can be certain that storage is always the expected type. In turn, each type knows what storage it has, so can avoid an instanceof check. In fact, I'd go so far as to say that the storage instanceof IFile check is actually a this instanceof ModuleFile check..

As such, I think a better fix than branching on the type of storage is to make these methods abstract, and correctly implement them in their subtypes. That is, ModuleJarResource will return "" for each of these, and ModuleFile will return the old logic.

Note that we could also use isBinary() as a "is it a file" check, since that is overridden by each type in this way, to return true/false.

To make things a bit more bulletproof along the way, AbstractModule could be made generic on <S extends IStorage>, and then each type could avoid a cast to read its own storage (there are comments, such as ModuleFile.getFile() that would go away, since this would become obvious)...

niloc132 · 2023-02-21T18:41:37Z

.../com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/AbstractModule.java

-    return moduleName;
-  }
-
  private String getGWtMaven2ModuleName() {


Fix typo in this method name while you're changing this?

Suggested change

private String getGWtMaven2ModuleName() {

private String getGwtMaven2ModuleName() {

niloc132 · 2023-02-23T00:20:21Z

...m.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/ModuleJarResource.java

-  }
-
-}
+/*******************************************************************************


note that you're changing line endings

niloc132 · 2023-02-23T00:21:27Z

...tplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/dialogs/ModuleSelectionDialog.java

+      Image image = null;
+
+      IModule module = (IModule) element;
+      if (module instanceof ModuleFile) {


Hmm, while technically true, is this better? I think it might be, but wanted your opinion.

The first thing each block does is to cast it, so that might be right.

niloc132 · 2023-02-23T00:23:00Z

...tplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/dialogs/ModuleSelectionDialog.java

+  @Override
+  protected Comparator<?> getItemsComparator() {
+    return (Object o1, Object o2) -> {
+      Collator collator = Collator.getInstance();
+      IModule module1 = (IModule) o1;
+      IModule module2 = (IModule) o2;


While you're at it, change this to specify the right type, so a cast isn't even needed?

Suggested change

@Override

protected Comparator<?> getItemsComparator() {

return (Object o1, Object o2) -> {

Collator collator = Collator.getInstance();

IModule module1 = (IModule) o1;

IModule module2 = (IModule) o2;

@Override

protected Comparator<IModule> getItemsComparator() {

return (Object module1, Object module2) -> {

Collator collator = Collator.getInstance();

niloc132 · 2023-02-23T00:23:23Z

.../com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/AbstractModule.java

-  protected AbstractModule(IStorage storage) {
-    assert (storage != null);
-    this.storage = storage;
+  protected AbstractModule() {


shouldnt even be necessary, the class is already abstract and has no other ctors

niloc132 · 2023-02-23T00:26:33Z

plugins/com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/modules/ModuleFile.java

+   * @return module name
+   */
+  private String getGwtMaven2ModuleName() {
+    IFile file = storage;


This is silly to have, and the moduleName variable isnt needed either. Probably should just inline it (but the code is so much better I'd be okay with leaving it).

niloc132

Approved, a few thoughts, up to you if we address now or not.

niloc132 · 2023-02-23T16:58:28Z

...com.gwtplugins.gwt.eclipse.core/src/com/google/gwt/eclipse/core/wizards/NewModuleWizard.java

        gwtModuleDtd = "<!DOCTYPE module PUBLIC \"-//Google Inc.//DTD Google Web Toolkit " + versionNum
-            + "//EN\" \"http://google-web-toolkit.googlecode.com/svn/tags/" + versionNum
-            + "/distro-source/core/src/gwt-module.dtd\">";
+            + "//EN\" \"http://gwtproject.org/doctype/" + versionNum


Defer this to a followup patch, since it isn't related to this other work?

niloc132

Fixes the bug (and possibly other latent bugs), cleans up the code, net decrease in code, great work!

Fixed NPE when adding module file

ce1e9a1

protoism commented Feb 22, 2023

View reviewed changes

protoism requested a review from niloc132 February 22, 2023 09:54

niloc132 reviewed Feb 22, 2023

View reviewed changes

Small fixes as per PR review

b1f9940

niloc132 reviewed Feb 23, 2023

View reviewed changes

niloc132 previously approved these changes Feb 23, 2023

View reviewed changes

Fixed eol

57530d2

protoism dismissed niloc132’s stale review via 57530d2 February 23, 2023 08:56

protoism added 3 commits February 23, 2023 10:59

Small cleanup as per PR review

467f24f

Fixed line endings

2c978db

Using typed Comparator + Cosmetic fixes

4979498

protoism requested a review from niloc132 February 23, 2023 11:43

niloc132 reviewed Feb 23, 2023

View reviewed changes

Rolled back DTD Url change

baf43c9

protoism requested a review from niloc132 February 24, 2023 10:33

niloc132 approved these changes Feb 24, 2023

View reviewed changes

protoism merged commit 317bf57 into gwt-plugins:main Feb 24, 2023

protoism deleted the features/fix_module_creation branch February 24, 2023 17:44

niloc132 mentioned this pull request Feb 26, 2023

Compatibility with m2e 1.x - 2.x #451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed ClassCastException when adding module file #453

Fixed ClassCastException when adding module file #453

Uh oh!

protoism commented Feb 17, 2023

Uh oh!

protoism Feb 22, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

niloc132 left a comment •

edited

Loading

Uh oh!

niloc132 Feb 21, 2023

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

protoism Feb 23, 2023

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

protoism Feb 23, 2023

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

niloc132 left a comment •

edited

Loading

Uh oh!

niloc132 Feb 23, 2023

Uh oh!

niloc132 left a comment

Uh oh!

Uh oh!

	private String getGWtMaven2ModuleName() {
	private String getGwtMaven2ModuleName() {

Fixed ClassCastException when adding module file #453

Fixed ClassCastException when adding module file #453

Uh oh!

Conversation

protoism commented Feb 17, 2023

Uh oh!

protoism Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

niloc132 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niloc132 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niloc132 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

protoism Feb 22, 2023 •

edited

Loading

niloc132 left a comment •

edited

Loading

niloc132 left a comment •

edited

Loading