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

ObservableBuffer fails to notify about some changes #4461

Closed
scabug opened this issue Apr 12, 2011 · 5 comments
Closed

ObservableBuffer fails to notify about some changes #4461

scabug opened this issue Apr 12, 2011 · 5 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Apr 12, 2011

=== What steps will reproduce the problem (please be specific and use wikiformatting)? ===

    import scala.collection.mutable._
    import scala.collection.script._

    val buf = new ArrayBuffer[Int] with ObservableBuffer[Int]
    buf.subscribe(new Subscriber[Message[Int], ObservableBuffer[Int]] {
      def notify(pub: ObservableBuffer[Int], event: Message[Int]) = println(event)
    })
    
    buf += 1 // works
    buf ++= Array(2) // works
    buf ++= ArrayBuffer(3) // fails

=== What is the expected behavior? ===
I expect to see three messages printed to the console:

Include(End,1)
Include(End,2)
Include(End,3)

=== What do you see instead? ===
Only the first two messages are printed:

Include(End,1)
Include(End,2)

=== Additional information ===
Comes from the overloaded ++= in ArrayBuffer, which handles IndexedSeqs differently. Maybe other overloaded methods prevent the ObservableXXX decorators from notifying property in other circumstances, too.

=== What versions of the following are you using? ===

  • Scala: 2.8.1
  • Java: 1.6.0_24-b07-334-10M3326
  • Operating system: Mac OS X 10.6.7
@scabug
Copy link
Author

scabug commented Apr 12, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4461?orig=1
Reporter: Jean-Philippe Pellet (jppellet)
Attachments:

@scabug
Copy link
Author

scabug commented Apr 18, 2011

@magarciaEPFL said:
Background info:

The overrides in ObservableBuffer currently do not include ++=. They currently are:

  abstract override def +=(element: A): this.type

  abstract override def +=:(element: A): this.type

  abstract override def update(n: Int, newelement: A): Unit

  abstract override def remove(n: Int): A

  abstract override def clear(): Unit

(some of these overrides are for BufferLike, others for Growable, and others for SeqLike).

In the example, the following callsite is not "observed"

 buf ++= ArrayBuffer(3) // fails

because the override in ArrayBuffer that gets picked is:

  /** Appends a number of elements provided by a traversable object.
   *  The identity of the buffer is returned.
   *
   *  @param xs    the traversable object.
   *  @return      the updated buffer.
   */
  override def ++=(xs: TraversableOnce[A]): this.type = xs match {
    case v: IndexedSeq[_] =>
      val n = v.length
      ensureSize(size0 + n)
      v.copyToArray(array.asInstanceOf[scala.Array[Any]], size0, n)
      size0 += n
      this
    case _ =>
      super.++=(xs)
  }

It would be great if a patch were provided (and tests run!). Additionally, are there any other mutator methods that don't get "observed"?

@scabug
Copy link
Author

scabug commented May 4, 2011

@axel22 said:
(In r24885) Fixes and closes #4461. No review.

@scabug
Copy link
Author

scabug commented Nov 4, 2011

@Blaisorblade said (edited on Nov 4, 2011 3:36:33 PM UTC):
According to BufferLike ScalaDocs, also insertAll needs to be overridden, but it's not in trunk. Not only that method is affected - it's called also by ++=: and insert. I've attached a runnable testcase+fix (IssueSI4461.scala). It compares the behavior trunk ObservableBuffer with another trait which also overrides insertAll correctly, confirming that the bug occurs - look at the console output and notice the difference between notifications of the two buffers. The implementation of insertAll could be added to ObservableBuffer directly.

Moreover, your implementation disables any optimized implementation. Would you consider a different fix? I'm giving it a shot.

An unrelated question - ArrayBuffer duplicates this definition of ++=: from BufferLike, and that's bad. Is the duplication needed because of some weird reason? If yes, I'd like to know, because it should be addressed.

override def ++=:(xs: TraversableOnce[A]): this.type = { insertAll(0, xs.toTraversable); this }

@scabug
Copy link
Author

scabug commented May 18, 2012

@axel22 said:
Fixed in scala/scala#574.

You can certainly submit a pull request if you think anything is missing - just add:
review by @axel22
review by @Prokopec
at the end of your pull request description

@scabug scabug closed this as completed May 18, 2012
@scabug scabug added this to the 2.10.0-M3 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants