Closed Bug 180057 Opened 22 years ago Closed 18 years ago

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

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: smeredith, Assigned: standard8)

References

Details

(Keywords: fixed-seamonkey1.1a, fixed1.8.1)

Attachments

(2 files, 6 obsolete files)

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.)
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.
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
*** Bug 221561 has been marked as a duplicate of this bug. ***
I see the same problem. Before the function is added, I create a card and fill 
all the names myself.
Assignee: sspitzer → mail
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
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)
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)
Attached patch Add skip v2 (obsolete) — Splinter Review
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)
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...
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.
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)
Attached patch Add Skip to import v3 (obsolete) — Splinter Review
Addressed previous review comments and improve the visible name of the field.
Attachment #188582 - Flags: review?(bienvenu)
Attachment #188582 - Flags: review?(bienvenu) → review+
Attachment #188582 - Flags: superreview?(neil.parkwaycc.co.uk)
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+
(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.
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?
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
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?
Attachment #188781 - Flags: approval1.8b4? → approval1.8b4+
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)
Status: NEW → ASSIGNED
Whiteboard: Import fixed, export to do.
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
No longer blocks: 266691
*** Bug 266691 has been marked as a duplicate of this bug. ***
*** Bug 116975 has been marked as a duplicate of this bug. ***
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
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?
Attachment #210363 - Flags: review?(neil)
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 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+
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 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 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+
Updated patch for checkin.
Attachment #211183 - Attachment is obsolete: true
Attachment #212136 - Flags: superreview+
Attachment #212136 - Flags: review+
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)
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)
All checked in -> FIXED.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
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)
Attachment #213240 - Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
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)
Whiteboard: Import fixed, export to do.
*** Bug 343305 has been marked as a duplicate of this bug. ***
*** 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.

Attachment

General

Created:
Updated:
Size: