Scala Programming Language
  1. Scala Programming Language
  2. SI-4461

ObservableBuffer fails to notify about some changes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: Scala 2.10.0-M3
    • Component/s: Misc Library
    • 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

        Activity

        Hide
        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
        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
        Aleksandar Prokopec added a comment -

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

        Show
        Aleksandar Prokopec added a comment - (In r24885) Fixes and closes SI-4461 . No review.
        Hide
        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
        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
        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
        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:
            Aleksandar Prokopec
            Reporter:
            Jean-Philippe Pellet
          • Votes:
            3 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development