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

Support multiple values with Array #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dragn
Copy link

@dragn dragn commented Jun 28, 2015

Add support to set and get tags with multiple values.
getImageTags will return a string for single value or an array of strings for multiple values.
setImageTags will add multiple records with the same key when array is provided.
Solves #23
I'm just starting to use that in my own project, so any issues may pop up later.
Using localized text in tags would certainly impose some difficulties... (Usage of Exiv2::AsciiValue, for example...).
I don't really like that part with a lot of copy-paste stuff for ExifData/IptcData/XmpData. But, given that they don't derive from the same class, I have no idea what to do with it (except using macros, which would not do much better).

@@ -5,6 +5,7 @@
#include <unistd.h>
#include <string>
#include <map>
#include <list>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why you went with a list rather than say a vector?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most cases this list will contain single element. And all manipulations, such as conversion to v8::Array, happen to iterate through all values. Given that we don't need random access to elements and want the quick addition, "list" should be a good choice here.

@drewish
Copy link
Collaborator

drewish commented Jun 30, 2015

Looking through this, I really like the idea, but I'm wondering if by returning both arrays and strings we'll be breaking backwards compatibility. If people are assuming they'll get a string back and suddenly get an array that's going to cause problems. In that case I'd almost rather just always get an array back rather than having to check the type and then loop or not.

Thoughts?

/cc @dberesford

@dragn
Copy link
Author

dragn commented Jun 30, 2015

Most of the values are single, so returning array for each value is not very useful. Moreover, if we keep strings for single-value tags, the majority of existing code will still work.

@dberesford
Copy link
Owner

Looks good to me, nice job! @drewish I'm happy to merge this, how about you?

@rayshan
Copy link

rayshan commented Jul 29, 2015

+1

Was just about to submit a bug report. Currently returned Iptc.Application2.Keywords is just the first one, e.g. lightroom, but Xmp.dc.subject and Xmp.lr.hierarchicalSubject are comma-delimited strings, e.g. lightroom, photography, ray shan.

This patch should help.

@whitelynx
Copy link

@dragn Any chance this could be rebased so it can be merged?

@drewish
Copy link
Collaborator

drewish commented Jan 24, 2016

If we do merge this we need to bump the major version since it's not backwards compatible. Also should have an example in the readme since it's a little odd to have to check if it's a string or array. I still think that's going to be a confusing interface.

@dragn
Copy link
Author

dragn commented Jan 25, 2016

@drewish I think if we'd change it to return (and expect) tags as a single string with comma-separated values, we'd have the same functionality and backward compatibility, except for the fact that you can't use comma in tags...

@whitelynx
Copy link

How about an option to choose how multiple values are handled? Something like multipleValuesAsArray: true to get an array back, with the default being a comma-separated list. That way, if you don't mind doing the type checking (and/or have a need to store commas in tags) you can get arrays, but by default it won't break backward compatibility.

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

5 participants