TotalCross / totalcross

TotalCross is a Software Development Kit that helps cross platform application development. Currently supported platforms are: Windows, Wince, Android, iOS, Linux and Linux ARM for embedded systems.
https://www.totalcross.com
GNU Lesser General Public License v2.1
222 stars 40 forks source link

Feature/weak reference implementation #357

Closed flsobral closed 3 years ago

flsobral commented 3 years ago

Added support for WeakReference, along with some related bugfixes and improvements.

This was motivated by the leak caused by update listeners registered to the MainWindow and not properly unregistered after its usage.

jeffque commented 3 years ago

By the description of the 82d4ab5c74bbe15f1c commit, it finally solves a problem reported some 3 years ago in https://gitlab.com/totalcross/TotalCross/-/issues/184: implementing Operator and other interfaces that extends an interface from jdkcompat

jeffque commented 3 years ago

Using iterators would allow for some more idiomatic work for removing/adding the listener. It is cumbersome the way it is written.

Instead of

  public void removeUpdateListener(UpdateListener listener) {
    int idx = updateListeners.size() - 1;
    while (idx >= 0 && updateListeners.get(idx).get() != listener) {
      idx--;
    }
    if (idx >= 0) {
      updateListeners.remove(idx);
    }
  }

one would have

  public void removeUpdateListener(UpdateListener listener) {
    Iterator<WeakReference<UpdateListener>> it = updateListeners.iterator();
    while (it.hasNext()) {
      WeakReference<UpdateListener> el = it.next();
      if (el.get() == listener) {
        it.remove();
        return;
      }
    }
  }

Also, when iterating and removing from the tail while triggering those listeners, with iterators, should be something like:

      ListIterator<WeakReference<UpdateListener>> it = updateListeners.listIterator(updateListeners.size());
      while (it.hasPrevious()) {
        UpdateListener ul = it.previous().get();
        if (ul != null) ul.updateListenerTriggered(elapsedMilliseconds);
        else it.remove();
      }

if wished for a tail first remotion. But also

      Iterator<WeakReference<UpdateListener>> it = updateListeners.iterator();
      while (it.hasNext()) {
        UpdateListener ul = it.next().get();
        if (ul != null) ul.updateListenerTriggered(elapsedMilliseconds);
        else it.remove();
      }

Note that both alternatives aren't great uses for ArrayLists, as removing from anything but the tail can be burdensome because of array shifts. But I think that it is a little more idiomatic Java than keeping those pesky indexes.

As I like a more idiomatic approach, however, one should notice that there may be more calls to virtual methods in my suggestions than in the current PR. And also as it is expected that this list has more volatile members, may using LinkedList is a better approach to avoid array shifts? Needs some more performance research to a final word, maybe.