CSV export should output field names as first record in the output

RESOLVED FIXED

Status

RESOLVED FIXED
16 years ago
10 years ago

People

(Reporter: smeredith, Assigned: standard8)

Tracking

({fixed-seamonkey1.1a, fixed1.8.1})

Trunk
fixed-seamonkey1.1a, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

16 years ago
It is common for an application to export the field names as the first record
when  exporting to csv. This helps when it is time to import the file to some
other application because it is common for apps to show the first record when
setting up the import fields (as Mozilla does.) 

Some application give the user the choice of exporting the field names as the
first record. This is good, but not required since you almost always want it,
and if not, it's pretty easy to delete the first record by hand after the fact
(it's just a plain text file after all.)
(Reporter)

Comment 1

16 years ago
The code to do this already exist, but is commented out with this note:

// when Outlook Express exports to .csv, the first line of the file is the
column descriptions.
// and I think we should too.  (otherwise, this data is just data, who knows
what is what.)
// but we have to fix the import code to skip the first card before we can do that.
// the problem is Netscape 4.x didn't this, so if we always skip the first card
// we'll lose data when importing from 4.x
// for now, do it how 4.x does it and how the current import code expect it.

I think we should enable this feature, but not change the import code. This will
result in one extra record if you export the address book to .csv and re-import
it. The extra record will just have a field name in each value. That's not
ideal, but better than what we have now. The user can just delete the extra record.
(Reporter)

Comment 2

16 years ago
Actually, it would be great if the import UI had a check box to ask if the first
record contained the field names or not. If it did, we'd just skip it when
importing.
mass re-assign.
Assignee: racham → sspitzer
Product: Browser → Seamonkey
(Assignee)

Comment 4

14 years ago
*** Bug 221561 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

14 years ago
Blocks: 266691

Comment 5

14 years ago
I see the same problem. Before the function is added, I create a card and fill 
all the names myself.

Updated

14 years ago
Assignee: sspitzer → mail
(Assignee)

Comment 6

14 years ago
I'm taking a look at sorting this out (including the skip of the first line when
importing). Taking bug so that people know.
Assignee: mail → bugzilla
(Assignee)

Comment 7

14 years ago
Created attachment 188557 [details] [diff] [review]
Add skip first record option to import

Ok, this is the first of two patches. If we're going to allow exporting the
field names as the first record in a csv file, we should also allow skipping it
when we import it.
Attachment #188557 - Flags: review?(bienvenu)
(Assignee)

Comment 8

14 years ago
Comment on attachment 188557 [details] [diff] [review]
Add skip first record option to import

opps, sorry I forgot to update the uuid on the nsIImportFieldMap header. I'll
correct it now.
Attachment #188557 - Flags: review?(bienvenu)
(Assignee)

Comment 9

14 years ago
Created attachment 188559 [details] [diff] [review]
Add skip v2

Ok, this fixes the uuid, sorry about that. Also forgot to mention that this
works for both sm & tb.
Attachment #188557 - Attachment is obsolete: true
Attachment #188559 - Flags: review?(bienvenu)

Comment 10

14 years ago
NS_IMETHODIMP nsImportFieldMap::GetSkipFirstRecord(PRBool *aSkipFirstRecord)
+{
+    NS_ENSURE_ARG_POINTER(aSkipFirstRecord);
+
+    *aSkipFirstRecord = m_skipFirstRecord;
+
+    return NS_OK;
+}
+NS_IMETHODIMP nsImportFieldMap::SetSkipFirstRecord(PRBool aSkipFirstRecord)
+{
+    m_skipFirstRecord = aSkipFirstRecord;
+    return NS_OK;
+}
+

can just use NS_IMPL_GETSET...

Is there any way to avoid having UI for this? It would also be a drag to force
the user to click a checkbox when importing a file exported by sm/tb in order to
import it correctly...I guess there's no way to distinguish the first records as
field names.

Does this UI only apply to importing csv? If we have to have this UI, I think we
need more descriptive text explaining why the user want might to skip the first
record, something like "First record contains field names" or something
friendlier...
(Assignee)

Comment 11

14 years ago
As discussed with David on IRC, there's no easy way to avoid a UI for allowing
the user to select whether or not the first row contains field names on a
csv/tab/txt file - the user must have the choice.

It then follows that when we export, we must also give the user a UI to choose
if they wish to add field names to the file. This means a new dialog but we
don't see how we can avoid either of these.
(Assignee)

Comment 12

14 years ago
Comment on attachment 188559 [details] [diff] [review]
Add skip v2

Removing obsolete review request.
Attachment #188559 - Attachment is obsolete: true
Attachment #188559 - Flags: review?(bienvenu)
(Assignee)

Comment 13

14 years ago
Created attachment 188582 [details] [diff] [review]
Add Skip to import v3

Addressed previous review comments and improve the visible name of the field.
Attachment #188582 - Flags: review?(bienvenu)

Updated

14 years ago
Attachment #188582 - Flags: review?(bienvenu) → review+
(Assignee)

Updated

14 years ago
Attachment #188582 - Flags: superreview?(neil.parkwaycc.co.uk)

Comment 14

14 years ago
Comment on attachment 188582 [details] [diff] [review]
Add Skip to import v3

>+#include "nsISupportsObsolete.h"
This is spurious, I hope!

>+    PRBool     firstRecord = PR_TRUE;
>+    PRBool     skipFirstRecord = PR_FALSE;
I thought your boolean logic was unnecessarily tortuous. Compare:
PRBool skipRecord;
m_fieldMap->GetSkipFirstRecord(&skipRecord);
while (...) {
  ... ReadRecord(...) ...
  if (!skipRecord)
    ProcessLine(...);
  // /* the */ else /* is optional */
  skipRecord = PR_FALSE;
}
Or alternatively
PRBool skipRecord;
m_fieldMap->GetSkipFirstRecord(&skipRecord);
if (skipRecord)
  ReadRecord(...);
while (...) {
  ... ReadRecord(...) ... ProcessRecord(...) ...
}
sr=me if you can come up with some nicer logic.
Attachment #188582 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
(Assignee)

Comment 15

14 years ago
(In reply to comment #14)
> (From update of attachment 188582 [details] [diff] [review] [edit])
> >+#include "nsISupportsObsolete.h"
> This is spurious, I hope!

Well, David wanted me to use NS_IMPL_GETSET, which resides in that header, the
header resides only in xpcom/base - no info about it being obsolete or being
removed at any time (nothing in bugzilla) other than the name.... I'll remove it
if you want, but I'll have to revert the code back to what it was (or similar)
in comment 10.
(Assignee)

Comment 16

14 years ago
Created attachment 188781 [details] [diff] [review]
Add Skip to import v4 (checked in)

This patch addresses the sr nits, Neil said over IRC it's ok to leave
NS_IMPL_GETSET as it is.

Carrying forward David's r and Neil's sr.

Requesting approval - low risk, affects tb & sm, a couple of l10n string
additions (one each).
Attachment #188582 - Attachment is obsolete: true
Attachment #188781 - Flags: superreview+
Attachment #188781 - Flags: review+
Attachment #188781 - Flags: approval1.8b3?
Attachment #188781 - Flags: approval-aviary1.1a2?
(Assignee)

Comment 17

14 years ago
Comment on attachment 188781 [details] [diff] [review]
Add Skip to import v4 (checked in)

Opps, try setting the right title for the patch ;-)
Attachment #188781 - Attachment description: Add Skip to import v3 → Add Skip to import v4
(Assignee)

Comment 18

14 years ago
Comment on attachment 188781 [details] [diff] [review]
Add Skip to import v4 (checked in)

looks like 1.8b3/avairy1.1a2 have passed, requesting approval for 1.8b4, same
comments as previous apply.
Attachment #188781 - Flags: approval1.8b4?
Attachment #188781 - Flags: approval1.8b3?
Attachment #188781 - Flags: approval-aviary1.1a2?

Updated

14 years ago
Attachment #188781 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 19

14 years ago
Comment on attachment 188781 [details] [diff] [review]
Add Skip to import v4 (checked in)

Patch checked in:
/cvsroot/mozilla/mailnews/mailnews.js,v  <--  mailnews.js
new revision: 3.245; previous revision: 3.244
Checking in mailnews/import/public/nsIImportFieldMap.idl;
/cvsroot/mozilla/mailnews/import/public/nsIImportFieldMap.idl,v  <-- 
nsIImportFieldMap.idl
new revision: 1.5; previous revision: 1.4
Checking in mailnews/import/resources/content/fieldMapImport.js;
/cvsroot/mozilla/mailnews/import/resources/content/fieldMapImport.js,v	<-- 
fieldMapImport.js
new revision: 1.19; previous revision: 1.18
Checking in mailnews/import/resources/content/fieldMapImport.xul;
/cvsroot/mozilla/mailnews/import/resources/content/fieldMapImport.xul,v  <-- 
fieldMapImport.xul
new revision: 1.21; previous revision: 1.20
Checking in mailnews/import/resources/locale/en-us/fieldMapImport.dtd;
/cvsroot/mozilla/mailnews/import/resources/locale/en-us/fieldMapImport.dtd,v 
<--  fieldMapImport.dtd
new revision: 1.7; previous revision: 1.6
Checking in mailnews/import/src/nsImportFieldMap.cpp;
/cvsroot/mozilla/mailnews/import/src/nsImportFieldMap.cpp,v  <-- 
nsImportFieldMap.cpp
new revision: 1.16; previous revision: 1.15
Checking in mailnews/import/src/nsImportFieldMap.h;
/cvsroot/mozilla/mailnews/import/src/nsImportFieldMap.h,v  <-- 
nsImportFieldMap.h
new revision: 1.5; previous revision: 1.4
Checking in mailnews/import/text/src/nsTextAddress.cpp;
/cvsroot/mozilla/mailnews/import/text/src/nsTextAddress.cpp,v  <-- 
nsTextAddress.cpp
new revision: 1.47; previous revision: 1.46
Checking in mailnews/import/text/src/nsTextImport.cpp;
/cvsroot/mozilla/mailnews/import/text/src/nsTextImport.cpp,v  <-- 
nsTextImport.cpp
new revision: 1.36; previous revision: 1.35
Checking in mail/locales/en-US/chrome/messenger/fieldMapImport.dtd;
/cvsroot/mozilla/mail/locales/en-US/chrome/messenger/fieldMapImport.dtd,v  <-- 
fieldMapImport.dtd
new revision: 1.5; previous revision: 1.4
Attachment #188781 - Attachment description: Add Skip to import v4 → Add Skip to import v4 (checked in)
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Whiteboard: Import fixed, export to do.
(Assignee)

Updated

13 years ago
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
(Assignee)

Updated

13 years ago
No longer blocks: 266691
(Assignee)

Comment 20

13 years ago
*** Bug 266691 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 21

13 years ago
*** Bug 116975 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 22

13 years ago
I've just checked with someone on #seamonkey and apparently in czech outlook express, field names are in czech, therefore when I do the export for field names, I'll need to pull the translated versions.
OS: Windows XP → All
QA Contact: nbaca → addressbook
Hardware: PC → All
(Assignee)

Comment 23

13 years ago
Created attachment 210363 [details] [diff] [review]
Add Field Names to first line of output v1

This adds the exporting of field names as the first line of the csv/tab export. Its a bit of an ugly way of doing it, but I'm not quite ready to re-write our export code yet ;-)

As I touched on before in this bug, this code also re-uses the localized string names from the import code when adding the first line.
Attachment #210363 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #210363 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #210363 - Flags: review?(neil)

Comment 24

13 years ago
Comment on attachment 210363 [details] [diff] [review]
Add Field Names to first line of output v1

I haven't actually tried this yet.

>+  {kCategoryColumn, MOZ_AB_LDIF_PREFIX "Category", PR_FALSE, 0},
Nit: Trailing 0 is the default, isn't it?

>     if (EXPORT_ATTRIBUTES_TABLE[i].includeForPlainText) 
Nit: Could use .resourceStringId (nit: should be ID; maybe name it .plainTextStringID?) here and elsewhere thus eliminating includeForPlainText.

>+      if (NS_FAILED(bundle->GetStringFromID(EXPORT_ATTRIBUTES_TABLE[i].resourceStringId, getter_Copies(columnName))))
>+      {
>+        columnName.AppendInt(EXPORT_ATTRIBUTES_TABLE[i].resourceStringId);
Nit: You don't clear the previous value.

Comment 25

13 years ago
Comment on attachment 210363 [details] [diff] [review]
Add Field Names to first line of output v1

r=me with my nits fixed.
Attachment #210363 - Flags: review?(neil) → review+
(Assignee)

Comment 26

13 years ago
Created attachment 211183 [details] [diff] [review]
Add Field Names to first line of output v2

Addressed Neil's nits. Note it was decided on IRC that we don't need the truncate that Neil mentioned as getter_Copies will do this for us. Added comment to that effect as well.
Attachment #210363 - Attachment is obsolete: true
Attachment #211183 - Flags: superreview?(bienvenu)
Attachment #211183 - Flags: review+

Comment 27

13 years ago
Comment on attachment 211183 [details] [diff] [review]
Add Field Names to first line of output v2

>+      importService->SystemStringFromUnicode(columnName.get(), revisedName);
biesi seems to think that this is the same as nsMsgI18NCopyUTF16ToNative from nsMsgI18n.h in which case this deserves a followup bug to switch these for some good cleanup.

Comment 28

13 years ago
Comment on attachment 211183 [details] [diff] [review]
Add Field Names to first line of output v2

+      if (NS_FAILED(bundle->GetStringFromID(EXPORT_ATTRIBUTES_TABLE[i].plainTextStringId, getter_Copies(columnName))))
+      {
+        columnName.AppendInt(EXPORT_ATTRIBUTES_TABLE[i].plainTextStringId);
+      }

don't need braces here.
Attachment #211183 - Flags: superreview?(bienvenu) → superreview+
(Assignee)

Comment 29

13 years ago
Created attachment 212136 [details] [diff] [review]
Add Field Names to first line of output v3 (checked in)

Updated patch for checkin.
Attachment #211183 - Attachment is obsolete: true
Attachment #212136 - Flags: superreview+
Attachment #212136 - Flags: review+
(Assignee)

Comment 30

13 years ago
Comment on attachment 212136 [details] [diff] [review]
Add Field Names to first line of output v3 (checked in)

Checked into trunk:

Checking in mailnews/addrbook/src/nsAddressBook.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsAddressBook.h,v  <--  nsAddressBook.h
new revision: 1.40; previous revision: 1.39
done
Checking in mailnews/addrbook/src/nsAddressBook.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsAddressBook.cpp,v  <--  nsAddressBook.cpp
new revision: 1.148; previous revision: 1.147
Attachment #212136 - Attachment description: Add Field Names to first line of output v3 → Add Field Names to first line of output v3 (checked in)
(Assignee)

Comment 31

13 years ago
Comment on attachment 212136 [details] [diff] [review]
Add Field Names to first line of output v3 (checked in)

Requesting branch approval for this patch. Should be low risk. Adds extra bit of functionality when exporting address books.
Attachment #212136 - Flags: approval-branch-1.8.1?(mscott)
(Assignee)

Comment 32

13 years ago
All checked in -> FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

13 years ago
Created attachment 213240 [details] [diff] [review]
Add Field Names v4 updated for nits (checked into trunk and branch)

This patch addresses Neil's review comment about Id being ID that I missed on original review.

Re-requesting approval for the 1.8.1 branch with the new patch.
Attachment #212136 - Attachment is obsolete: true
Attachment #213240 - Flags: approval-branch-1.8.1?(mscott)
Attachment #212136 - Flags: approval-branch-1.8.1?(mscott)

Updated

13 years ago
Attachment #213240 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
(Assignee)

Updated

13 years ago
Attachment #213240 - Attachment description: Add Field Names v4 updated for nits (checked into trunk) → Add Field Names v4 updated for nits (checked into trunk and branch)
(Assignee)

Updated

13 years ago
Keywords: fixed-seamonkey1.1a, fixed1.8.1
Whiteboard: Import fixed, export to do.
(Assignee)

Comment 34

13 years ago
*** Bug 343305 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 35

12 years ago
*** Bug 357385 has been marked as a duplicate of this bug. ***
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.