eclipse / xtext-eclipse

xtext-eclipse
Eclipse Public License 2.0
49 stars 73 forks source link

Fix FormatterModifyDialog (breaks with Photon) #539

Closed cdietrich closed 6 years ago

cdietrich commented 6 years ago

our heavy use of internal api again makes us suffer like hell. who ever thought it would be a good idea: no it was not

FormatterModifyDialog has compile errors.

get rid of them or make platform revert their changes

cdietrich commented 6 years ago

and make sure eclipse/xtend photon target contains the latest stuff see https://github.com/eclipse/xtext-eclipse/issues/530 as well

cdietrich commented 6 years ago

https://github.com/eclipse/eclipse.jdt.ui/commits/81c6f1bc2edf390bae5ade83672ec64a1903f7d0/org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/preferences/formatter/ModifyDialog.java

cdietrich commented 6 years ago

looks devastating 😠 😠 😠 😠 😠

cdietrich commented 6 years ago

https://github.com/eclipse/xtext/issues/2393

cdietrich commented 6 years ago

@dhuebner any idea how to fix this besides copy & pasting whole of jdt?

cdietrich commented 6 years ago

Jdt Formatter Looks completely different now

bildschirmfoto 2018-01-30 um 11 13 01 bildschirmfoto 2018-01-30 um 11 12 41
dhuebner commented 6 years ago

@cdietrich Is the C&P issue already fixed?

cdietrich commented 6 years ago

no, if you build against photon youll see the compile errors

cdietrich commented 6 years ago

started http://services.typefox.io/open-source/jenkins/job/xtext-xtend/job/master/ for you

kthoms commented 6 years ago

Class ModifyDialog in JDT Internal has been changed significantly. I do not see yet that we could get back the method that we were using. This would mean that the dialog would be at least broken for <= 2.13.

cdietrich commented 6 years ago

hmmm sounds like a situation to investiagate how we can deliver feature patches

kthoms commented 6 years ago

Yes that might be necessary.

cdietrich commented 6 years ago

Options i see (1) copy & paste the Oxygen Code (2) copy & paste the Photon code and adapt appearance to the one of photon (3) develop own style of preference page (4) remove the preference page (5) leave it broken

cdietrich commented 6 years ago

@ArneDeutsch will investigate cc @dhuebner @kthoms

ArneDeutsch commented 6 years ago

I have investigated our options.

  1. C&P from Oxygen will result in >1000 LOC of copied code (three classes full of warnings because of usage of internal classes) but not much additional effort because our code can stay nearly untouched. Dialog will look as before for all eclipse versions.
  2. C&P from Photon will result at least in >2000 LOC of copied code (three classes full of warnings because of usage of internal classes). and an uncertain amount of additional effort to adapt our own code. Dialog will look as new Photon version for all eclipse versions.
  3. Adapting FormatterModifyDialog with some reflection magic to work for all eclipse versions will result in >250 LOC of partly copied, partly ugly reflective code. Dialog will look as before for all eclipse versions.

Because of these results I would propose to proceed as followes:

  1. Inside this ticket adapt the FormatterModifyDialog with some reflective code to work for Photon and older versions as well. It is not THAT much code, resolves the compile errors and functionality stays as it is without to much risk and effort.
  2. Afterwards, as part of an other ticket to be released for Photon or probably later, start implementation of our own formatter preference API on Xtext level. Then we can rewrite the Xtend formatter preferences to use the new Xtext API and user can build on that, too.
cdietrich commented 6 years ago

sounds promising and viable @kthoms @szarnekow what do you think?

szarnekow commented 6 years ago

Sounds good to me, too. We have to ensure that the code is not only compile clean but is also executed during the build. That is, we have to have automated tests for the stuff.

cdietrich commented 6 years ago

do you think a smoke test that klicks through the package page would be sufficient enhough?

szarnekow commented 6 years ago

"package" <- preference? Yes, that would be a great starter. If we see regressions, we can refine that later e.g. by checking that preferences are persisted

cdietrich commented 6 years ago

yes meant the prefernce package

kthoms commented 6 years ago

screenshot 23 The text 'Xtend formatterPreview' looks strange. I think just 'Preview' like in JDT would be better. Will create a PR for it.

kthoms commented 6 years ago

Thanks Arne!

cdietrich commented 6 years ago

thanks @ArneDeutsch. great work