On Wed, May 6, 2015 at 9:47 PM, Marc André Tanner <[email protected]> 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
