I've cleaned whitespaces, removed completely qstrdump (left one copy_string where it was needed), merged 2 later patches into one (3. was just changing message from 2.), changed author of the commits to match Signed-off-by.
2015-03-15 15:31 GMT+01:00 Dirk Hohndel <[email protected]>: > On Sun, Mar 15, 2015 at 11:01:06AM +0100, Jan Darowski wrote: >> 1) To be honest I'm not sure where it comes from, seems like >> QtDesigner chenged it... > > OK, that makes sense. > >> 2) I think this qstrdup can and should be removed... I used qstrdup >> instead of strdup (they work the same) because somewhere else qstrdup >> was used. > > In general we should use copy_string(). > A quick grep shows that we have one single other use of qstrdup and > usually use our own copy_string (and occasionally use strdup). The > difference is that copy_string handles NULL pointers while strdup doesn't. > In this case you shouldn't have the risk of getting a NULL pointer back > but using copy_string might still be the most consistent choice. > > From looking at the code in this particular instance the copy isn't needed > because the fileNames QStringList is in scope until the end of this > function. Since the called function only references the string but doesn't > keep the pointer that is passed in around there is no issue. > >> It's good that you pointed it out because most of these >> qstrdup uses cause mem leaks right now. > > Well, usually they are there because either the original string would go > out of scope or because we want to make sure that we can free the pointer > later. Subsurface does leak some memory here and there, but I don't think > "most of" the string copying is incorrect. There might be a few calls to > free missing here and there (even though we do our occasional attempts to > find and add those). > >> In the evening I will send corrected version of these patches, sorry >> for the bugs. > > Thanks. Just in case this wasn't obvious - your patches looked good. What > I did was mostly nitpick since this was your first submission to > Subsurface... > > One more small comment - your From line (which turns into the "Author") > and your Signed-of-by line don't agree with each other. > Eltharan <[email protected]> / Jan Darowski <[email protected]> > > It would look better if you could be consistent. We have many contributors > who use several email addresses and I usually don't mind, but again, as > you set things up, this might be an easy thing to fix. > > Thanks > > /D >
From a9bc13de8cea1110aba2208b8ce07347fc92bd6e Mon Sep 17 00:00:00 2001 From: Jan Darowski <[email protected]> Date: Sat, 14 Mar 2015 15:35:47 +0100 Subject: [PATCH 1/2] Refactored image timestamp checking. Seperated getting image timestamp from picture_load_exif_data() and ShiftImageTimesDialog::syncCameraClicked() into picture_get_timestamp() and seperated checking timestamp from dive_create_picture() to dive_check_picture_time(). Signed-off-by: Jan Darowski <[email protected]> --- dive.c | 44 +++++++++++++++++++++++++++++++++++--------- dive.h | 5 ++++- qt-ui/simplewidgets.cpp | 14 ++++---------- qthelper.cpp | 19 +++++++++++++++++-- 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/dive.c b/dive.c index 2fb0875..afa38e4 100644 --- a/dive.c +++ b/dive.c @@ -2884,22 +2884,47 @@ static bool new_picture_for_dive(struct dive *d, char *filename) // only add pictures that have timestamps between 30 minutes before the dive and // 30 minutes after the dive ends #define D30MIN (30 * 60) +bool dive_check_picture_time(struct dive *d, char *filename, int shift_time) +{ + timestamp_t timestamp = 0; + picture_get_timestamp(filename, ×tamp); + offset_t offset; + if (timestamp) { + offset.seconds = timestamp - d->when + shift_time; + if (offset.seconds > -D30MIN && offset.seconds < (int)d->duration.seconds + D30MIN) { + // this picture belongs to this dive + return true; + } + } + return false; +} + +bool picture_check_valid(char *filename, int shift_time) +{ + bool result = false; + int i; + struct dive *d; + + for_each_dive (i, d) + if (d->selected) + result = result || dive_check_picture_time(d, filename, shift_time); + return result; +} + void dive_create_picture(struct dive *d, char *filename, int shift_time) { timestamp_t timestamp; if (!new_picture_for_dive(d, filename)) return; + if (!dive_check_picture_time(d, filename, shift_time)) + return; + struct picture *p = alloc_picture(); p->filename = filename; - picture_load_exif_data(p, ×tamp); - if (timestamp) { - p->offset.seconds = timestamp - d->when + shift_time; - if (p->offset.seconds < -D30MIN || p->offset.seconds > (int)d->duration.seconds + D30MIN) { - // this picture doesn't belong to this dive - free(p); - return; - } - } + picture_get_timestamp(filename, ×tamp); + p->offset.seconds = timestamp - d->when + shift_time; + picture_load_exif_data(p); + dive_add_picture(d, p); dive_set_geodata_from_picture(d, p); } @@ -2940,6 +2965,7 @@ static void picture_free(struct picture *p) free(p->hash); free(p); } + void dive_remove_picture(char *filename) { struct picture **ep = ¤t_dive->picture_list; diff --git a/dive.h b/dive.h index 528b45a..97d8523 100644 --- a/dive.h +++ b/dive.h @@ -381,11 +381,14 @@ struct picture { for (struct picture *picture = (_divestruct).picture_list; picture; picture = picture->next) extern struct picture *alloc_picture(); +extern bool dive_check_picture_time(struct dive *d, char *filename, int shift_time); extern void dive_create_picture(struct dive *d, char *filename, int shift_time); extern void dive_add_picture(struct dive *d, struct picture *newpic); extern void dive_remove_picture(char *filename); extern unsigned int dive_get_picture_count(struct dive *d); -extern void picture_load_exif_data(struct picture *p, timestamp_t *timestamp); +extern bool picture_check_valid(char *filename, int shift_time); +extern void picture_load_exif_data(struct picture *p); +extern void picture_get_timestamp(char *filename, timestamp_t *t); extern void dive_set_geodata_from_picture(struct dive *d, struct picture *pic); extern int explicit_first_cylinder(struct dive *dive, struct divecomputer *dc); diff --git a/qt-ui/simplewidgets.cpp b/qt-ui/simplewidgets.cpp index ecb8a8b..8a0f2b0 100644 --- a/qt-ui/simplewidgets.cpp +++ b/qt-ui/simplewidgets.cpp @@ -272,9 +272,7 @@ void ShiftImageTimesDialog::buttonClicked(QAbstractButton *button) void ShiftImageTimesDialog::syncCameraClicked() { - struct memblock mem; - EXIFInfo exiv; - int retval; + timestamp_t timestamp; QPixmap picture; QDateTime dcDateTime = QDateTime(); QStringList fileNames = QFileDialog::getOpenFileNames(this, @@ -290,13 +288,9 @@ void ShiftImageTimesDialog::syncCameraClicked() scene->addPixmap(picture.scaled(ui.DCImage->size())); ui.DCImage->setScene(scene); - if (readfile(fileNames.at(0).toUtf8().data(), &mem) <= 0) - return; - retval = exiv.parseFrom((const unsigned char *)mem.buffer, (unsigned)mem.size); - free(mem.buffer); - if (retval != PARSE_EXIF_SUCCESS) - return; - dcImageEpoch = exiv.epoch(); + + picture_get_timestamp(fileNames.at(0).toUtf8().data(), ×tamp); + dcImageEpoch = timestamp; dcDateTime.setTime_t(dcImageEpoch); ui.dcTime->setDateTime(dcDateTime); connect(ui.dcTime, SIGNAL(dateTimeChanged(const QDateTime &)), this, SLOT(dcDateTimeChanged(const QDateTime &))); diff --git a/qthelper.cpp b/qthelper.cpp index 189d47e..b63eb66 100644 --- a/qthelper.cpp +++ b/qthelper.cpp @@ -334,7 +334,7 @@ extern "C" xsltStylesheetPtr get_stylesheet(const char *name) return xslt; } -extern "C" void picture_load_exif_data(struct picture *p, timestamp_t *timestamp) +extern "C" void picture_load_exif_data(struct picture *p) { EXIFInfo exif; memblock mem; @@ -343,7 +343,6 @@ extern "C" void picture_load_exif_data(struct picture *p, timestamp_t *timestamp goto picture_load_exit; if (exif.parseFrom((const unsigned char *)mem.buffer, (unsigned)mem.size) != PARSE_EXIF_SUCCESS) goto picture_load_exit; - *timestamp = exif.epoch(); p->longitude.udeg= lrint(1000000.0 * exif.GeoLocation.Longitude); p->latitude.udeg = lrint(1000000.0 * exif.GeoLocation.Latitude); @@ -352,6 +351,22 @@ picture_load_exit: return; } +extern "C" void picture_get_timestamp(char *filename, timestamp_t *t) +{ + EXIFInfo exif; + memblock mem; + int retval; + + if (readfile(filename, &mem) <= 0) + return; + retval = exif.parseFrom((const unsigned char *)mem.buffer, (unsigned)mem.size); + free(mem.buffer); + if (retval != PARSE_EXIF_SUCCESS) + return; + *t = exif.epoch(); + return; +} + extern "C" const char *system_default_directory(void) { static char filename[PATH_MAX]; -- 2.2.2
From 52ac6d420ab440e6753b7c28b2b6a95319b31c28 Mon Sep 17 00:00:00 2001 From: Jan Darowski <[email protected]> Date: Sat, 14 Mar 2015 17:44:24 +0100 Subject: [PATCH 2/2] Added warning when not all images can be added. Added label in the ShiftImageTimesDialog which appears when not all of the selected images have timestamp in the checked range. Made cancel button in this widget actually work. Signed-off-by: Jan Darowski <[email protected]> --- qt-ui/divelistview.cpp | 7 ++++--- qt-ui/shiftimagetimes.ui | 25 +++++++++++++++++++++++++ qt-ui/simplewidgets.cpp | 38 +++++++++++++++++++++++++++++++++++++- qt-ui/simplewidgets.h | 5 ++++- 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/qt-ui/divelistview.cpp b/qt-ui/divelistview.cpp index e4ccb72..b905e9c 100644 --- a/qt-ui/divelistview.cpp +++ b/qt-ui/divelistview.cpp @@ -850,9 +850,10 @@ void DiveListView::loadImages() return; updateLastUsedImageDir(QFileInfo(fileNames[0]).dir().path()); - ShiftImageTimesDialog shiftDialog(this); + ShiftImageTimesDialog shiftDialog(this, fileNames); shiftDialog.setOffset(lastImageTimeOffset()); - shiftDialog.exec(); + if (!shiftDialog.exec()) + return; updateLastImageTimeOffset(shiftDialog.amount()); Q_FOREACH (const QString &fileName, fileNames) { @@ -861,7 +862,7 @@ void DiveListView::loadImages() for_each_dive (j, dive) { if (!dive->selected) continue; - dive_create_picture(dive, qstrdup(fileName.toUtf8().data()), shiftDialog.amount()); + dive_create_picture(dive, copy_string(fileName.toUtf8().data()), shiftDialog.amount()); } } diff --git a/qt-ui/shiftimagetimes.ui b/qt-ui/shiftimagetimes.ui index cce51e8..56a2228 100644 --- a/qt-ui/shiftimagetimes.ui +++ b/qt-ui/shiftimagetimes.ui @@ -117,6 +117,31 @@ </widget> </item> <item> + <widget class="QLabel" name="warningLabel"> + <property name="enabled"> + <bool>true</bool> + </property> + <property name="styleSheet"> + <string notr="true">color: red;</string> + </property> + <property name="text"> + <string>Warning! +Not all images have timestamps in the range between +30 minutes before the start and 30 minutes after the end of any selected dive.</string> + </property> + </widget> + </item> + <item> + <widget class="QLabel" name="invalidLabel"> + <property name="styleSheet"> + <string notr="true">color: red; </string> + </property> + <property name="text"> + <string/> + </property> + </widget> + </item> + <item> <widget class="QDialogButtonBox" name="buttonBox"> <property name="orientation"> <enum>Qt::Horizontal</enum> diff --git a/qt-ui/simplewidgets.cpp b/qt-ui/simplewidgets.cpp index 8a0f2b0..58959c1 100644 --- a/qt-ui/simplewidgets.cpp +++ b/qt-ui/simplewidgets.cpp @@ -303,11 +303,12 @@ void ShiftImageTimesDialog::dcDateTimeChanged(const QDateTime &newDateTime) setOffset(newDateTime.toTime_t() - dcImageEpoch); } -ShiftImageTimesDialog::ShiftImageTimesDialog(QWidget *parent) : QDialog(parent), m_amount(0) +ShiftImageTimesDialog::ShiftImageTimesDialog(QWidget *parent, QStringList fileNames) : QDialog(parent), m_amount(0), fileNames(fileNames) { ui.setupUi(this); connect(ui.buttonBox, SIGNAL(clicked(QAbstractButton *)), this, SLOT(buttonClicked(QAbstractButton *))); connect(ui.syncCamera, SIGNAL(clicked()), this, SLOT(syncCameraClicked())); + connect(ui.timeEdit, SIGNAL(timeChanged(const QTime &)), this, SLOT(timeEditChanged(const QTime &))); dcImageEpoch = (time_t)0; } @@ -327,6 +328,41 @@ void ShiftImageTimesDialog::setOffset(time_t offset) ui.timeEdit->setTime(QTime(offset / 3600, (offset % 3600) / 60, offset % 60)); } +void ShiftImageTimesDialog::updateInvalid() +{ + timestamp_t timestamp; + QDateTime time; + bool allValid = true; + ui.warningLabel->hide(); + ui.invalidLabel->hide(); + ui.invalidLabel->clear(); + + Q_FOREACH (const QString &fileName, fileNames) { + if (picture_check_valid(fileName.toUtf8().data(), m_amount)) + continue; + + // We've found invalid image + picture_get_timestamp(fileName.toUtf8().data(), ×tamp); + dcImageEpoch = timestamp; + time.setTime_t(timestamp + m_amount); + ui.invalidLabel->setText(ui.invalidLabel->text() + fileName + " " + time.toString() + "\n"); + allValid = false; + } + + if (!allValid){ + ui.warningLabel->show(); + ui.invalidLabel->show(); + } +} + +void ShiftImageTimesDialog::timeEditChanged(const QTime &time) +{ + m_amount = time.hour() * 3600 + time.minute() * 60; + if (ui.backwards->isChecked()) + m_amount *= -1; + updateInvalid(); +} + bool isGnome3Session() { #if defined(QT_OS_WIW) || defined(QT_OS_MAC) diff --git a/qt-ui/simplewidgets.h b/qt-ui/simplewidgets.h index 75b1df4..5f2402a 100644 --- a/qt-ui/simplewidgets.h +++ b/qt-ui/simplewidgets.h @@ -97,7 +97,7 @@ private: class ShiftImageTimesDialog : public QDialog { Q_OBJECT public: - explicit ShiftImageTimesDialog(QWidget *parent); + explicit ShiftImageTimesDialog(QWidget *parent, QStringList fileNames); time_t amount() const; void setOffset(time_t offset); private @@ -105,8 +105,11 @@ slots: void buttonClicked(QAbstractButton *button); void syncCameraClicked(); void dcDateTimeChanged(const QDateTime &); + void timeEditChanged(const QTime &time); + void updateInvalid(); private: + QStringList fileNames; Ui::ShiftImageTimesDialog ui; time_t m_amount; time_t dcImageEpoch; -- 2.2.2
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
