Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Observable buffer ignores update changes from javafx #169

Closed
scalasolist opened this issue Dec 16, 2014 · 9 comments
Closed

Observable buffer ignores update changes from javafx #169

scalasolist opened this issue Dec 16, 2014 · 9 comments

Comments

@scalasolist
Copy link

scalafx.collection.ObservableBuffer.onChange handles three types of ListChangeListener.Change:

  • Permutation
  • Add
  • Remove

The scalafx code checks for corresponded methods: wasPermutated, wasAdded, wasRemoved.

There is another change type UpdateChange and wasUpdated method.

Documentation: https://docs.oracle.com/javase/8/javafx/api/javafx/collections/ListChangeListener.Change.html#wasUpdated--

@jpsacha
Copy link
Member

jpsacha commented Dec 17, 2014

Can you provide a test case that will demonstrate the deficiency?

@scalasolist
Copy link
Author

The intended test case was:

import scalafx.collections.ObservableBuffer
import scalafx.application.JFXApp
import scalafx.Includes._

object TestMain extends JFXApp {
  val items : ObservableBuffer[String] = ObservableBuffer()

  items.onChange( (obs, chs) => {
    for (change <- chs)
      change match {
        case ObservableBuffer.Add(_, _) => println("add")
        case ObservableBuffer.Remove(_, _) => println("remove")
        case ObservableBuffer.Reorder(_, _, _) => println("reorder")
      }
  } )

  println("appending")
  items.append("test")
  println("updating")
  items(0) = ("update")
}

I expect that update method would generate update change, but javafx generated instead pair of delete-remove changes. It differs from its own documentation. Theoretically I still can create delegate object that would generate such change.

@scalasolist
Copy link
Author

I've wrote another test case. It show how scalafx listener lose information.

import scalafx.collections.ObservableBuffer
import scalafx.application.JFXApp
import scalafx.Includes._
import javafx.collections.ListChangeListener

object TestMain extends JFXApp {
  val items : ObservableBuffer[String] = ObservableBuffer()

  val listener = new ListChangeListener[String] {
    def onChanged(chgs : ListChangeListener.Change[_ <: String]) {
      var order = 0
      while ( chgs.next() ) {
        order += 1
        println(order)
        if ( chgs.wasPermutated )
          println("permutated")
        if ( chgs.wasAdded )
          println("added")
        if ( chgs.wasRemoved )
          println("removed")
        if ( chgs.wasReplaced )
          println("replaced")
        if ( chgs.wasUpdated )
          println("updated")
      }
    }
  }

  items.delegate.addListener(listener)

  println("appending")
  items.append("test")
  println("updating")
  items(0) = ("update")
}

The key is that original javafx listener is equivalent to Seq[Set[Change]] not to Seq[Change]. When two changes take place inside single step that means that changes are correlated. Added and Removed when correlated become Replaced. Replaced shows that item in fact was modified. It is very important for cost-significant computation performed when list is actually reducing or expanding.

@jpsacha
Copy link
Member

jpsacha commented Dec 18, 2014

I do not see here a problem with information loss as the issue title would indicate: "Observable buffer ignores update changes from javafx". The final content of the observable buffer can be recreated from the change notifications issued from ScalaFX. While change notification in JavaFX may be ambiguous: a single change item can have "added", "removed", and "replaced" flags at the same time. ScalaFX clarifies change notification, so a change is only one of "Add", "Remove", or "Reorder" (permute). Note that there is no change of type "Replace" in ScalaFX to avoid confusion with "Add"/"Remove" combination.

ScalaFX approach lets you write change handler with pattern matching easily (as you did):

change match {
  case Add(position, added)             => ...
  case Remove(position, removed)        => ...
  case Reorder(start, end, permutation) => ...
}

In other words, while JavaFX and ScalaFX change notifications equivalent, details are not the same. There are changes made in ScalaFX to make them less ambiguous and easier to deal with. For instance, there is not strange liked list to iterate through (in JavaFX the argument to change is a head of a liked list, it is both an item change description and link to other items, in ScalaFX those two concepts are separate). JavaFX have 5, not mutually exclusive (ambiguous) change flags: "added", "permurated", "removed", "replaced", "updated". The 5 can be replaced by 3 mutually exclusive: "added, "removed", "permuted".

Possible item of discussion I see is whether ScalaFX should have "Replace" as possible change when of "Remove"/"Add" is at the same location. That is having only "Replace" instead of "Remove"/Add" pair. Similar goes for "Update". I would tend to stay with current solution rather than having more complex approach (additional 2 cases to deal with, I rather have 3 than 5).

jpsacha added a commit that referenced this issue Dec 18, 2014
@scalasolist
Copy link
Author

I finally worked out an example that generate change with empty change list:

import scalafx.collections.ObservableBuffer
import scalafx.application.JFXApp
import scalafx.Includes._
import javafx.collections.{ListChangeListener, ObservableList}
import javafx.collections.FXCollections
import javafx.beans.Observable

object TestMain extends JFXApp {
  val items : ObservableBuffer[ObservableList[String]] = new ObservableBuffer(
    FXCollections.observableArrayList[ObservableList[String]]( (elem : ObservableList[String]) => Array[Observable](elem) ) )

  items.onChange( (obs, chs) => {
    println("changed")
    for (change <- chs)
      change match {
        case ObservableBuffer.Add(_, _) => println("add")
        case ObservableBuffer.Remove(_, _) => println("remove")
        case ObservableBuffer.Reorder(_, _, _) => println("reorder")
      }
  } )

  println("appending")
  items.append( FXCollections.observableArrayList("test") )
  println("updating")
  items(0).addAll( ("update") )
}

@jpsacha
Copy link
Member

jpsacha commented Dec 19, 2014

Can you clarify what you expected to see compared to what is actually going on?

@jpsacha
Copy link
Member

jpsacha commented Dec 19, 2014

OK. I think I see a problem. Current implementation does not handle update situation. The add event generated (in JavaFX 2 only) in the code seems to be "noise" in JavaFX flags - it is generated but should not be used. Still I am interested to hear your opinion, in case there may be something else going on.

@scalasolist
Copy link
Author

onChange handler was called but change list was null. If I want to process only changes and not recalculate data based on entire list that would be problem.

What I got:

appending
changed
add
updating
changed

The frist and the fourth strings describe operations on the list. At the first step I populate observable list with new item and so was called handler (changed to the output) and it detects add event (add to the output) as expected.

Then I modify existing item. ObservableBuffer is created with explicitly specified delegate. Extractor method was specified for the delegate. Extractor is a hint for observable collection that each item holds bunch of observable values itself. So when this values are modified enclosing observable collection fires change event with the update type.

scalafx swallows the event. It still calls handler (changed to the output), but handler detects that no changes was provided.

Change list is useful for handling large collections when it is too costly to recalculate all data on single changes and would be appropriate to update only parts of dependent data.

The add/remove pair is the only exception of rule "one change chunk -> one modification" that javafx provides. It would be more concise to declare it as new type of event on scalafx level.

Something like this (omitting arguments and constructors)

trait ContentChange extends Change
trait Add extends ContentChange
trait Remove extends ContentChange
class Replace extends Add with Remove

jpsacha added a commit that referenced this issue Dec 19, 2014
@jpsacha jpsacha added the bug label Dec 19, 2014
@jpsacha jpsacha added this to the 2.2.*-R11 milestone Dec 19, 2014
@jpsacha
Copy link
Member

jpsacha commented Dec 19, 2014

There was a problem with "update" event being ignored, so I marked this issue as "bug". It is now fixed and snapshot build are published for testing: 2.2.67-R11-SNAPSHOT and 8.0.20-R7-SNAPSHOT.
Please let me know if there is now working correctly on your end.

As for the separate "replace" event. It should be moved to a new issue that would be marked as "enhancement" (?). This is rather about philosophy of representing change events than code behaving incorrectly. Just trying to avoid scope creep in this issue. It would be great to have some use cases that will illustrate need for "replace" and also how optimal handling code would look like. I am thinking to make "add" "remove" and "replace" to be exclusive to each other. Use cases will be critical here.

jpsacha added a commit that referenced this issue Jan 8, 2015
…ns full information about the changes.

The documentation for the "update" event does not state that there is information about the content that changes, only its location (from, to).
It cannot be assumed that `getAddedSubList` contains the update. The behaviour changed between JavaFX 2 and 8.
Provide fix that is in line with JavaFX 2 documantation but also works in JavaFX 8, so there are no uncessary diferences between versions.
@jpsacha jpsacha closed this as completed Jan 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants