Bug 428355 - [New Contributors] Fix warnings in platform projects
Summary: [New Contributors] Fix warnings in platform projects
Status: CLOSED DUPLICATE of bug 418661
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.4   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Platform-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: bugday
Depends on:
Blocks: 420779
  Show dependency tree
 
Reported: 2014-02-17 09:15 EST by Lars Vogel CLA
Modified: 2014-04-28 14:54 EDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Vogel CLA 2014-02-17 09:15:52 EST
This bug is supported to serve as a placeholder for fixing simple warnings in the platform code. These changes are very welcome as they help us to maintain a clean  code basis but we want to avoid a flush of new bug reports for every warning.

I suggest that such changes are NOT added to the copyright header to avoid to pollute it with such minor changes.
Comment 1 Jeanderson Candido CLA 2014-02-20 01:13:33 EST
I don't know if it is considered a simple warning, but it was about an unchecked casting that could possibly lead to a ClassCastException.

In fact, it is possible to have elements that are not Strings, like in the example found here: http://stackoverflow.com/questions/17174026/javas-properties-differences-between-propertynames-and-stringpropertynames

Because the specified execution environment (Java 1.5) doesn't have implemented stringPropertyNames(), it wasn't possible to use it.

Here is the review: https://git.eclipse.org/r/#/c/22285/

I apologize if I should have opened a new issue. In this case, I can update my patch.

Thanks in advance.
Comment 2 Lars Vogel CLA 2014-02-20 05:54:16 EST
(In reply to Jeanderson Candido from comment #1)
> Because the specified execution environment (Java 1.5) doesn't have
> implemented stringPropertyNames(), it wasn't possible to use it.

Does is help if I upgrade JFace to 1.6?
Comment 3 Jeanderson Candido CLA 2014-02-20 08:13:10 EST
(In reply to Lars Vogel from comment #2)
> (In reply to Jeanderson Candido from comment #1)
> > Because the specified execution environment (Java 1.5) doesn't have
> > implemented stringPropertyNames(), it wasn't possible to use it.
> 
> Does is help if I upgrade JFace to 1.6?

If there's no problem, I think it would be better, because it wouldn't be necessary to allocate an ArrayList as an "mediator" to have a static array of Strings. Also, we wouldn't have to care about checking object's type since stringPropertyName is tied to a set of Strings.
Comment 4 Lars Vogel CLA 2014-02-20 11:49:46 EST
(In reply to Jeanderson Candido from comment #3)
> (In reply to Lars Vogel from comment #2)
> > (In reply to Jeanderson Candido from comment #1)
> > > Because the specified execution environment (Java 1.5) doesn't have
> > > implemented stringPropertyNames(), it wasn't possible to use it.
> > 
> > Does is help if I upgrade JFace to 1.6?
> 
> If there's no problem, I think it would be better, because it wouldn't be
> necessary to allocate an ArrayList as an "mediator" to have a static array
> of Strings. Also, we wouldn't have to care about checking object's type
> since stringPropertyName is tied to a set of Strings.

org.eclipse.jface has now been upgraded to Java 1.6 in master. Please update your Gerrit review
Comment 5 Jeanderson Candido CLA 2014-02-20 11:59:37 EST
Lars, changing to Java 1.6 caused some warnings in the code (e.g. implemented methods should use @Override).

Is it okay if I update my review and create another one to fix these new warnings (e.g. add @Override on implemented methods)?
Comment 6 Jeanderson Candido CLA 2014-02-20 12:13:41 EST
(In reply to Lars Vogel from comment #4)
> (In reply to Jeanderson Candido from comment #3)
> > (In reply to Lars Vogel from comment #2)
> > > (In reply to Jeanderson Candido from comment #1)
> > > > Because the specified execution environment (Java 1.5) doesn't have
> > > > implemented stringPropertyNames(), it wasn't possible to use it.
> > > 
> > > Does is help if I upgrade JFace to 1.6?
> > 
> > If there's no problem, I think it would be better, because it wouldn't be
> > necessary to allocate an ArrayList as an "mediator" to have a static array
> > of Strings. Also, we wouldn't have to care about checking object's type
> > since stringPropertyName is tied to a set of Strings.
> 
> org.eclipse.jface has now been upgraded to Java 1.6 in master. Please update
> your Gerrit review

I updated the review. I believe changing to Java 1.6 will not affect other projects.
Comment 7 Jeanderson Candido CLA 2014-02-20 16:20:45 EST
Removed warnings from AbstractAction: https://git.eclipse.org/r/#/c/22333/
Comment 8 Jeanderson Candido CLA 2014-02-20 16:41:26 EST
Removed Warnings from ProgressMonitorPart: https://git.eclipse.org/r/22335
Comment 9 Lars Vogel CLA 2014-02-21 07:27:30 EST
(In reply to Jeanderson Candido from comment #7)
> Removed warnings from AbstractAction: https://git.eclipse.org/r/#/c/22333/

Thanks. Applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=2be6ebb15c991c7a168d0c6c3930acdfb5785fe3
Comment 10 Jeanderson Candido CLA 2014-02-21 23:27:32 EST
PreferenceStore fixed with https://git.eclipse.org/r/#/c/22285/
Comment 11 Jeanderson Candido CLA 2014-02-21 23:40:08 EST
Class org.eclipse.jface.action.Separator has something a little weird:

Method #1
----------------
public void fill(Menu menu, int index) {
    if (index >= 0) {
	new MenuItem(menu, SWT.SEPARATOR, index);
    } else {
	new MenuItem(menu, SWT.SEPARATOR);
    }
}
----------------

Method #2
----------------
public void fill(ToolBar toolbar, int index) {
    if (index >= 0) {
	new ToolItem(toolbar, SWT.SEPARATOR, index);
    } else {
	new ToolItem(toolbar, SWT.SEPARATOR);
    }
}
----------------

What's the meaning of creating an object and not keep its reference or returning it? Is it really supposed to work like this?

Regards
Comment 12 Thomas Schindl CLA 2014-02-22 03:36:26 EST
That's completely legal code and nothing to worry about
Comment 13 Lars Vogel CLA 2014-02-22 05:53:00 EST
(In reply to Jeanderson Candido from comment #11)
> Class org.eclipse.jface.action.Separator has something a little weird:
> 
> Method #1
> ----------------
> public void fill(Menu menu, int index) {
>     if (index >= 0) {
> 	new MenuItem(menu, SWT.SEPARATOR, index);
>     } else {
> 	new MenuItem(menu, SWT.SEPARATOR);
>     }
> }
> ----------------
> 
> Method #2
> ----------------
> public void fill(ToolBar toolbar, int index) {
>     if (index >= 0) {
> 	new ToolItem(toolbar, SWT.SEPARATOR, index);
>     } else {
> 	new ToolItem(toolbar, SWT.SEPARATOR);
>     }
> }
> ----------------
> 
> What's the meaning of creating an object and not keep its reference or
> returning it? Is it really supposed to work like this?
> 
> Regards

The menu item is inserted in the menu so the code is OK.
Comment 14 Matthias Mailänder CLA 2014-02-26 08:37:05 EST
Reduced warnings in AboutPluginsPage: https://git.eclipse.org/r/#/c/22558/
Comment 15 Matthias Mailänder CLA 2014-02-26 10:00:32 EST
Resolved all warnings in AboutBundleData https://git.eclipse.org/r/22571
Comment 16 Lars Vogel CLA 2014-02-28 06:46:31 EST
*** Bug 413611 has been marked as a duplicate of this bug. ***
Comment 17 Jeanderson Candido CLA 2014-03-01 13:09:46 EST
Patch https://git.eclipse.org/r/#/c/22285/ has been updated.
Now it only contains code changes.
Comment 18 Lars Vogel CLA 2014-03-02 17:15:01 EST
(In reply to Jeanderson Candido from comment #17)
> Patch https://git.eclipse.org/r/#/c/22285/ has been updated.
> Now it only contains code changes.

Thanks applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=c88f1112a3569cf9232a90fac548c58af269a5a2
Comment 19 Lars Vogel CLA 2014-03-10 13:06:17 EDT
(In reply to Matthias Mailänder from comment #15)
> Resolved all warnings in AboutBundleData https://git.eclipse.org/r/22571

Thanks, applied with https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=26159c418417325015fc97d686a6fac7082c1eca
Comment 22 Pablo Cabrera CLA 2014-03-19 00:25:48 EDT
I got a fix for this in https://git.eclipse.org/r/#/c/23580/
Comment 25 Lars Vogel CLA 2014-04-28 14:54:20 EDT

*** This bug has been marked as a duplicate of bug 418661 ***