Created attachment 141950 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 1

Created attachment 141951 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 2

At this stage, most of the parser code now lives in Glom::CsvParser. It compiles but I had to deactive the tree model creation/insertions (which of course renders it useless). I need to think a bit more about this.

Created attachment 142418 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 3

Created attachment 142419 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 4

Created attachment 142420 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 5

Created attachment 142421 [details] [review]
Refactoring Glom::Dialog_Import_CSV, part 6 (ChangeLog update only)

Created attachment 142422 [details] [review]
Refactoring Glom::Dialog_Import_CSV (complete patch set for convenience)

Regarding the decoupling of parsing and dialog components in Glom::Dialog_Import_CSV the refactoring so far was only partially successful. More signals might be the appropiate answer, since the critical code itself proved to be quite resistant towards refactoring. The next step however is to cut down on the numbers of member variables, then have some tests for the parser (more accurate: scanner).

(In reply to comment #6)
> Created an attachment (id=142421) [details]
> Refactoring Glom::Dialog_Import_CSV, part 6 (ChangeLog update only)

This single ChangeLog entry mentions the same file(s) repeatedly. If it's meant to be several ChangeLog entries then please write it as several entries. If not (if you want to push several smaller commits) then please write one ChangeLog entry correctly.

(In reply to comment #7)
> Created an attachment (id=142422) [details]
> Refactoring Glom::Dialog_Import_CSV (complete patch set for convenience)

Some initial superficial comments:

+  Parser(const char* encoding)
+  : conv("UTF-8",
+    encoding),
+    input_position(0),
+    line_number(0)
+  {

Please 
1. initialize member variables in the .cc file, not the .h file,
2. Use m_ for member variables,
3. Put method implementations in the .cc file, not the .h file.
as elsewhere in glom.
Oh, you did most of that already - see the comment below about this weird multi-commit patch.


+// The CsvParser will parse a CSV file by calling token handlers whenver the scanner recognizes a token.
+class CsvParser

Please use Doxygens /// or /** */ comment syntax.


 m_parser.reset(NULL);
+  //m_parser.reset(NULL);

Maybe this is old code, but you should avoid NULL in C++ code.



I think it would be clearer for me to see one patch rather than several commits in one "patch". The intermediate stuff doesnt seem that useful to me, and slows down my review.

> The CsvParser class learned a new signal, SignalLineScanned

Maybe you mean "gained" instead of "learned".


+/**  Parse a row from a .cvs file.

You mean a .csv file.

Created attachment 142462 [details] [review]
A properly rebased patch

The obvious thing that still needs fixing is to make the parser handle encoding changes from the UI. Apart from that, the import dialog should have the same functionality as before.

+2009-09-04  Michael Hasselmann <michaelh@openismus.com>
+
+	* Makefile_glom.am, glom/dialog_import_csv.[h|cc], glom/import_csv.[h|cc],
+	glom/dialog_import_csv_progress.cc: Moved the inlined Parser class (in
+	Glom::Dialog_Import_CSV) to glom/import_csv.h. The goal is to split this
+	class up in an import model and import dialog.  The parsing code (more
+	accurate: the scanning code) now lives Glom::CsvParser now, which still has
+	an all-public interface until the requirements for its API are clear.
+	Building the tree model from the scanned tokens (the actual parsing) is
+	still a responsibility of the Glom::Dialog_Import_CSV class because this
+	part is too dependent on the import dialog. However, it now gets its tokens
+	by handling the Glom::CsvParser's new lineScanned signal.  The
+	Glom::Dialog_Import_CSV_Progress class should propably not depend on the
+	parser's state but be connected to some signals instead.

You've just mashed your commit messages again instead of talking about the actual change.
1. You talk about a goal, but you actually did something, so say what you did.
2. You should talk about the files (well, the classes) individually.
3. Also, the mention of the Glom:: namespace is superfluous.


CsvParser still has implementation (initialization and method implementations) in the .h file.

Otherwise it looks fine. Please correct and commit.

The changes are committed, from Michael's github remote branch. Now we just need the tests.

This bug now has its own feature branch: http://git.gnome.org/cgit/glom/log/?h=import_csv_refactored

The logic for CsvParser::signal_line_scanned() changed with the latest commits: Instead of letting the dialog tokenize the lines it gets from the parser, putting the tokens in its tree model, we now only signal that we scanned and tokenized (that is, a more sophisticated scanning than just finding lines/rows) a line. Thus, the signal will only be useful for the progress bar.

The dialog is supposed to grab the data after a full parse run it seems (which is a change of how it worked before, line-wise). Of course, returning a vector of parsed rows wouldn't make sense here, so we offer an API for the parser's internal model (basically, a (m,n) table): CsvParser::get_data(...).

I hope I got that change of application logic right? If so, I'll move forward to make it work again with the dialog (it currently doesn't import anything).

The dialog should continue to read progressively - one row at a time. I think it can (and does) still do that because it is signalled by the CsvParser for each line. As I said, this is untested.

As we discussed elsewhere, it would be nice if the CsvParser did not need to store the entire data, but that can be a later optimization.

To make the tests usefule again you might want to add a save_to_temp_file_and_parse() utility function to your test code (not to the parser).

Looking at the dialog's get_row_count() (http://git.gnome.org/cgit/glom/tree/glom/import_csv/dialog_import_csv.cc?h=import_csv_refactored#n212) I think the API exposed a race condition there (now and before, too). Imagine a big file, and the async read hasn't finished yet before the user chooses to import. Then, theoretically speaken, the row count would still be too small, and a lot of the dialog's logic is based on the exact row count.

Well, to be honest, with the new API we aim for, I do not think the dialog needs to know the exact row count. Perhaps a parsing_finished signal would be more helpful.

Also, the only remaining client of that method is the progess bar. But we could replace that by a simple heuristic: "size of file / avg(size of first n rows)". We need the first n rows for the preview feature anyway. The progress bar would then show a % value, instead of trying to be exact with the max row count (which it can't).

The only way to get an exact row count (mind the newlines issues with CSV data) I can imagine is to parse the file twice (surely we want to avoid that?).

(On another note: the save_to_temp_file_and_parse() utility function is a good idea.)

That sounds sensible. Please do try to do that, though after you've fixed the tests.

I think this is dealt with, apart from the hang in the tests mentioned in the latest bug.