gEDA

  1. Bug #700448
  2. Comment #9

Comment 9 for bug 700448

Revision history for this message
Peter TB Brett (peter-b) wrote :

Hi Eivind,

I've now had time to do a more in-depth review of your patch (looking at the latest commit on your github branch, http://goo.gl/tJW63), and I have a few comments. I'm afraid I still haven't been able to actually go through it on a line-by-line basis, though, so there might be some remaining issues that I've missed. Sorry. :-(

- Please indent with spaces, not tabs.

- In o_complex_prepare_place(), the correct thing to do if the symbol can't be loaded is to abort the symbol placement operation and show an error dialog box to the user.

- I personally am not a fan of generic_msg_dialog(), because it usually doesn't show the correct message dialog variant (warning/error/info etc) for the type of message to be displayed. Consider using a GtkMessageDialog directly.

- For the preview, would it be possible not to pop up a modal dialog and instead show the error message in the preview pane somehow? Displaying a preview is not a "critical operation" such that failure should interrupt what the user is doing. By contrast, if the user attempts to open the file for viewing/editing, that load *is* a critical operation and a message dialog would be exactly the right thing. What do you think?

- I'm not convinced that EDA_ERROR_READ is the best possible name for this error. What about EDA_ERROR_PARSE or something that similarly describes the source of the error more specifically? To me, EDA_ERROR_READ suggests that an error occurred in actually *reading* the data from storage/network... It's also not clear why you're using EDA_ERROR_READ in some places and G_FILE_ERROR_FAILED in others.

- Similarly, you've chosen "invalid-string" as the key symbol for the corresponding Scheme error. "Invalid" has quite a large number of interpretations. Consider something like "string-format" or a name that otherwise specifically identifies the class of error that you're looking for.

This might be a good opportunity to also standardise the formatting and wording of the errors emitted during file loading.

Thanks for your hard work on this. I appreciate that the existing file loader code is both slightly baroque and also spread across a bunch of different files, and that fixing it up is a non-trivial exercise!

                               Peter