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)
MailNews Core
Address Book
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)
12.96 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
asa
:
approval1.8b4+
|
Details | Diff | Splinter Review |
9.99 KB,
patch
|
mscott
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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•22 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•22 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.
Updated•20 years ago
|
Product: Browser → Seamonkey
Assignee | ||
Comment 4•19 years ago
|
||
*** 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.
Updated•19 years ago
|
Assignee: sspitzer → mail
Assignee | ||
Comment 6•19 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•19 years ago
|
||
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•19 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•19 years ago
|
||
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•19 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•19 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•19 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•19 years ago
|
||
Addressed previous review comments and improve the visible name of the field.
Attachment #188582 -
Flags: review?(bienvenu)
Updated•19 years ago
|
Attachment #188582 -
Flags: review?(bienvenu) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #188582 -
Flags: superreview?(neil.parkwaycc.co.uk)
Comment 14•19 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•19 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•19 years ago
|
||
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•19 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•19 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•19 years ago
|
Attachment #188781 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 19•19 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•19 years ago
|
Status: NEW → ASSIGNED
Whiteboard: Import fixed, export to do.
Assignee | ||
Updated•19 years ago
|
Component: Address Book → MailNews: Address Book
Product: Mozilla Application Suite → Core
Assignee | ||
Comment 20•19 years ago
|
||
*** Bug 266691 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 21•19 years ago
|
||
*** Bug 116975 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 22•19 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•18 years ago
|
||
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•18 years ago
|
Attachment #210363 -
Flags: review?
Assignee | ||
Updated•18 years ago
|
Attachment #210363 -
Flags: review?(neil)
Comment 24•18 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•18 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•18 years ago
|
||
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•18 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•18 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•18 years ago
|
||
Updated patch for checkin.
Attachment #211183 -
Attachment is obsolete: true
Attachment #212136 -
Flags: superreview+
Attachment #212136 -
Flags: review+
Assignee | ||
Comment 30•18 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•18 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•18 years ago
|
||
All checked in -> FIXED.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 33•18 years ago
|
||
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•18 years ago
|
Attachment #213240 -
Flags: approval-branch-1.8.1?(mscott) → approval-branch-1.8.1+
Assignee | ||
Updated•18 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•18 years ago
|
Keywords: fixed-seamonkey1.1a,
fixed1.8.1
Whiteboard: Import fixed, export to do.
Assignee | ||
Comment 34•18 years ago
|
||
*** Bug 343305 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 35•18 years ago
|
||
*** Bug 357385 has been marked as a duplicate of this bug. ***
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•