KDAB/knut

Refactor the text range/mark API

narnaud opened this issue · 5 comments

This is related to #54 , we currently have 4 different classes to handle position in text:

  • simple int: that's the general position in a TextDocument
  • TextRange: simple classes with start/end position
  • TextLocation: contains a TextRange + a document
    • use only for CodeDocument::references
    • shouldn't be documented as it's not even public API
  • Mark: position in a document, which is updated based on edits
  • RangeMark: as the name suggest, that's a range of 2 marks, for start/end
    • very similar of TextRange and TextLocation, except for the auto update

I would like to reduce to 3 different classes:

  • simple int: we keep this one for recording script, it's perfect
  • Cursor: position in a document (ex Mark)
  • CursorRange: range of cursor (ex RangeMark)

Cursor is a better term and more explicit, and I don't think we need the non-document aware version:

  • either you want it for reading something, and it's not a problem
  • or you want to keep it longer after edit, meaning you want updates.

@LeonMatthesKDAB What do you think? would it make sense?

API-wise this sounds good, although I don't know if there really is a need to rename Mark to Cursor.
Just removing TextRange and TextLocation is probably good enough.

Before refactoring this, we should also check how many TextRange and TextLocation instances are actually used in Knut.
Mark and RangeMark have a slightly higher impact on performance, as they listen to all changes in a document and update their position.
I doubt it will be an issue, but maybe we can benchmark some of our scripts to see whether there is a noticeable difference.

TextLocation is only used by Code::references, so no problem here.

`TextRange it's a bit more complex:

  • use internally by TextLocation
  • use by Symbols: but symbols are invalidated when the document is changed...
  • use for some API in TextDocument: selectRange, replace and deleteRange => we could use a RangeMark
  • use internally by CodeDocument => I think we could do without it

Creation shouldn't be a problem in term of performances (there's a shared_ptr + a connect, but it's not really that big). I don't think updates will be noticeable either... we could try and benchmark.

This isn't really fixed with #84, right? Let's not yet close this issue

Oups, #84 was meant for #69 sorry