Replace Guava's collection factories by standard Java

[HannesWell]

• Replace LinkedList with ArrayList or ArrayDeque

Part of #199

[NiklasRentzCAU]

Generally I like the idea of reducing the amount of dependencies, especially as they are not needed anymore with newer versions of Java. I would like to merge this (and further merge other changes completely removing Guava). However, here I would like to discuss this with @sailingKieler first, as from earlier I remember he had some objections from making the source code base incompatible with Java 1.8 in the past.

I have no objections to have the main branch of this project getting incompatible with deprecated Java versions, so that further development can use such newer features, and older systems using Java 1.8 can use older releases, or update Java as well.

The code changes look good, with a few changes of which I am not entirely sure from just looking at the diff if that has the same behavior or if the tests cover that. Have you tested the code yourself at those parts, where you did not just replace the list/map constructors?
Furthermore, is there a specific reason on why you changed some linked lists to array lists?

[HannesWell]

Thanks for having a look at this!

However, here I would like to discuss this with @sailingKieler first, as from earlier I remember he had some objections from making the source code base incompatible with Java 1.8 in the past.

Of course. I just want to mention that Java-11 is already required and the generated byte-code probably already targets Java-11 JVMs. And since the project is configured to build with Java-11 sooner or later Java-11 will probably be used intentionally or unintentionally.

The code changes look good, with a few changes of which I am not entirely sure from just looking at the diff if that has the same behavior or if the tests cover that. Have you tested the code yourself at those parts, where you did not just replace the list/map constructors?

Oh looks like some of the non-trivial changes already slipped into this. Let me remove them for this one. I can propose them in a follow-up again.
I didn't test the changes (I'm not the greatest expert of your code 😅) and only did code replacements.

Furthermore, is there a specific reason on why you changed some linked lists to array lists?

As far as I can tell it is usually recommended to not use linked list since ArrayList is faster in all cases except if you want to remove the first element of a list.
But I can do this change in a separate PR so that this one only contains trivial replacements.

[NiklasRentzCAU]

Of course. I just want to mention that Java-11 is already required and the generated byte-code probably already targets Java-11 JVMs. And since the project is configured to build with Java-11 sooner or later Java-11 will probably be used intentionally or unintentionally.

Sure, I already updated to this to make the build compatible with newer Eclipse releases. Generally I would like to eventually update this project to at least Java-17.

Oh looks like some of the non-trivial changes already slipped into this. Let me remove them for this one. I can propose them in a follow-up again. I didn't test the changes (I'm not the greatest expert of your code 😅) and only did code replacements.

For most of the Eclipse / Eclipse UI stuff I am also not an expert, as I have never really touched that and only worked on the standalone / language server part of KLighD, but I can test a few further things if you provide more changes.

As far as I can tell it is usually recommended to not use linked list since ArrayList is faster in all cases except if you want to remove the first element of a list. But I can do this change in a separate PR so that this one only contains trivial replacements.

This was not my code, but I do not think there was a specific preference of using linked lists over array lists. I was just curious to if this has anything to do with the Guava dependencies. You can leave that in this PR, I do not mind. Especially if it is just for lists which never really get that big where a performance difference would be noticable.

[HannesWell]

Of course. I just want to mention that Java-11 is already required and the generated byte-code probably already targets Java-11 JVMs. And since the project is configured to build with Java-11 sooner or later Java-11 will probably be used intentionally or unintentionally.

Sure, I already updated to this to make the build compatible with newer Eclipse releases. Generally I would like to eventually update this project to at least Java-17.

That would be nice. :)
The latest Eclipse (which is at least a requirement for some UI parts) also requires Java-17 and will probably require Java-21 in the not to distant future.

Oh looks like some of the non-trivial changes already slipped into this. Let me remove them for this one. I can propose them in a follow-up again. I didn't test the changes (I'm not the greatest expert of your code 😅) and only did code replacements.

For most of the Eclipse / Eclipse UI stuff I am also not an expert, as I have never really touched that and only worked on the standalone / language server part of KLighD, but I can test a few further things if you provide more changes.

Understand. I can try it also with our Eclipse App where we integrate KLighD.

As far as I can tell it is usually recommended to not use linked list since ArrayList is faster in all cases except if you want to remove the first element of a list. But I can do this change in a separate PR so that this one only contains trivial replacements.

This was not my code, but I do not think there was a specific preference of using linked lists over array lists. I was just curious to if this has anything to do with the Guava dependencies. You can leave that in this PR, I do not mind. Especially if it is just for lists which never really get that big where a performance difference would be noticable.

Yes in the end it is probably irrelevant what's used. Thanks.
I just rebased this PR on the latest master and removed the one rework of a Iterable- to a stream-pipeline for now.

[HannesWell]

However, here I would like to discuss this with @sailingKieler first, as from earlier I remember he had some objections from making the source code base incompatible with Java 1.8 in the past.

@sailingKieler any update from your side on this topic?

[sailingKieler]

However, here I would like to discuss this with @sailingKieler first, as from earlier I remember he had some objections from making the source code base incompatible with Java 1.8 in the past.

@sailingKieler any update from your side on this topic?

Hi @HannesWell, @NiklasRentzCAU

sorry for being silent in the past, there was summer vacation time, etc.
I'm basically fine with dropping Guava, as the variety of major releases out there easily yields dependency trouble, as you pointed out. In the past I felt some kind of sympathy for expressions like Maps.newLinkedHashMap() over the pure constructor call, but that doesn't justify the dependency.

Thank's a lot for your effort Hannes!

[HannesWell]

However, here I would like to discuss this with @sailingKieler first, as from earlier I remember he had some objections from making the source code base incompatible with Java 1.8 in the past.

@sailingKieler any update from your side on this topic?

Hi @HannesWell, @NiklasRentzCAU

sorry for being silent in the past, there was summer vacation time, etc.

No problem, you were not the only one :)

I'm basically fine with dropping Guava, as the variety of major releases out there easily yields dependency trouble, as you pointed out. In the past I felt some kind of sympathy for expressions like Maps.newLinkedHashMap() over the pure constructor call, but that doesn't justify the dependency.

Great! I'm glad you appreciate this work.
Thank you for this library :)

[HannesWell]

@NiklasRentzCAU can we then continue this one? Thanks in advance.

[NiklasRentzCAU]

Alright, good start for removing this dependency. I'll accept and merge this, though two notes:

• changing PlacementUtil.findChildArea's second parameter from a LinkedList to a Deque would probably technically considered an API-breaking change that is not necessary for this change, however Deque being an implemented interface of LinkedList, this should not cause any breaking code or even in binary replacement (at least from what I understand). I think I would like to keep the API as-is if not required (e.g. API using Guava directly).
• The change in the TextActivator code is a change in code generated automatically by Xtext, which should generally never be manually updated. However, the code has not been re-generated since Xtext 2.10 and the corresponding .mwe2 workflow file does not work anyways, I don't know when this broke, but also do not have any urge to fix that. I saw you also proposed changes in Xtext, so I'll let this slip for now. If there are not too many Guava dependencies in automatically-generated code, this is probably fine, but if many more occur, this might be a problem if this code should be re-generated anytime in the future (which I would like to not block). We should keep an eye on this.

[NiklasRentzCAU]

Thanks again @HannesWell! Sorry for the delay, holiday and a conference also hindered me in looking into this earlier. Now I got a little more time to look at this again!

[HannesWell]

Thank you for your review and no problem for the delay.

* changing PlacementUtil.findChildArea's second parameter from a LinkedList to a Deque would probably technically considered an API-breaking change that is not necessary for this change, however Deque being an implemented interface of LinkedList, this should not cause any breaking code or even in binary replacement (at least from what I understand). I think I would like to keep the API as-is if not required (e.g. API using Guava directly).

Almost any change in the method signature (except changing parameter names) is a binary incompatible change, even changing the return type in a source compatible way:

• https://docs.oracle.com/javase/specs/jls/se17/html/jls-13.html#jls-13.4.14

So yes this is indeed a breaking change. I assume I get wrong that you consider this an API.
I create #207 to restore the old method.

* The change in the TextActivator code is a change in code generated automatically by Xtext, which should generally never be manually updated. However, the code has not been re-generated since Xtext 2.10 and the corresponding .mwe2 workflow file does not work anyways, I don't know when this broke, but also do not have any urge to fix that. I saw you also proposed changes in Xtext, so I'll let this slip for now. If there are not too many Guava dependencies in automatically-generated code, this is probably fine, but if many more occur, this might be a problem if this code should be re-generated anytime in the future (which I would like to not block). We should keep an eye on this.

Agree. If I have some time I can have a look why the workflow fails.

[NiklasRentzCAU]

Almost any change in the method signature (except changing parameter names) is a binary incompatible change, even changing the return type in a source compatible way:

• https://docs.oracle.com/javase/specs/jls/se17/html/jls-13.html#jls-13.4.14

So yes this is indeed a breaking change. I assume I get wrong that you consider this an API. I create #207 to restore the old method.

I think a good way forward is to deprecate any API that might be used somewhere and replace other internal occurrences silently. Technically API is anything that is public, but I would exclude anything that is defined in internal folders or code that is obviously meant for internal use and not as part of KLighD's API (such as the Iterables2 class you looked at).

Agree. If I have some time I can have a look why the workflow fails.

The first failure is related to previous restructuring in the code, as the test code is now located elsewhere. The second failure after fixing that is related to the xbase import of the GRandom.xtext grammar not getting resolved. Here is where I left digging further.