On Wed, May 6, 2015 at 9:47 PM, Marc André Tanner <mat_AT_brain-dump.org> wrote:
> Thanks, this will require more time to properly review than I currently
> have. What follows are therefore only a few general remarks.
>
>> The code for the read/write loops communicating through a pipe with the
>> external process is very ugly. There must be a better way to implement
>> the pipe communication but I could not figure it out.
>
> This is where a proper libuv based main loop would come in handy. Ideally
> the editor core should remain responsive while the asynchronous I/O is beeing
> done. There should be some kind of progress indication and a way to cancel
> those long running tasks. Also while doing so the text should essentially
> be marked read only.
I agree that having a responsive editor is of utmost importance and a
progress indication would be nice to have. What I am not so sure about
is the following points:
* If the text is marked read only anyways, is it actually useful to
have a "responsive" editor at all?
* Measuring progress is hard when you do not know the goal. In the sed
case you can only guess how much progress has been done by comparing
the count of input and output bytes for example. Even then, since you
can do something like 's/a/AAAAAAAAAAAA/g' there is no direct
correspondance. That said, there is the 'pv'[0] program that does
something similar. I assume it just checks the amount of data sent
through the pipe (but depending on the buffering and processing speed
of the output end of the pipe, predictions on running time will
probably be quite inaccurate)?
* Is it worth adding the libuv dependency in that case?
>> This approach uses a unnecessary amount of memory too since it keeps a lot
>> of the input data for the external process
>
> This should not be necessary, it should be possible to repeatedly call
> text_range_write (ignoring for now that it always takes a snapshot) with
> a resonable range which is adjusted each time.
>
>> as well as its output data in memory at the same time
>
> Similarly it should be possible to insert the data from the external process
> as it becomes available. One idea is to write the new data after the
> specified range which is being written out.
This is of course true. At the same time that would mean that the
external command and the text_range_write have to be called several
times which has some overhead (especially with big ranges of course)
while requiring more elaborate logic. It's a tradeoff.
> Assuming we want to process the range [10, 100] it would work as follows:
>
> 1) text_snapshot
>
> 2) while (data to write or data to read)
> if (data to write)
> write range [10, 20] (on next iteration [20, 30] etc ...)
> if (data to read)
> read range and insert into [100, 110] ([110, 120], ...)
>
> 3) read command exit status depending on success
> - either delete original range [10, 100] and snapshot
> - or revert to last snapshot (text_undo)
I think that would work but in case that there is some text after
position 100 already you would have to either
1. handle the existing text by moving it on each insertion
2. or move the text out of the way first and re-insert it at the end
of the newly added text after the external command is done.
Thanks for taking the time to review the patch(es)! I'm sure I will
learn something from it... :)
[0]
http://www.ivarch.com/programs/pv.shtml
Received on Thu May 07 2015 - 10:15:34 CEST