Iterator peek next

[cgestes]

Please review. Some changes may be better done in another way.

This PR is needed to support Markdown auto formatting rules in Notus.

[cgestes]

clone and resetTo are needed to be able to store a position or go back in time.

[cgestes]

can this just be null or Operation.retain(-1) ?

[cgestes]

or anything that Operation can express as invalid

[cgestes]

it used to return Operation.retain(length) which sound like a hack to me.

can we return null or Operation.retain(-1). I mean anything invalid... (I understand null is probably very inpractical)

Zefyr/Notus needs very little adjustment after this changes. (4 lines needs to be changed) (and they become more understandable)

[pulyaevskiy]

I like this idea! I'm thinking we can use Operation.insert with a special data value, e.g. create constant and use it:

class _EOF {
  const _EOF();
}
const _eof = _EOF();

// Then in the iterator
return Operation.insert('insert', 1, _eof);

Can also add Operation.isEof to allow easy checking for this.

That said, I'd prefer for this change to be a separate PR, if you don't mind. There is already a lot of new changes here which makes it hard to review.

[pulyaevskiy]

Thanks this looks impressive!

It took me a while to digest everything here but I have a couple of concerns about the new APIs introduced by this change (specifically rewind is something I'd prefer to avoid).

I also believe we can achieve similar results with a bit simpler solution. It'd only require one additional method on the iterator - peek, which is similar to peekLength but returns the actual operation instead of just length. E.g.:

  Operation peek() {
    if (_index < delta.length) {
      final op = delta._operations[_index];
      if (_offset == 0) return op;
      return Operation._(op.key, op.length, op.data, op.attributes);
    }
    return Operation.retain(maxLength);
  }

Next we can implement both skipToLineAtIndex and skipToEndOfLine using existing methods of the iterator. One important change is that I suggest tweak the return type of these functions.

A semi-pseudo code of both of these is below:

/// Skips [length] characters in source delta and returns contents of the last
/// line up until [length].
/// The returned list of operations can be used to compose the text prefix pretty easily
///
/// Only makes sense to use on a document delta (containing only insert operations).
List<Operation> skipToLineAt(DeltaIterator iter, int length) {
  assert(length > 0);

  final prefix = <Operation>[];

  var skipped = 0;
  while (skipped < length && iter.hasNext) {
    final opLength = iter.peekLength();
    final skip = math.min(length - skipped, opLength);
    final op = iter.next(skip);
    if (op.data is! String) {
      prefix.add(op);
    } else {
      var text = op.data as String;
      var pos = text.lastIndexOf('\n');
      if (pos == -1) {
        prefix.add(op);
      } else {
        prefix.clear();
        prefix.add(Operation.insert(text.substring(pos + 1), op.attributes));
      }
    }
    skipped += op.length;
  }
  return prefix;
}

/// Skips to the end of current line and returns all skipped operations
/// including operation containing the newline character.
/// The returned list of operations can be used to find the total skipped length
///
/// Only makes sense to use on a document delta (containing only insert operations).
List<Operation> skipToEndOfLine(DeltaIterator iter) {
  assert(iter.hasNext);

  final result = <Operation>[];
  while (iter.hasNext) {
    final op = iter.peek();
    if (op.data is! String) {
      result.add(op);
      iter.next();
      continue;
    }
    final text = op.data as String;
    final pos = text.indexOf('\n');
    if (pos == -1) {
      result.add(op);
      iter.next();
      continue;
    }
    result.add(iter.next(pos + 1));
  }
  return result;
}

I still think that these two functions better to keep in Zefyr as these have a narrower use and only make sense for document deltas (only insert ops).

Sorry for dragging you into a different direction here, hope it makes sense. WDYT?

[pulyaevskiy]

This doesn't seem to be used anywhere.

[cgestes]

Left over from implementing peekNext 😅

[pulyaevskiy]

I like this idea! I'm thinking we can use Operation.insert with a special data value, e.g. create constant and use it:

class _EOF {
  const _EOF();
}
const _eof = _EOF();

// Then in the iterator
return Operation.insert('insert', 1, _eof);

Can also add Operation.isEof to allow easy checking for this.

That said, I'd prefer for this change to be a separate PR, if you don't mind. There is already a lot of new changes here which makes it hard to review.

[cgestes]

I can move the extension in Notus. No issue. I believe they should just exist somewhere cause they are a pain in the ass to write.

I did implement peek before but it makes the code that uses it hard to grok.

And peek now is:

Final newIter = iter.clone()
newIter.next()

Also I believe skipToBeginningOfLineBeforeIndex would be more optimised if we skip to index and then look backward. Otherwise we look for \n on every single line from start of doc.

[cgestes]

For EOF I like the idea too. Will implement in a future PR.
I came to it cause the doc on DeltaItarator.next was incorrect, so I reimplement by mistake what the doc was saying...then had to fix the test in Notus and noticed it was all making more sense 😅

[cgestes]

just simplification of the code. no logic changes

[codecov]

Codecov Report

Merging #26 (b8c13d5) into master (4846633) will decrease coverage by 10.48%.
The diff coverage is 43.75%.

❗ Current head b8c13d5 differs from pull request most recent head c950b1b. Consider uploading reports for the commit c950b1b to get more accurate results

@@             Coverage Diff             @@
##           master      #26       +/-   ##
===========================================
- Coverage   93.56%   83.08%   -10.49%     
===========================================
  Files           1        1               
  Lines         342      396       +54     
===========================================
+ Hits          320      329        +9     
- Misses         22       67       +45     

Impacted Files	Coverage Δ
lib/quill_delta.dart	83.08% <43.75%> (-10.49%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4846633...c950b1b. Read the comment docs.