ImageView improvements

[tremby]

• Build tag programmatically, for less error-prone code and more maintainability
• Grab image alt and copyright from Json, expose it to the user via getAlt and getCopyright
• Add alt attribute to tag if alt is available

Potential issue: as far as I know there's no way to distinguish between alt being set and empty, and alt not being set at all. Sometimes it is desirable to emit alt="" but at other times not. At present this doesn't allow for an alt="" attribute since we're assuming it was just not set at all.

[tremby]

Oh, I've ruined a whole bunch of tests. Ignore for now...

[tremby]

Right, this is because the test Json data doesn't have alt and copyright set on the images. But all the API calls I make have them present (even if empty) on all images I've seen. Are there any cases where these keys won't be present? I'll have to update the code or the tests either way.

[dohzya]

Hi @tremby

I'm really sorry I took so long to respond to you :-(

alt and copyright should be returned by every API request, so you can add them in the JSON data.

[tremby]

Okay, I've rewritten some commits so tests pass at each point, added alt and copyright fields to the test JSON and am expecting them to always exist, I'm always outputting alt="whatever", even if it's an empty string, and I've added a suite of tests which use DOMDocument to make sure the HTML output is parsable as HTML, has exactly one (image) tag and this contains the right attributes. I've also rebased it on the current upstream master branch.

This is ready to look at.

[tremby]

I've added a new commit which allows an array of attributes to be passed to asHtml. This allows setting custom attributes or overriding the default ones. A common use case would be setting a class attribute.

[dohzya]

I see one problem with your last commit: every asHtml methods accept one argument: the LinkResolver. Updating this particular implementation of asHtml to makes it use an other argument could lead to some very frustrating problems. I would be more consistant to accept a LinkResolver as 1st parameter (even if it is not used) and to accept the attributes as 2nd parameter.

However, I feel a bit weird that this attributes parameter is available only on ImageView#asHtml (and not in Text#AsHtml for instance) and I'm sure this implementation could be extended (allowing to override some attributes for instance)… So is it ok if—for this PR—I merge only the commit a72a399b6f096380369d9fc6b0b810b9b5fb6250 and let you put 36c37a36874f5e89a6a5f52238b73f19ad16255f in a new branch?

[tremby]

It feels strange to pass a parameter which is never needed, but I agree that consistency is important. I've amended that last commit, which is now c56dc32. Is that better?

Honestly I can't think of any time I'd want to set attributes on anything Text#AsHtml would emit, since it seems to me that could output various things whereas with an image it seems pretty obvious I'm going to get an <img> tag. But this patch certainly doesn't preclude the possibility of adding similar parameters to other asHtml functions.

[dohzya]

Thanks a lot!

[tremby]

Thank you.