Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Rows-CSV format to SFM export #79

Merged
merged 3 commits into from
May 3, 2022
Merged

Conversation

darcywong00
Copy link
Contributor

@darcywong00 darcywong00 commented Mar 24, 2022

Fixes #74

To support users exporting from SA to Cog, this inserts an option "Rows Comma-Separated Values" to the SFM export dialog.
This option uses the , character to separate the entries instead of spaces .

Limitations

  • If a field contains a comma, the user will need to surround it in " quotes in order to prevent the field from getting split.
  • The resulting .csv file will need to be converted to UTF-8 BOM (e.g. in Notepad++)

@darcywong00
Copy link
Contributor Author

Some initial comments from @sdysart:

I took a look at the csv feature you added to SA.
The export as CVS option will really help.
I did notice that in the export, the phonetic row was off by one towards the end, that was because they had inserted a comma in one of the phonetic fields

I guess they had not decided how to proceed. I suppose that we can assume that the users will clean up the data prior to export, but it should be noted in the instructions somewhere
that comma's can't be in the exported fields if exporting as csv. It would be easily to fix. but it should be noted.

The export still exports as SFM, not csv. I had to rename it in File Explorer, which is easy enough to do.
Also, Excel doesn't seem to like the way SA exports IPA. It goes weird. When I went through the process of editing the sfm in Notepad++ Tim E. pointed out that I had to mark the encoding as UTF-8 BOM in Notepad++ in order for Excel to pick it up.
LibreOffice Calc asks me to pick the encoding when opening the csv file.

@megahirt
Copy link
Contributor

megahirt commented Mar 27, 2022 via email

@darcywong00
Copy link
Contributor Author

Best way to test is
to take the offending CSV that @sdysart mentions and put quotes around the
field in the line where the data has a comma. See if it then imports
correctly.

Yeah, we can try that next time. Though we'd also have to escape quotes within the string. For Excel, it might be ""?

@@ -255,17 +258,19 @@ void CDlgExportSFM::OnOK() {
m_bLanguage = m_bDialect = m_bSpeaker = m_bGender = m_bEthnologue = m_bFamily = m_bRegion = m_bNotebookRef =
m_bTranscriber = m_bComments = m_bCountry = TRUE;

m_szSeparator = (m_nExportFormat != 2) ? L" " : L",";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any value in having m_szSeparator as a class member. I'm thinking it should just be local scope.
I see why you did it, but it bothers me a little that the Export* methods are protected, meaning that they can be called by a derived class and end up with an un-initialized m_szSeparator. So either the Exports* could be private, or pass the separator in as a parameter.

Copy link
Contributor Author

@darcywong00 darcywong00 Apr 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. I've made the following updates:

  • Remove class member m_szSeparator
  • Updated the applicable Exports* to use an optional separator parameter, with a default separator L" ".

@darcywong00
Copy link
Contributor Author

We should be able to escape commas in the standard CSV way. I'm thinking
that just means that each field has quotes around it. Best way to test is
to take the offending CSV that @sdysart mentions and put quotes around the
field in the line where the data has a comma. See if it then imports
correctly.

@sdysart tested the latest build. We found if he surrounds a field in " quotes, the CSV doesn't split the line with a comma.

@darcywong00 darcywong00 marked this pull request as ready for review April 29, 2022 03:24
@darcywong00 darcywong00 merged commit eaa1c0d into master May 3, 2022
@darcywong00 darcywong00 deleted the feat/sfm-csv branch May 3, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Request to export CSV
3 participants