search

Glucosio / glucosio-android

Glucosio Android App
GNU General Public License v3.0
338 stars 162 forks source link

Escape characters in CSV #397

Closed emartynov closed 6 years ago

emartynov commented 6 years ago

It is fix for #383.

Changes:

  1. Refactored CSV export
  2. Escaped '\n', '\r', '"', ',' when generating CSV
  3. Extracted one more string constant
  4. Added some missing tests

No changes here:

  1. Using async task
  2. Resume export after user allows write permission

Please review and comment.

emartynov commented 6 years ago

@AndyScherzinger can you review?

coveralls commented 6 years ago

Coverage Status

Coverage increased (+1.002%) to 20.148% when pulling a57e9d0efb6087b23016c106f606066c08a7b43a on 383-fix-for-csv-export into 54dd90f6fc135ab23c40ccd792d0c1e26f308dbd on develop.

AndyScherzinger commented 6 years ago

@emartynov sure, I'll give the branch a test drive :)

AndyScherzinger commented 6 years ago

Looks good to me code wise, just a minor comment. Will test the export later today after work.

AndyScherzinger commented 6 years ago

Like mentioned in the in line comment only " and ' seem to be escaped, line breaks still exist and won't be escaped. This leads to new lines which will be treated as new rows, e.g. the following export:

Date,Time,Concentration,Preferred Glucose unit,Measured,Notes
1/22/18,21:37,25,mg/dL,After dinner,"Yet
Another
'
""
Test"
1/22/18,21:36,23.5,mg/dL,After dinner,"This, is
A
New line
Test"

When imported into MS Excel (komma delimiter, "-String key): 2018-01-22 21_53_03-mappe1 - excel

So I would guess an in-app import of it would still fail (don't know how to do an import :/)

emartynov commented 6 years ago

We don't have import, only export :(

I was quickly checking https://tools.ietf.org/html/rfc4180. It looks like MS Excel is wrong with importing CSV.

I will dive more :)

AndyScherzinger commented 6 years ago

I also send it to my gmail adress and opened it as preview within gmail and there it works fine... so yeah, maybe we just have to ignore Excel...

emartynov commented 6 years ago

Conclusion on this PR?

AndyScherzinger commented 6 years ago

@emartynov fine by me and as discussed excel has its cvs issues but that is excel not glucose :)

AndyScherzinger commented 6 years ago

Hi @emartynov I tried to explain it again, sorry for not getting it clearly explained :( Hope this time I got it right :)

AndyScherzinger commented 6 years ago

All fine by me. Any feedback @paolorotolo?

paolorotolo commented 6 years ago

I'll try it ASAP

paolorotolo commented 6 years ago

Works fine in LibreOffice, good work

screenshot from 2018-01-29 15-35-52