[FIX][E] #32 Splitting annotations do not honor the history #596

srossbach · 2019-07-28T00:49:29Z

This patch fixes the last outstanding issues regarding #32.

Annotations that were split are now correctly inserted to the history
and so correctly removed.

This patch introduces a new method to the annotation helper as well as
it reduces LOC in the ContributionAnnotationManager.

Drakulix

I just found some low hanging fruits, as I am not very familiar with the annotation system.
But this seems reasonable and tested, the overall approach seems fine.

Drakulix · 2019-07-29T08:49:43Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

+        final History history = getHistory(source);
+
+        for (final ListIterator<HistoryEntry> lit = history.entries.listIterator();
+            lit.hasNext(); ) {


This could be a for-each loop instead of manually advancing the iterator.

Please look carefully at the code again, especially the code starting at line 203. This is not a simple iteration.

Okay right, then please add some comments to the code.
It took me way too long to grasp, what this is doing (especially because I have no idea about annotations) and I think this could be explained. (I also feel like this could be way easier using functional-programming, but that's my personal opinion.)

Functional Programming is normally harder to read.

removeAllAnnotation ended up with:

sourceToHistory .values() .stream() .flatMap(h -> h.entries.stream()) .collect( HashSet<IAnnotationModel>::new, (s, e) -> s.add(e.annotation.getModel()), HashSet::addAll) .forEach( m -> annotationModelHelper.removeAnnotationsFromModel( m, (a) -> a instanceof ContributionAnnotation));

On the other hand optimization is done by manipulating the existing data structure. That is something that you do not do in functional programming, e.g you have a big list where some entries should be removed. In a functional approach you create a new list containing all the entries without the entries that should be removed. While this may be fine for an ArrayList in terms of performance it is a huge performance loss for a LinkedList.

Functional Programming is normally harder to read.

I would in general not agree with that statement. One could even argue your suggested functional code is more readable, because it does the same, while being shorter.

But I get, that a stream of streams is generally not what you want. (Which is why I just requested some comments on your old approach.)

While this may be fine for an ArrayList in terms of performance it is a huge performance loss for a LinkedList.

While this is obviously true, this is, in my opinion, an optimization the compiler should be able to infer.

The Java compiler does not really optimize anything. Even the JIT does not optimize much stuff. You are simply expecting too much from a compiler.

Not from a compiler, just from Java/JVM...

Read again. Just In Time (compiler) ... it is part of the JVM.

Really the can not optimize every thing.

for (int i = 0; i < x; i++)
{
Collection c = xyz.calculate(...) // will run in O(n^10) and always return the same result
... do something with c
}

I have seen such cases where the JIT is unable to move such calls out of the loop because the compiler fails to determine if calculate is idempotent in the current context and so you end up with a total running time of O(x * n^10) instead of O(x + n^10).

Drakulix · 2019-07-29T08:51:35Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

+
+    final Iterator<HistoryEntry> it = history.entries.iterator();
+
+    if (!it.hasNext()) return removedEntries;


So if history.entries.isEmpty()? Would be more readable in my opinion. (Maybe even add an isEmpty function to the history class.)

Drakulix · 2019-07-29T08:52:05Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

+    if (!it.hasNext()) return removedEntries;
+
+    while (it.hasNext()) {
+      final HistoryEntry entry = it.next();


~~Again. Why is this no for-each loop? You can break on those as well, if I am not mistaken.~~

Alright, another removing loop. Also not very well readable, but fine.

Drakulix · 2019-07-29T08:52:43Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

-      ContributionAnnotation annotation = it.next();
-      if (annotation.equals(oldAnnotation)) {
-        it.set(newAnnotation);
+    for (final ListIterator<HistoryEntry> it = history.entries.listIterator(); it.hasNext(); ) {


Okay, am I missing something at this point? What is the matter with all those manual iterator loops?

Well in this case I could you for:each.

I guess my intention was to replace the HistoryEntry but it seems I just replaced the annotation in the entry as this works as well.

m273d15 · 2019-07-29T08:42:23Z

eclipse/src/saros/editor/internal/AnnotationModelHelper.java

+      Map<Annotation, Position> replacement) {
+
+    // Collect annotations.
+    ArrayList<Annotation> annotationsToRemove = new ArrayList<Annotation>(128);


Why is the capacity initialized with 128? If required: Should we move the number into a constant with a meaningful name?

The default size of an array list is like 8 or something. This is simply a tiny performance gain (several nanoseconds) as you can expect to be at least more than 8 of or annotations in an editor / annotation model.

m273d15 · 2019-07-29T09:10:55Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

-   * See also https://github.com/saros-project/saros/issues/32 that includes
-   * also another defect which is part of this behavior
-   */
-  @SuppressWarnings("unchecked")
  public void splitAnnotation(final IAnnotationModel model, final int offset) {


This might be a topic for a future refactoring: Splitting the method into multiple methods in order to improve the readability.

m273d15 · 2019-07-29T10:12:14Z

eclipse/src/saros/editor/internal/ContributionAnnotationManager.java

+    final Set<IAnnotationModel> annotationModels = new HashSet<>();
+
+    for (final History history : sourceToHistory.values())
+      while (!history.entries.isEmpty())


Is this an eternal loop? The def. of peek says Retrieves, but does not remove, the head (first element) of this list..
Why not simply for (HistoryEntry entry : history.entries) ?

~~maybe for (HistoryEntry entry : history.entries.sublist(0, history.entries.size()) if your intention is to clear history.entries to avoid that for loop altogether.~~

scratch that, there is no method that returns all contents, while also clearing the list...

This patch fixes the last outstanding issues regarding #32. Annotations that were split are now correctly inserted to the history and so correctly removed. This patch introduces a new method to the annotation helper as well as it reduces LOC in the ContributionAnnotationManager.

srossbach requested review from Drakulix, m273d15, tobous and stefaus July 28, 2019 00:49

srossbach added Area: Eclipse Issue affecting Saros for Eclipse (Saros/E) Aspect: GUI Issues specific to the Saros GUI Aspect: User Experience Issue that describes a problem with the usability/user experience of Saros labels Jul 28, 2019

Drakulix reviewed Jul 29, 2019

View reviewed changes

m273d15 requested changes Jul 29, 2019

View reviewed changes

srossbach force-pushed the pr/fix_32 branch from 0a76658 to f89f081 Compare July 29, 2019 13:49

saros-project deleted a comment Jul 29, 2019

m273d15 approved these changes Aug 1, 2019

View reviewed changes

srossbach merged commit 9d4eae0 into master Aug 2, 2019

srossbach deleted the pr/fix_32 branch August 2, 2019 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX][E] #32 Splitting annotations do not honor the history #596

[FIX][E] #32 Splitting annotations do not honor the history #596

srossbach commented Jul 28, 2019

Drakulix left a comment

Drakulix Jul 29, 2019

srossbach Jul 29, 2019

Drakulix Jul 29, 2019

srossbach Jul 29, 2019

Drakulix Jul 29, 2019 •

edited

Loading

srossbach Jul 29, 2019

Drakulix Jul 29, 2019

srossbach Jul 29, 2019

Drakulix Jul 29, 2019

Drakulix Jul 29, 2019 •

edited

Loading

Drakulix Jul 29, 2019

srossbach Jul 29, 2019

m273d15 Jul 29, 2019

srossbach Jul 29, 2019

m273d15 Jul 29, 2019

m273d15 Jul 29, 2019

srossbach Jul 29, 2019

Drakulix Jul 29, 2019 •

edited

Loading


		final Iterator<HistoryEntry> it = history.entries.iterator();

		if (!it.hasNext()) return removedEntries;

[FIX][E] #32 Splitting annotations do not honor the history #596

[FIX][E] #32 Splitting annotations do not honor the history #596

Conversation

srossbach commented Jul 28, 2019

Drakulix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drakulix Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drakulix Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drakulix Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Drakulix Jul 29, 2019 •

edited

Loading

Drakulix Jul 29, 2019 •

edited

Loading

Drakulix Jul 29, 2019 •

edited

Loading