List should support removeFirst [patch]

[DartBot]

This issue was originally filed by [email protected]

---

What steps will reproduce the problem?

1. html::Nodelist supports the ability get to the first node with a property called "first", but doesn't support the ability to remove it. Much as there is a matching 'removeLast', I'd like to see removeFirst.

Note that I've missed something in this patch because it doesn't actually case my NodeList in frog to grow this method.

I think that having "*first" methods on a Nodelist is going to be important for JS-based implementations. The List wrapper makes aesthetic sense, but it maps somewhat poorly to the underlying JS (as seen by the number that are sitting unimplemented).

jeffbailey@cirrostratus:~/Programming/gclienttree/dart/dart$ svn diff
Index: client/html/src/Element.dart
===================================================================

--- client/html/src/Element.dart (revision 1856)
+++ client/html/src/Element.dart (working copy)
@@ -5,6 +5,7 @­@
 interface ElementList extends List<Element> {
   // TODO(jacobr): add element batch manipulation methods.
   Element get first();

• Element removeFirst();
     // TODO(jacobr): add insertAt
   }
   
  Index: client/html/src/ElementWrappingImplementation.dart
  ===================================================================

--- client/html/src/ElementWrappingImplementation.dart (revision 1856)
+++ client/html/src/ElementWrappingImplementation.dart (working copy)
@@ -132,6 +132,15 @­@
     _element.textContent = '';
   }
 

• Element removeFirst() {
• // Use 'node' instead of 'first' to avoid confusion.
• final node = this.first;
• if (node != null) {

•  _element.removeChild(LevelDom.unwrap(node));
  

• }
• return node;
• }

   Element removeLast() {
     final last = this.last();
     if (last != null) {
@@ -243,6 +252,10 @­@
     throw 'Not impl yet. todo(jacobr)';
   }
 

• Element removeFirst() {
• throw 'Not impl yet.';
• }

   Element removeLast() {
     throw 'Not impl yet. todo(jacobr)';
   }
Index: client/html/src/NodeList.dart
===================================================================

--- client/html/src/NodeList.dart (revision 1856)
+++ client/html/src/NodeList.dart (working copy)
@@ -4,4 +4,5 @­@
 
 interface NodeList extends List<Node> {
   Node get first();

• Node removeFirst();
   }
  Index: client/html/src/NodeWrappingImplementation.dart
  ===================================================================

--- client/html/src/NodeWrappingImplementation.dart (revision 1856)
+++ client/html/src/NodeWrappingImplementation.dart (working copy)
@@ -136,6 +136,15 @­@
     _node.textContent = '';
   }
 

• Node removeFirst() {
• // Use 'node' instead of 'first' to avoid confusion.
• final node = this.first;
• if (node != null) {

•  _node.removeChild(LevelDom.unwrap(node));
  

• }
• return node;
• }

   Node removeLast() {
     final last = this.last();
     if (last != null) {
Index: client/html/src/DocumentFragmentWrappingImplementation.dart
===================================================================

--- client/html/src/DocumentFragmentWrappingImplementation.dart (revision 1856)
+++ client/html/src/DocumentFragmentWrappingImplementation.dart (working copy)
@@ -82,6 +82,15 @­@
     _childNodes.clear();
   }
 

• Element removeFirst() {
• // Use 'node' instead of 'first' to avoid confusion.
• var node = this.first;
• if (node != null) {

•  node.remove();
  

• }
• return node;
• }

   Element removeLast() {
     var last = this.last();
     if (last != null) {

[DartBot]

This comment was originally written by [email protected]

---

Added Area-UI, Triaged labels.

[DartBot]

This comment was originally written by [email protected]

---

Assigning per Jacob's request.

---

Set owner to @jacob314.

[DartBot]

This comment was originally written by [email protected]

[vsmenon]

Removed Area-UI label.
Added Area-DOM label.

[vsmenon]

I'll take a look at this, but it should just implement the core List interface.

---

Set owner to @vsmenon.
Added this to the M1 milestone.

[vsmenon]

Lasse: reassigning this to library. Should List support first and removeFirst (to match last and removeLast)?

We will update the dom NodeList, etc., accordingly.

---

cc @lrhn.
Removed the owner.
Removed Area-DOM label.
Added Area-Library label.
Changed the title to: "List should support removeFirst [patch]".

[DartBot]

This comment was originally written by @seaneagan

---

also addFirst (to match addLast)

[lrhn]

Whoops, missed this during vacation. Let me answer now.

I don't particularly like addFirst and removeFirst (and maybe addLast should be removed, it's equivalent to add). We already have insertAt and removeAt which can be used at position zero.

For the normal list, an addFirst is likely to be the most expensive operation possible on a list. On the other hand it'll be O(1) for a linked list, where indexed lookup is slow, so we might want to allow it anyway if we ever introduce a LinkedList implementation of List (but should it really be a List and not just a double-ended queue?)
It all depends on how one sees a list: If it's like a JS list, growing at the end, using the add() function, is natural, but doing it at the start is dangerous and slow, so it's not natural. If you don't wear those glasses, there really isn't any difference between the two ends, so we should support the same operations at either end.
And I think the latter is better - the accidental implementation of our lists is just that, and we should't bind our abstract interfaces to them.

I.e., I think we should add
 E get first;
 E removeFirst();
 E addFirst();
to List<E> with fair warnings that they might be slow for some List implementations, including the default implementation.

[lrhn]

Removed this from the M1 milestone.
Added this to the M2 milestone.

[floitschG]

Removed this from the M2 milestone.
Added this to the M3 milestone.

[anders-sandholm]

Removed this from the M3 milestone.
Added this to the M4 milestone.

[larsbak]

Removed this from the M4 milestone.
Added this to the M5 milestone.

[lrhn]

We have removed addLast, and have no current plans to add *First methods on list.
I.e., we will keep seeing a list as an upward-growing array, and not as any sequential data structure.

---

Added NotPlanned label.