Differential D14523 Diff 38876 src/kdefrontend/datasources/ImportFileWidget.cpp

Changeset View

Standalone View

View Options

src/kdefrontend/datasources/ImportFileWidget.cpp

Show First 20 Lines • Show All 599 Lines • ▼ Show 20 Line(s)584void ImportFileWidget::selectFile() {
600 //TODO: decide whether the selection of several files should be possible 600​ //TODO: decide whether the selection of several files should be possible
601// QStringList filelist = QFileDialog::getOpenFileNames(this,i18n("Select one or more files to open")); 601​// QStringList filelist = QFileDialog::getOpenFileNames(this,i18n("Select one or more files to open"));
602// if (! filelist.isEmpty() ) 602​// if (! filelist.isEmpty() )
603// ui.leFileName->setText(filelist.join(";")); 603​// ui.leFileName->setText(filelist.join(";"));
604} 604​}
605 605​
606/************** SLOTS **************************************************************/ 606​/************** SLOTS **************************************************************/
607 607​
608​QString absolutePath(const QString& fileName)
609​{
610​#ifndef HAVE_WINDOWS
611​ // make absolute path // FIXME
612​ if ( !fileName.isEmpty() && fileName.at(0) != QDir::separator())
fkristofUnsubmitted
Done

Remove space

( !fileName.isEmpty()
fkristof: Remove space ``` ( !fileName.isEmpty() ```
fkristofUnsubmitted
Not Done

I'd say it would be safer to have 2 separate ifs, even if we know that the order is left to right, if somebody changes/adds here something and the fileName.at(0) will be before the isEmpty() then there will be a crash.

if (!fileName.isEmpty()) {
       if (fileName.at(0) != QDir::separator()) {
fkristof: I'd say it would be safer to have 2 separate ifs, even if we know that the order is left to…
asemkeUnsubmitted
Not Done

This part Christoph copied simply from another place... This logic we have in many places in the code where we striclty rely on the fixed ordering of the evaluation which has also to be guaranteed by the compiler. Many people are used to this and I think this is ok...

asemke: This part Christoph copied simply from another place... This logic we have in many places in…
croickAuthorUnsubmitted
Not Done

I agree with Alexander. What's actually bothering me here, is the assumption that a relative path is supposed to be relative to the home folder. Maybe there is some reason for this, but it seems weird at least.

croick: I agree with Alexander. What's actually bothering me here, is the assumption that a relative…
asemkeUnsubmitted
Not Done

Let's decouple this problem from your patch that is fixing another problem. Let's finalize your patch and submitt it. Maybe @sgerlach can have another look at this piece of code where already another FIXME is placed.

asemke: Let's decouple this problem from your patch that is fixing another problem. Let's finalize your…
croickAuthorUnsubmitted
Not Done

Actually it was me putting the FIXME there.

croick: Actually it was me putting the FIXME there.
613​ return QDir::homePath() + QDir::separator() + fileName;
614​#endif
615​ return fileName;
616​}
617​
608/*! 618​/*!
609 called on file name changes. 619​ called on file name changes.
610 Determines the file format (ASCII, binary etc.), if the file exists, 620​ Determines the file format (ASCII, binary etc.), if the file exists,
611 and activates the corresponding options. 621​ and activates the corresponding options.
612*/ 622​*/
613void ImportFileWidget::fileNameChanged(const QString& name) { 623​void ImportFileWidget::fileNameChanged(const QString& name) {
614 QString fileName = name; 624​ const QString fileName = absolutePath(name);
asemkeUnsubmitted
Done

const QString& fileName

asemke: const QString& fileName
croickAuthorUnsubmitted
Not Done

You meant const QString fileName, right?

croick: You meant `const QString fileName`, right?
asemkeUnsubmitted
Not Done

I meant const reference. Though the difference with respect to the performance between copying an object and creating a reference to it might be very small for implicitly shared Qt classes (QString is implicitely shared), it's still matter of convention and of code styly. With const QString& you _explicitly_ say I do not want to copy and I'm not going to change the reference - this is what we try to consistenly use throughout the code.

asemke: I meant const reference. Though the difference with respect to the performance between copying…
croickAuthorUnsubmitted
Not Done

I agree that arguments should be passed by reference, not by value, but I cannot have a const reference to a temporary object. The object it is referring to (fileName in absolutePath() or just the temporary return value) would be deleted once the function returns.

croick: I agree that arguments should be passed by reference, not by value, but I cannot have a const…
asemkeUnsubmitted
Not Done

Ok. This is a valid point. I forgot that we are having here a new function.

asemke: Ok. This is a valid point. I forgot that we are having here a new function.
615#ifndef HAVE_WINDOWS ​
616 // make relative path ​
617 if ( !fileName.isEmpty() && fileName.at(0) != QDir::separator()) ​
618 fileName = QDir::homePath() + QDir::separator() + fileName; ​
619#endif ​
620 625​
621 bool fileExists = QFile::exists(fileName); 626​ bool fileExists = QFile::exists(fileName);
622 if (fileExists) 627​ if (fileExists)
623 ui.leFileName->setStyleSheet(""); 628​ ui.leFileName->setStyleSheet("");
624 else 629​ else
625 ui.leFileName->setStyleSheet("QLineEdit{background:red;}"); 630​ ui.leFileName->setStyleSheet("QLineEdit{background:red;}");
626 631​
627 ui.gbOptions->setEnabled(fileExists); 632​ ui.gbOptions->setEnabled(fileExists);
Show All 15 Lines638 if (!fileExists) {
643 648​
644 emit fileNameChanged(); 649​ emit fileNameChanged();
645 return; 650​ return;
646 } 651​ }
647 652​
648 if (currentSourceType() == LiveDataSource::FileOrPipe) { 653​ if (currentSourceType() == LiveDataSource::FileOrPipe) {
649 const AbstractFileFilter::FileType fileType = AbstractFileFilter::fileType(fileName); 654​ const AbstractFileFilter::FileType fileType = AbstractFileFilter::fileType(fileName);
650 ui.cbFileType->setCurrentIndex(fileType); 655​ ui.cbFileType->setCurrentIndex(fileType);
651 if (auto filter = currentFileFilter()) { 656​ updateContent(fileName, fileType);
652 switch(fileType) { ​
653 case AbstractFileFilter::HDF5: ​
654 m_hdf5OptionsWidget->updateContent((HDF5Filter*)filter, fileName); ​
655 break; ​
656 case AbstractFileFilter::NETCDF: ​
657 m_netcdfOptionsWidget->updateContent((NetCDFFilter*)filter, fileName); ​
658 break; ​
659 case AbstractFileFilter::FITS: ​
660#ifdef HAVE_FITS ​
661 m_fitsOptionsWidget->updateContent((FITSFilter*)filter, fileName); ​
662#endif ​
663 break; ​
664 case AbstractFileFilter::Json: ​
665 m_jsonOptionsWidget->loadDocument(fileName); ​
666 break; ​
667 case AbstractFileFilter::ROOT: ​
668 m_rootOptionsWidget->updateContent((ROOTFilter*)filter, fileName); ​
669 break; ​
670 case AbstractFileFilter::Ascii: ​
671 case AbstractFileFilter::Binary: ​
672 case AbstractFileFilter::Image: ​
673 case AbstractFileFilter::NgspiceRawAscii: ​
674 case AbstractFileFilter::NgspiceRawBinary: ​
675 break; ​
676 } ​
677 } ​
678 } 657​ }
679 658​
680 refreshPreview(); 659​ refreshPreview();
681 emit fileNameChanged(); 660​ emit fileNameChanged();
682} 661​}
683 662​
684/*! 663​/*!
685 saves the current filter settings 664​ saves the current filter settings
▲ Show 20 Lines • Show All 99 Lines • ▼ Show 20 Line(s)687void ImportFileWidget::fileTypeChanged(int fileType) {
785 ui.cbFilter->clear(); 764​ ui.cbFilter->clear();
786 ui.cbFilter->addItem( i18n("Automatic") ); 765​ ui.cbFilter->addItem( i18n("Automatic") );
787 ui.cbFilter->addItem( i18n("Custom") ); 766​ ui.cbFilter->addItem( i18n("Custom") );
788 767​
789 //TODO: populate the combobox with the available pre-defined filter settings for the selected type 768​ //TODO: populate the combobox with the available pre-defined filter settings for the selected type
790 ui.cbFilter->setCurrentIndex(lastUsedFilterIndex); 769​ ui.cbFilter->setCurrentIndex(lastUsedFilterIndex);
791 filterChanged(lastUsedFilterIndex); 770​ filterChanged(lastUsedFilterIndex);
792 771​
772​ if (currentSourceType() == LiveDataSource::FileOrPipe) {
773​ const QString fileName = absolutePath(ui.leFileName->text());
asemkeUnsubmitted
Done

const QString& fileName

asemke: const QString& fileName
croickAuthorUnsubmitted
Not Done

You meant const QString fileName, right?

croick: You meant `const QString fileName`, right?
774​
775​ if (QFile::exists(fileName))
776​ updateContent(fileName, fileType);
777​ }
778​
793 refreshPreview(); 779​ refreshPreview();
794} 780​}
795 781​
796 782​
797const QStringList ImportFileWidget::selectedHDF5Names() const { 783​const QStringList ImportFileWidget::selectedHDF5Names() const {
798 return m_hdf5OptionsWidget->selectedHDF5Names(); 784​ return m_hdf5OptionsWidget->selectedHDF5Names();
799} 785​}
800 786​
▲ Show 20 Lines • Show All 48 Lines • ▼ Show 20 Line(s)
849} 835​}
850 836​
851void ImportFileWidget::refreshPreview() { 837​void ImportFileWidget::refreshPreview() {
852 if (m_suppressRefresh) 838​ if (m_suppressRefresh)
853 return; 839​ return;
854 840​
855 WAIT_CURSOR; 841​ WAIT_CURSOR;
856 842​
857 QString fileName = ui.leFileName->text(); 843​ QString fileName = absolutePath(ui.leFileName->text());
asemkeUnsubmitted
Not Done

const QString& name

asemke: const QString& name
croickAuthorUnsubmitted
Not Done

Cannot make it const, it's modified later on.

croick: Cannot make it const, it's modified later on.
asemkeUnsubmitted
Not Done

ok.

asemke: ok.
858#ifndef HAVE_WINDOWS ​
859 if (!fileName.isEmpty() && fileName.at(0) != QDir::separator()) ​
860 fileName = QDir::homePath() + QDir::separator() + fileName; ​
861#endif ​
862 DEBUG("refreshPreview(): file name = " << fileName.toStdString()); 844​ DEBUG("refreshPreview(): file name = " << fileName.toStdString());
863 845​
864 QVector<QStringList> importedStrings; 846​ QVector<QStringList> importedStrings;
865 AbstractFileFilter::FileType fileType = (AbstractFileFilter::FileType)ui.cbFileType->currentIndex(); 847​ AbstractFileFilter::FileType fileType = (AbstractFileFilter::FileType)ui.cbFileType->currentIndex();
866 848​
867 // generic table widget 849​ // generic table widget
868 if (fileType == AbstractFileFilter::Ascii || fileType == AbstractFileFilter::Binary 850​ if (fileType == AbstractFileFilter::Ascii || fileType == AbstractFileFilter::Binary
869 || fileType == AbstractFileFilter::Json || fileType == AbstractFileFilter::NgspiceRawAscii 851​ || fileType == AbstractFileFilter::Json || fileType == AbstractFileFilter::NgspiceRawAscii
▲ Show 20 Lines • Show All 246 Lines • ▼ Show 20 Line(s)1097 } else {
1116 m_fileEmpty = true; 1098​ m_fileEmpty = true;
1117 } 1099​ }
1118 1100​
1119 emit previewRefreshed(); 1101​ emit previewRefreshed();
1120 1102​
1121 RESET_CURSOR; 1103​ RESET_CURSOR;
1122} 1104​}
1123 1105​
1106​void ImportFileWidget::updateContent(const QString& fileName, int fileType)
fkristofUnsubmitted
Done

AbstractFileFilter::FileType fileType

fkristof: AbstractFileFilter::FileType fileType
croickAuthorUnsubmitted
Not Done

fileTypeChanged() gets the fileType as int. I would have to explicitly cast this to AbstractFileFilter::FileType for the call of updateContent(). I thought I should stick to the current code, but I can change it, if you think that it is important.

croick: `fileTypeChanged()` gets the fileType as `int`. I would have to explicitly cast this to…
asemkeUnsubmitted
Done

Let's cast this to the enum value like we do this in ImportFileWidget::fileNameChanged() already and in all other widgets where we cast from integers coming from the comboboxes to the internal enum values, also in order to benefit from the compiler checks for the missing switch-cases. The current code in fileTypeChanged() is not good in this respect, yes. So, if it's not a big issue for you, let's clean up here in your patch.

asemke: Let's cast this to the enum value like we do this in ImportFileWidget::fileNameChanged()…
1107​{
1108​ if (auto filter = currentFileFilter()) {
1109​ switch(fileType) {
1110​ case AbstractFileFilter::HDF5:
1111​ m_hdf5OptionsWidget->updateContent((HDF5Filter*)filter, fileName);
1112​ break;
1113​ case AbstractFileFilter::NETCDF:
1114​ m_netcdfOptionsWidget->updateContent((NetCDFFilter*)filter, fileName);
1115​ break;
1116​ case AbstractFileFilter::FITS:
1117​#ifdef HAVE_FITS
1118​ m_fitsOptionsWidget->updateContent((FITSFilter*)filter, fileName);
1119​#endif
1120​ break;
1121​ case AbstractFileFilter::ROOT:
1122​ m_rootOptionsWidget->updateContent((ROOTFilter*)filter, fileName);
1123​ break;
1124​ case AbstractFileFilter::Ascii:
1125​ case AbstractFileFilter::Binary:
1126​ case AbstractFileFilter::Image:
1127​ case AbstractFileFilter::Json:
1128​ case AbstractFileFilter::NgspiceRawAscii:
1129​ case AbstractFileFilter::NgspiceRawBinary:
1130​ break;
1131​ }
1132​ }
1133​}
1134​
1135​
1124void ImportFileWidget::updateTypeChanged(int idx) { 1136​void ImportFileWidget::updateTypeChanged(int idx) {
1125 const LiveDataSource::UpdateType UpdateType = static_cast<LiveDataSource::UpdateType>(idx); 1137​ const LiveDataSource::UpdateType UpdateType = static_cast<LiveDataSource::UpdateType>(idx);
1126 1138​
1127 switch (UpdateType) { 1139​ switch (UpdateType) {
1128 case LiveDataSource::UpdateType::TimeInterval: 1140​ case LiveDataSource::UpdateType::TimeInterval:
1129 ui.lUpdateInterval->show(); 1141​ ui.lUpdateInterval->show();
1130 ui.sbUpdateInterval->show(); 1142​ ui.sbUpdateInterval->show();
1131 break; 1143​ break;
▲ Show 20 Lines • Show All 185 Lines • Show Last 20 Lines