[CSV import window] add option to choose dropdown menu field names from CSV#490
Conversation
|
Maybe a screenshot? I will test this in the next few days |
|
Sure! Without the new feature: With new feature: |
| m_ui->labelSizeRowsCols->setText(m_parserModel->getFileInfo()); | ||
| m_ui->spinBoxSkip->setValue(0); | ||
| m_ui->spinBoxSkip->setMaximum(qMax(0, m_parserModel->rowCount() - 1)); | ||
| m_ui->spinBoxSkip->setValue(minSkip); |
There was a problem hiding this comment.
Just a minor note:
here you can use setRange to both set max and min value.
You should set the min value to minSkip so when the checkBoxFieldNames is selected, people can't skip the first row in the spinbox
|
|
||
| for (i = 1; i < m_parserModel->getCsvCols(); ++i) { | ||
| QString s = QString(tr("Column ")) + QString::number(i); | ||
| fieldName = m_parserModel->getCsvTable().at(0).at(i); |
There was a problem hiding this comment.
shouldn't this be inside the if below?
| fieldName = "<" + tr("Empty fieldname ") + QString::number(++emptyId) + ">"; | ||
| s = fieldName; | ||
| } | ||
| else |
There was a problem hiding this comment.
please use proper coding style (curly brackets are always enforced)
if (..) {
...
} else {
...
}
|
|
||
| int i; | ||
| int i, emptyId = 0; | ||
| QString s; |
There was a problem hiding this comment.
Get rid of this variable and just use fieldName. fieldName should also probably be renamed columnName.
This should also go back into the for loop since you only reference it in that scope.
seatedscribe
force-pushed
the
feature/csv-field-names
branch
from
1ee7329 to
28225c2
Compare
|
New commit addressing requested changes 👍 |
|
|
||
| int i; | ||
| int i, emptyId = 0; | ||
| QString columnName; |
There was a problem hiding this comment.
This variable and i need to be put inside the loop to conform to scoping rules.
for (int i = 1; ....)
There was a problem hiding this comment.
Well, that i variable is used some lines below so it needs to be at that scope level.
There was a problem hiding this comment.
Can't you use two separate var, one into the for and one on line 158?
| QString s = QString(tr("Column ")) + QString::number(i); | ||
| list << s; | ||
| if (m_ui->checkBoxFieldNames->isChecked()) { | ||
| columnName = m_parserModel->getCsvTable().at(0).at(i); |
There was a problem hiding this comment.
QString columnName = ....
Also, be careful here, you start i at 1, should we evaluate i == 0??
There was a problem hiding this comment.
Why do you think it is a mistake?
There was a problem hiding this comment.
See comment at src/gui/csvImport/CsvParserModel.h line 53 (for some reason, link below does not point to such line)
seatedscribe
force-pushed
the
feature/csv-field-names
branch
from
28225c2 to
eb7f4d2
Compare
|
@TheZ3ro applied your change request, as soon as this is merged I will start from here to implement the "more columns" CSV import feature. Ciao! |
- Added YubiKey 2FA integration for unlocking databases [#127] - Added TOTP support [#519] - Added CSV import tool [#146, #490] - Added KeePassXC CLI tool [#254] - Added diceware password generator [#373] - Added support for entry references [#370, #378] - Added support for Twofish encryption [#167] - Enabled DEP and ASLR for in-memory protection [#371] - Enabled single instance mode [#510] - Enabled portable mode [#645] - Enabled database lock on screensaver and session lock [#545] - Redesigned welcome screen with common features and recent databases [#292] - Multiple updates to search behavior [#168, #213, #374, #471, #603, #654] - Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480] - Fixed auto-type errors on Linux [#550] - Prompt user prior to executing a cmd:// URL [#235] - Entry attributes can be protected (hidden) [#220] - Added extended ascii to password generator [#538] - Added new database icon to toolbar [#289] - Added context menu entry to empty recycle bin in databases [#520] - Added "apply" button to entry and group edit windows [#624] - Added macOS tray icon and enabled minimize on close [#583] - Fixed issues with unclean shutdowns [#170, #580] - Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515] - Compare window title to entry URLs [#556] - Implemented inline error messages [#162] - Ignore group expansion and other minor changes when making database "dirty" [#464] - Updated license and copyright information on souce files [#632] - Added contributors list to about dialog [#629]
3 participants
A checkbox let user choose field names from CSV first row, instead of an aseptic "Column n"
This should also close #458
Description
Motivation and context
A more cohesive fit with CSV column meaning was needed in order to make the right mapping between CSV fields and KXC DB fields
How has this been tested?
Manual tests
Types of changes
Checklist:
-DWITH_ASAN=ON. [REQUIRED]