Cleanup of unused code and marks deprecated methods, constants for removal.

[deepika-u]

No description provided.

[github-actions]

Test Results

 3 018 files  ±0   3 018 suites  ±0   2h 5m 46s ⏱️ - 17m 6s
 8 234 tests ±0   7 985 ✅ +1  249 💤 ±0  0 ❌  - 1 
23 622 runs  ±0  22 828 ✅ +1  794 💤 ±0  0 ❌  - 1 

Results for commit 745252f. ± Comparison against base commit 1a43fc1.

♻️ This comment has been updated with latest results.

[deepika-u]

@akurtakov
When you get some time, can you take a look at this please?

[akurtakov]

I would look again in further deprecations (unless driven by actual breakage) after release.

[deepika-u]

Thanks! Looks like you’ve got your hands full.
I’ll see if anyone else has time to take a look for now.

[deepika-u]

@vogella
Can you take a look at it when you get some time please.

[vogella]

IIRC I also tried to delete these internal methods a while ago but @merks or @BeckerWdf had an issue with that. I might be wrong here it is a while ago.

Ed and Matthias, are you ok with this change?

[merks]

Please don't. EMF uses this and EMF supports ancient versions of Eclipse. Oomph as well, though it could more easily adjust because it does support quite so ancient things. When important functionality is unavailable via APIs, the following is what happens.

In other words, it gets used anyway with Eclipse project itself being the trend setter.

So please no, don't delete things form HTMLPrinter. If the Eclipse project decided it was okay for PDE and JDT to use it, then you can assume the ecosystem is using it, and deleting a few methods is more likely to break the ecosystem than it is to provide any kind of improvement.

I'd need to investigate if EMF can remove it now; I think the oldest support is Photon and the StringBuillder things were available for that I think, and now that minimum Java 8 is supported, probably EMF can move. So if you really, really feel a compelling need, mark these things are deprecated for removal instead (with proper references to the replacement) because this is effectively API despite being internal as evidenced by PDE and JDT treating it as API and hence likely by anyone else wishing to produce rich hover like the Platform.

---

While I don't want to be overly negative, with so very many open bugs to fix and so very many features requests open, maybe we could avoid things like this that literally (for my projects) provide negative value.

[vogella]

I'm very proud of myself that I rememebered correct that @merks had an issue with this deletion. No need to thank me for that. :-)

[merks]

I'm very proud of myself that I rememebered correct that @merks had an issue with this deletion. No need to thank me for that. :-)

Yes you should be proud and I should thank you. ❣️

[merks]

@deepika-u

FYI, I have tested switching EMF to the StringBuilder methods and building against Photon. That succeeds so I will apply the changes. Also I will change Oomph.

So I suggest to treat HTMLPrinter as API, marking the methods for removal like the rest of the PR. That will give time for the EMF, Oomph, and any other fixes to percolate though the system before the methods are actually removed.

[deepika-u]

So I suggest to treat HTMLPrinter as API, marking the methods for removal like the rest of the PR. That will give time for the EMF, Oomph, and any other fixes to percolate though the system before the methods are actually removed.

@merks
Thanks alot for the details given, I have marked the HTMLPrinter APIs as well for deprecation and updated accordingly. Sorry to add more work for you on this.

Although I couldn’t determine the exact reason, my instinct was telling that HTMLPrinter.java is a key internal component, and removing it could have unintended side effects and that was the first reason i requested a review from akurtakov.

Thanks alot for taking time on this - @vogella and @merks

[merks]

Thank you. Sorry for my harsh comments.

[vogella]

@deepika-u Thanks for doing these cleanups. This is an important task for future maintenance and enhancements. Unfortunately, it’s also one of those tasks where you usually only get negative feedback if something breaks. I’ve done my fair share of cleanups in the past and got a lot of bashing for it. :-)

But without regular cleanups, our codebase would decay and make future development harder.

So, thank you for working on this.

[vogella]

The failing tests seem to be #1517

[merks]

Indeed cleanup is thankless work and "no good deed goes unpunished" on that front.

That being, there are two sides to every coin. It would also be very great for these numbers to be much smaller;

I think cleanups are often something that only we developers see (and we are of course king) while the list of things that the users sees has the potential to improve the day-to-day experience of millions of people.

Again, I don't think this is an either or thing, both things are important and both have value. I just sometimes feel that we might on the balance spend just more time looking outward rather than inward. "What can I do with my limited time to benefit the most people the most?"

Anyway, again sorry for the negativity, and thanks Lars for bringing this to my attention so that I could take action to allow this to move forward positively rather than that I notice this at an untimely moment.

[deepika-u]

Thank you. Sorry for my harsh comments.

No problem, atleast some one should do at some time. So i did some this time. Thats it.

[deepika-u]

Anyway, again sorry for the negativity, and thanks Lars for bringing this to my attention so that I could take action to allow this to move forward positively rather than that I notice this at an untimely moment.

Thanks for the motivation again, right now i have already moved to the other side of the coin. So i am on the right path already.
Thanks once again.

[deepika-u]

Thanks @merks for your approval.

@vogella : do you have any other suggestions on these changes? If we are good, can you take this pr forward please?

[vogella]

Thanks @merks for your approval.

@vogella : do you have any other suggestions on these changes? If we are good, can you take this pr forward please?

I only merge things if all checks are green, can you re-trigger the build? I'm actively working on reducing flacky tests but this one may have been an infrastructure issue.

[vogella]

New warning: The method addCSSToHTMLFragment(String) from the type new BrowserInformationControl(){} is never used locally

[merks]

I started a new build but the build looked like it failed simply because there was a new compiler warning; we've been seeing quite a few of those with JDT's deprecation warning changes. Although this looks like maybe it is wrong and should be cleaned up:

But it's unrelated to this PR and maybe JDT already cleaned it up.

[deepika-u]

But it's unrelated to this PR and maybe JDT already cleaned it up.

As part of this pr, we did a change in RevisionPainter.java as well as part of this cleanup. Since its internal and no reference to the setInformation() - it was removed and addCSSToHTMLFragment() is being called in setInformation() which is now throwing this compiler warning.
Does that mean we need to remove even addCSSToHTMLFragment() related part? or put back setInformation() as well and just mark for deprecation? or revert this change and do nothing with respect to this file change?

[vogella]

I vote for removing the unused private method.

[deepika-u]

I vote for removing the unused private method.

Thank you and i am also with you as it is internal file. Will wait for @merks opinion.

[deepika-u]

Making the above changes to RevisionPainter.java will be like the earlier change of HTMLPrinter.java file change so marking this also for removal as of now (Because this change is again dependent on HTMLPrinter.java change - i just tried).

@vogella : Thanks for sharing your opinion on this too.

[vogella]

Thanks @deepika-u

[HeikoKlare]

@deepika-u @vogella it seems like this was merged without properly checking for consumers of those methods inside the SDK. See, for example this triggered auto-cleanup that shows a suppressed deprecation for one of the touched methods: https://github.com/eclipse-platform/eclipse.platform/pull/2268/files

Would you please check the SDK for such cases and adapt where necessary? In my opinion, usages (at least in the SDK) should be replaced before marking a method as deprecated for removal. Please also see Ed's comment in the same direction: #3467 (comment)

[deepika-u]

Created a revert for that change -> #3538