Code review comment for lp:~novalis/inkscape/fix-167335

Revision history for this message
Martin Owens (doctormo) wrote :

I've done a full review of the code and it's features:

The code looks good, using the right syntax as much as I can fathom and doesn't add anything too bad.

The method to fix the bug is interesting and I wanted to see if the fix worked:

 1. Open an existing flowText svg: OK
 2. Save svg back to a new file: FAILED (SEGFAULT)
  a. Fixed by commenting out line 180 is the diff below.
  b. This then caused a SEGFAULT after the svg had been saved
 3. The new svg from the old file produces better and more compatible results: NO
  a. SVG file in text editor shows switch statement: YES
  b. New svg with switch statement improved in librsvg: NO, text does not appear
  c. New svg with switch statement improved in firefox: NO, text does not appear
  d. New svg with switch statement improved in chrome: NO, text does not appear
 4. Create new svg file and add a flowText to the document: OK
 5. Save document and test in browsers: FAILED (no text shown again)
 6. Opening svg file with switch statement for editing: OK
 7. flowText object appears as flowText object: NO (shows as conditional group)

Thus this branch does not yet provide a fix for the issue of flowedText and can not be recommended for Merge.

review: Needs Fixing (full)

« Back to merge proposal