improve correctness of GNSS datetime handling #6502
Conversation
This prevents initial positioning window not being greyed out even when there has never been a GNSS fix. This also avoids pretending the system time is the GNSS time in EXIF metadata. That could potentially break someone's batch image processing, so maybe add to release notes.
|
Additional comments in issue #6571 |
|
I'm an outsider, but were I reviewing for merge i'd want to see the multiple commits rebased into tthe series of commits you would have done had you done it perfectly the first time. Not a criticism of getting to working incrementlaly, , but On projects I maintain I prefer to merge clean commits, vs squashing things that don't necessarily go together. I don't know how qfield feels. |
|
|
||
| metadata["Exif.GPSInfo.GPSDateStamp"] = positionInformation.utcDateTime().date(); | ||
| metadata["Exif.GPSInfo.GPSTimeStamp"] = positionInformation.utcDateTime().time(); | ||
| // slight change in behaviour, but more correct |
There was a problem hiding this comment.
I prefer to see changes in commit messages, and comments in the code describe how it is. I find "slight change in behavior" unhelpful, and it will not age well. Change from when? Why? What actual change?
| { | ||
| metadata["Exif.GPSInfo.GPSDateStamp"] = positionInformation.utcDateTime().date(); | ||
| metadata["Exif.GPSInfo.GPSTimeStamp"] = positionInformation.utcDateTime().time(); | ||
| } |
There was a problem hiding this comment.
I see what you're doing, but elsewhere you are using system time instead of gnss time if gnss time is not valid. Why is this different? I'm not saying you don't have a reason but that reason is what should be in the comment.
Are you finding that there is a valid position but not valid time in gnss data?
| mLastGnssPositionInformation.hacc(), | ||
| mLastGnssPositionInformation.vacc(), | ||
| mLastGnssPositionInformation.utcDateTime(), | ||
| mLastGnssPositionInformation.utcDateTime().isValid() ? mLastGnssPositionInformation.utcDateTime() : QDateTime::currentDateTimeUtc(), |
There was a problem hiding this comment.
This code is repeated a bunch of times. That makes me wonder:
- should it be a procedure?
- why check if datetime is not valid, but use hacc/vacc unconditionally? Is there really a situation with vllid accuracy but not valid time?
It seems clear something tricky is going on, but I don't understand it from the comments you didn't add, and I don't understand it from the commit message. Maybe GNSS data should not be used at all if the associated datetime is not valid, and that's the real bug.
2 participants
This prevents initial positioning window not being greyed out even when there has never been a GNSS fix.
This also avoids pretending the system time is the GNSS time in EXIF metadata. That could potentially break someone's batch image processing, so maybe add to release notes.