Uploaded image for project: 'Scala Programming Language'
  1. Scala Programming Language
  2. SI-4461

ObservableBuffer fails to notify about some changes

    Details

    • Type: Bug
    • Status: CLOSED
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Scala 2.10.0-M3
    • Component/s: Library (Misc)
    • Labels:
      None
    • Environment:

      observable buffer

      Description

      === 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

        Attachments

          Activity

          Hide
          magarcia Miguel Garcia added a comment -

          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"?

          Show
          magarcia Miguel Garcia added a comment - 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"?
          Hide
          prokopec Aleksandar Prokopec added a comment -

          (In r24885) Fixes and closes SI-4461. No review.

          Show
          prokopec Aleksandar Prokopec added a comment - (In r24885) Fixes and closes SI-4461 . No review.
          Hide
          pggiarrusso Paolo G. Giarrusso added a comment - - edited

          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 }
          

          Show
          pggiarrusso Paolo G. Giarrusso added a comment - - edited 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 }
          Hide
          prokopec Aleksandar Prokopec added a comment -

          Fixed in https://github.com/scala/scala/pull/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

          Show
          prokopec Aleksandar Prokopec added a comment - Fixed in https://github.com/scala/scala/pull/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

            People

            • Assignee:
              prokopec Aleksandar Prokopec
              Reporter:
              jppellet Jean-Philippe Pellet
            • Votes:
              3 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: