chrisosaurus / dodo

scriptable in place file editor
MIT License
4 stars 1 forks source link

add relative offsets #22

Open chrisosaurus opened 8 years ago

chrisosaurus commented 8 years ago

currently byte is absolute (SEEK_SET), we should also support relative forward (SEEK_CUR) and ideally relative back.

proposed syntax:

bn (absolute byte n)
b+n (relative n bytes forward)
b-n (relative n bytes back)

we should centralise this logic for re-use across all commands taking numbers where this makes sense.

chrisosaurus commented 8 years ago

possible interface for centralising this:

parse_number returns a structure which has both a magnitude and mode (abs, forward-rel, back-rel) calc_number takes a the current number (byte or line dep. on command) and this number struct, and returns the new number

e.g.

calc_number( 4, {3, abs} )            => 3
calc_number( 4, {3, forward-rel} )    => 7
calc_number( 4, {3, back-rel} )       => 4

this is advisory calc_number is not the recommend name, nor are the values 'abs', 'forward-rel', or 'back-rel' necessarily sane, please take this as illustrative only.

chrisosaurus commented 8 years ago

although on second thought, this calc_number may not be needed, as it may be better to perform different operations in each case (e.g. line forward vs. line abs), time will tell :)

phillid commented 8 years ago

The syntax proposed seems logical.

Line offsets will prove a little bit more interesting than byte offsets of course. Using a "current line number" would get difficult. Imagine using a back-relative byte command sufficient to travel back past the start of the current line. Detecting such a case and correcting the stored "current line number" would require expensive work rather than simply fseek()ing.

chrisosaurus commented 8 years ago

@phillid good point, having to track current line across byte seeks is gross however a relative line offset can never just do a seek

so for relative byte you combine with current byte to do an fseek set (or do a relative seek instead) for relative line you have to use the existing scan logic, which gets around the need for keeping current line.

Very good points, thanks :)

chrisosaurus commented 8 years ago

and therefore the logic proposed above doesn't need to be as widespread, parsing logic should still be shared though :+1:

phillid commented 8 years ago

I'm thinking the tidiest option is to add a mode to the Argument struct and have parse_line set this accordingly… is there a tidier solution that I am missing?

chrisosaurus commented 8 years ago

@phillid that seems very reasonable to me, then parse_line and parse_byte both call parse_number which can set that mode (check or '+' or '-')

I don't think we need to track current line offset, as that will be too annoying to maintain across byte seeks, I am happy for an absolute line jump to always require a seek from the start of file.

chrisosaurus commented 8 years ago

@phillid I also think as part of this I am going to change line to be 0-based rather than 1-based, as I think it is gross to have byte 0-based, and to have +1 move forward one line but abs 1 to be line 0

chrisosaurus commented 8 years ago

@phillid please review my changes to make lines 0-based in https://github.com/mkfifo/dodo/tree/zero-based-lines

chrisosaurus commented 8 years ago

@phillid I also have some preliminary work for this relative offset work in https://github.com/mkfifo/dodo/tree/relative-offsets

it would be great if you could look at it.

TODO: add documentation (mostly user, but some internal too) TODO: add RELATIVE_BACKWARD support for line

phillid commented 8 years ago

I merged your relative-offsets branch into my branch of the same name and made some work on top of it in order to support back-relative offsets. I also removed the distinction between RELATIVE_BACKWARD and RELATIVE_FORWARD. My own original implementation of line-relative seeking didn't use this distinction, instead simply storing the offset with its sign to represent the direction.

This makes the maths simpler in most cases, dropping the need to manually negate argument.num.

I feel as if implementing forward and backward line seeking separately as I have is icky; I'm always afraid of writing code for special cases like this. There must surely be a link between the two problems of back- and forward-relative line seeking that I am missing. Let me know what you think regardless.

phillid commented 8 years ago

Okay, no, my implementation of backward line seeking doesn't quite handle l-1 to seek to line 0 properly. I will fix this and write some more tests to break it.