Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make exif_entry_initialize() set up entries in the GPS ifd #54

Merged
merged 6 commits into from Apr 21, 2021

Conversation

hlewin
Copy link
Contributor

@hlewin hlewin commented Sep 23, 2020

We need format and component infos in the ExifEntry to decide how to parse string-representations. Sadly exif_entry_initialize() does not set up this info for the GPS ifd.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 229

  • 24 of 29 (82.76%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 81.951%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libexif/exif-gps-ifd.c 4 5 80.0%
libexif/exif-entry.c 20 24 83.33%
Totals Coverage Status
Change from base Build 226: 0.006%
Covered Lines: 3142
Relevant Lines: 3834

💛 - Coveralls

@hlewin hlewin closed this Nov 3, 2020
@AlexandrePTJ AlexandrePTJ mentioned this pull request Apr 21, 2021
@msmeissn msmeissn reopened this Apr 21, 2021
@msmeissn msmeissn merged commit e12c352 into libexif:master Apr 21, 2021
@msmeissn
Copy link
Contributor

While I merged this now, was rthere a specific reason you closed this before?

@hlewin
Copy link
Contributor Author

hlewin commented Apr 21, 2021

@msmeissn Unforunately, yes - the initial commits contained some bugs, so needed further work.
Then I wanted to add tests for complete coverage, but haven't found time to do so, yet.
I was unsure if that feature is actually wanted so this wasn't high on my priority list.

PS: Just to be clear: The bugs should be fixed now, but there are no tests for them.

@msmeissn
Copy link
Contributor

hmm, i reverted it again from master branch then if there are bugs ... sorry :/

It seems there is interest in it though.

@hlewin
Copy link
Contributor Author

hlewin commented Apr 21, 2021

If there is interest, I can supply the missing test.
The bug itself, as said, should be fixed - we are using the patch in production for some time now.
Should I open another MR then when I am done?

@msmeissn
Copy link
Contributor

I think there is interest, so I would welcome it :)

msmeissn added a commit that referenced this pull request Apr 21, 2021
…)"

This reverts commit e12c352.

was not finished, see pullrequest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants