Bug 501579 - Update org.eclipse.e4.ui.model.workbench to Java 8 and clean up the generated code
Summary: Update org.eclipse.e4.ui.model.workbench to Java 8 and clean up the generated...
Status: RESOLVED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 4.6   Edit
Hardware: PC Windows 7
: P3 normal (vote)
Target Milestone: 4.12   Edit
Assignee: Ed Merks CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on: 394720
Blocks: 545330
  Show dependency tree
 
Reported: 2016-09-16 05:59 EDT by Lars Vogel CLA
Modified: 2019-04-05 08:22 EDT (History)
4 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 2016-09-16 05:59:03 EDT
All the rest has moved to Java 8 and it would be nice to use the same JRE for all plug-ins.
Comment 1 Lars Vogel CLA 2016-09-16 06:03:34 EDT
I changed the plug-in BREE / classpath and the Compliance Level in the gen model. I expected the a new generation run would re-generate the methods with @Override. 

Ed, could you advice? In case you what to see the changes, I will upload a Gerrit review.
Comment 2 Eclipse Genie CLA 2016-09-16 06:04:20 EDT
New Gerrit change created: https://git.eclipse.org/r/81225
Comment 3 Ed Merks CLA 2016-09-16 06:12:20 EDT
So far I've done nothing in EMF to generate special things beyond 1.5 compliance.

You can quick fix to add the annotations and the generator won't remove them, but that's it...
Comment 4 Lars Vogel CLA 2016-09-16 07:06:33 EDT
(In reply to Ed Merks from comment #3)
> So far I've done nothing in EMF to generate special things beyond 1.5
> compliance.

Is this on the EMF roadmap? @Override generation feels like something that should be supported by a code generator.

> You can quick fix to add the annotations and the generator won't remove
> them, but that's it...

Thanks.
Comment 5 Ed Merks CLA 2016-09-16 09:14:28 EDT
There's https://bugs.eclipse.org/bugs/show_bug.cgi?id=394720 but you can see it's really quite old. It's relatively tricky and time consuming to revisit all the templates.
Comment 6 Lars Vogel CLA 2016-09-16 10:45:23 EDT
(In reply to Ed Merks from comment #5)
> There's https://bugs.eclipse.org/bugs/show_bug.cgi?id=394720 but you can see
> it's really quite old. It's relatively tricky and time consuming to revisit
> all the templates.

Thanks Ed. I close this bug as for now as WONTFIX and once 394720 gets fixed we can do the update.
Comment 7 Lars Vogel CLA 2018-12-20 08:51:44 EST
Thanks Ed for the update of EMF. Could you please do the update of the Eclipse model to have the @Override included?
Comment 8 Ed Merks CLA 2019-02-10 00:39:01 EST
BTW, I'm not just ignoring this, and did in fact start working on it.  But I have customer work that takes priority...

The most annoying thing about this effort is the disastrous reformatting of the  generated Javadoc which makes the merger not find the <!-- ... --> markers because they've been split across multiple lines.  Also, fairly recent commits (to partially remove a deprecated EClass) appear to have made changes highly questionable changes to the code that.  :-(

Actually fixing these problems properly would be a very large effort and produce a large delta from the Javadoc disaster.  In my customer projects I add the following simple line to the org.eclipse.jdt.ui.prefs of a project with a model so that all other formatting choices are inherited, but for this particular project the Javadoc is not formatted:

org.eclipse.jdt.core.formatter.comment.format_javadoc_comments=false

It's impossible to produce this result with the UI's preference support; it has a bad habit of spitting out all settings, not just a delta...

In any case, I'll be away until March 11th, and at that point I'll be very busy with backlog, so it's highly unlikely I'll have time for this for quite some time...
Comment 9 Lars Vogel CLA 2019-02-19 03:32:18 EST
Mass change, please reset target if you still planning to fix this for 4.11.
Comment 10 Ed Merks CLA 2019-04-05 01:57:37 EDT
A Gerrit commit is pending.

This exercise has mostly turned into one of recovering a clean maintainable state of the generated model code, i.e., a state where in the future you will be able to regenerate and pick up any changes made to the generator and of course any changes made to the model.  

Rather disastrously depressing commits have been made while deprecating API:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=509868
https://bugs.eclipse.org/bugs/show_bug.cgi?id=531054

The complete mess was caused by two things: 

The worst of problem is the auto save action that does formatting compounded by the fact that the formatting settings are set to format comments.  This makes an unreadable mess of the comments and most significant it prevents merging of the comments. 

The approach of making massive manual changes to the Javadoc outside of the user-doc brackets.

------

Some rules to follow in the future:

All manual changes to comments must be done within the <!-- begin-user-doc --> <!-- end-user-doc --> brackets and those must remain on separate lines.  Much better would to add a documentation annotation to the model itself.

Never ever for any reason change any of the Java code in a generated package implementation.  Don't even think about it.

When deprecating something, do so by adding a GenModel documentation annotation to the Ecore model.

When adding something, add a GenModel documentation annotation to its Ecore model, including an @since tag.

If you change anything whatsoever in any generated class, regenerate the model before committing your changes.

-----

The tags @since, @deprecated, and @noreference in a documentation annotation are processed in a special way.  Note that the special processing for @noreference is new to EMF 2.18 and was added to support the usage pattern I encountered in the e4 model; it is not yet committed, pending the final results of this Bugzilla.

Generally the documentation annotation is emitted in exactly one place, on the primary "artifact" that's generated.  But these special tags are extracted and are also emitted in all the other places where code is generated because of that artifact.  For example, the full documentation for an EClass is emitted on the generated interface, but the special tags are also emitted in the generated implementation class, the package interface/class where that EClass' constants are accessed, the factory interface/class where an instance is created, and so on.

So when something is annotated as deprecated, the @deprecated is generated/emitted in all the correct places, and also an @Deprecated Java annotation is generated.  Similarly for the @since and the @noreference tags.  It seems you generally want to include an @noreference in addition to the @deprecated, which seems like overkill but so be it.

To recover a clean state, I hacked EMF so that regenerating both *.genmodel resource completely replaced all generated Java files with purely generated clean new results, except to include the original header comment of the target file.  This of course removes all hand written changes.  Then I carefully reviewed the changes in each and every Java file to see what was lost.  I then added documentation annotations to the Ecore model to recover now-missing deprecated/since/noreference comments. With the updated version of EMF, these tags all propagate to the right places.  To further reduce manual changes to Java code, mostly related to localization overrides, I added EOperations to the model with a GenModel body annotation so that many manual changes are now marked @generated because they actually come from the model.  This highlighted that a number of the updateLocalization() overrides were missing calls to super, i.e., the existing code was just kind of wrong.

In fact the whole exercise highlights problems and oversights in previous manual changes.  E.g., when a feature is deprecated, not only should the getter be deprecated but the setter as well.  E.g., org.eclipse.e4.ui.model.application.ui.menu.MHandledItem.setWbCommand(ParameterizedCommand) is not marked @noreference even though the getter is marked that way.  Also, in an attempt to produce a warning-free project, it became evident that while MDialog is deprecated, the method org.eclipse.e4.ui.model.application.MApplication.getDialogs() is not deprecated.  You would find, in the future, that when you finally want to delete Dialog form the model, you likely can't because you did not properly deprecate all API relating to that class.  This is why you should not live with a sea of warnings in your code; you won't notice the important ones among the ones you have been ignoring.

Unfortunately the set of changes is very large and will be difficult to review.  One other change I would like to make before the final reviewed changes are committed is to enabled "Code Formatting" for both *.genmodel resources so that any manual edits (which should generally be avoided) will not cause differences because for auto-save formatting action.  But to reduce the review delta size, I currently have not set that to true.

So let's agree that the changes so far are the appropriate changes, and then we can include formatting (white-space only) changes to the final commit.

I apologize in advance for the massive review.  Of course it was a lot of work to produce these results and it seems a bit overly complicated to review fully/properly.  But without this painful step, the current state is kind of a disaster area...

Note I ran some of the tests, e.g., UI Test Suite and those look fine, so we'll see what happens with the Gerrit build.
Comment 11 Ed Merks CLA 2019-04-05 02:05:43 EDT
Of course I should note that @Override is now present in all the necessary place.

Also of note is that this commit (from the referenced Bug 545330) made changes that are overwritten when the code is regenerated:

https://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=a07d2347039ab0036d8befb772277912ae74847c

In my Gerrit review this is fixed by modifying the model's documentation to not duplicate the @return tag.
Comment 12 Eclipse Genie CLA 2019-04-05 02:09:15 EDT
New Gerrit change created: https://git.eclipse.org/r/140082
Comment 14 Lars Vogel CLA 2019-04-05 05:37:57 EDT
Thanks, Ed.
Comment 15 Eclipse Genie CLA 2019-04-05 05:43:50 EDT
New Gerrit change created: https://git.eclipse.org/r/140098
Comment 17 Ed Merks CLA 2019-04-05 08:22:26 EDT
Note that I've committed the EMF generator changes for @noreference support via:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=546147