Images are 1st class citizens of TXP

Bloke · 2010-01-27 20:31:07

I (stupidly) wrote:

I’ll see how easy it is to cache the results in image_list

Duh. It’s using the new global $thisimage so I was utterly wrong with the number of DB calls.

In which case, there’s no penalty for having two tag calls to do the job of <txp:image_size />, it’s just more typing. Would anyone miss the break, type and as_html attributes from <txp:image_size /> so all it did was to output width="X" height="Y" — of either the main image or the thumb="1"? If you want to output the width and height you would have to use two <txp:image_info /> tags instead. Or should I just drop the tag altogether since it’s only of marginal benefit?

jsoo wrote:

in the absence of id, name, and category, grab the article image(s).

That makes a lot of sense. At the moment, without any id/category/name it uses every image (1=1). What I could do is add a step such that if nothing was chosen, it would first check if the tag was being used in an article context and if so try the article image field. Perhaps if that failed, it would then use all images. This would also lend itself nicely to being able to check if the image_list is being used inside a file or link context and therefore be able to automatically look in the (prospective) file_image or link_image fields if nothing has been chosen manually.

Question though: is this confusing behaviour? If you had a <txp:image_list> tag in an article form and you viewed article A (which had some values in its article image) you’d see a nice gallery. In Article B however, if there are no IDs in the article_image field (and still no category/id/name used) you’d see a gallery of all images in the database. Undesirable? I’d say yes.

A workaround would be to not default to ‘all images’. But then, at the other end of the scale, if you use a bare <txp:image_list> tag you’d surely expect something to be displayed if you’ve uploaded images, rather than nothing? In the same way that file_download_list will list all files if you don’t narrow it down. So I wonder if there should be either:

a) an ‘opt-in’ facility where you can say “yep, use the context if it’s there” and then it’s on your head if you have an empty article_image field

b) an ‘opt-out’ facility to say “if the tag matches no images, leave it with nothing and don’t default to all images”

Which is better? I think I favour method (a) but I’m willing to be swayed. Since you’re all on a roll and my brain is flagging courtesy of the 4am bed time last night after getting this patch out the door, any cool ideas on how this should be implemented? Perhaps some suggestions for a suitably-named attribute, or some very clever default values for the existing attributes?

maniqui wrote:

an alias for txp:image_* so it could be written as txp:img_* would be appreciated

It’s a lot of code for saving two measly characters. I’d say probably not going to happen, sorry.

Last edited by Bloke (2010-01-27 20:36:15)

jsoo · 2010-01-27 20:51:29

Bloke wrote:

Question though: is this confusing behaviour? If you had a <txp:image_list> tag in an article form and you viewed article A (which had some values in its article image) you’d see a nice gallery. In Article B however, if there are no IDs in the article_image field (and still no category/id/name used) you’d see a gallery of all images in the database. Undesirable? I’d say yes.

Easily avoided using if_article_image, so no need to add another attribute.

Bloke · 2010-01-27 20:53:43

jsoo wrote:

Easily avoided using if_article_image, so no need to add another attribute.

*sigh* told you my brain was mashed ;-) Thanks for the nudge.

maniqui · 2010-01-27 21:03:40

Bloke wrote:

Question though: is this confusing behaviour? If you had a <txp:image_list> tag in an article form and you viewed article A (which had some values in its article image) you’d see a nice gallery. In Article B however, if there are no IDs in the article_image field (and still no category/id/name used) you’d see a gallery of all images in the database. Undesirable? I’d say yes.

I would say yes too.
And I’m not sure about image_list returing all images if no attribute is specified. I wonder if there is a real case use for that. I would much prefer some kind of trigger value on category attribute for those situations, something like category="ALL" (the problem is to find this trigger word to avoid issues with any existing category already named “all”).

More: now that Bloke mentioned file_download_list, I would like to say that I don’t like the way that file_download_list works, returning all files when id attribute is there but it’s empty.
In code: <txp:file_download_list id="" /> returns all files.
So, if you have: <txp:file_download_list id='<txp:custom_field name="files" />' /> and your custom field is empty, it outputs a list of all files. Undesirable, as Bloke commented regarding image_list.

The workaround is:

<txp:if_custom_field name="files">
<txp:file_download_list id='<txp:custom_field name="files" />' />
</txp:if_custom_field>

Similarly,

<txp:if_article_image>
<txp:image_list id='<txp:article_image give_me_the_raw_thing="1" />' />
</txp:if_article_image>

could be an option.

But, imho, <txp:image_list id="" /> (and <txp:file_download_list id="" /> should return absolutely nothing.

To summarize: one thing is having the behaviour of <txp:image_list /> returning all images (may be desirable, although I don’t believe there is a real case use, same with files) but the othe thing is having <txp:image_list id="" /> returning all images, which I find totally undesirable.

A workaround would be to not default to ‘all images’. But then, at the other end of the scale, if you use a bare <txp:image_list> tag you’d surely expect something to be displayed if you’ve uploaded images, rather than nothing?

I think I prefer it to return nothing at all. Explicit is better than implicit in this case. And a trigger word on category attribute or new attribute to display all images may be the option to have all website images listed.
—
Hehehe, don’t worry about the txp:img_* request. My RSI won’t miraculously heal by avoiding typing just two characters.

jsoo · 2010-01-27 21:09:46

maniqui wrote:

I would much prefer some kind of trigger value on category attribute for those situations, something like category="ALL" (the problem is to find this trigger word to avoid issues with any existing category already named “all”).

<txp:image_list category='<txp:category_list type="image" break=","><txp:category /></txp:category_list>' />

jsoo · 2010-01-27 21:14:16

Although that doesn’t get images without an assigned category.

But I agree with Julián that defaulting to every image in the DB is strange behavior. If this needs to be an option I’d prefer something like <txp:image_list id="*" />.

Last edited by jsoo (2010-01-27 21:16:35)

Bloke · 2010-01-27 21:28:55

maniqui wrote:

I don’t like the way that file_download_list works, returning all files when id attribute is there but it’s empty.

Yes, it’s not ideal. And you’ll get the same behaviour with image_list :-(

It’s partly because of the way the internal lAtts() TXP function works, that 99% of plugins use. By the time that function has done its stuff and extracted/checked/processed all attributes there is no way to tell (programmatically) if the user specified a value that was empty or the default empty value has been used. They are identical.

I have bypassed it (in fact in smd_gallery) by putting some code before the lAtts() function that checks to see if $atts['id'] is set and raising a flag if it is. Then, lAtts() can do its stuff and I still know that there was an attempt to do something with the id tag, and it ‘failed’.

It’s easy enough to stop the ‘all images’ behaviour. I only left it in because it mirrored file_download_list and I was going for consistency between tags wherever possible. I can remove that easily and — as long as it’s made clear that the image_list tag returns absolutely nothing if your given options match nothing — we’re in the clear. The same can’t be said of file_download_list unfortunately because it’s a non-backwards-compatible change. Can people live with the differences or do you think that will cause us problems with countless support threads on why they’re different?

As jsoo says, another “problem” is how to get a list of all images that aren’t assigned to any category. Currently you can’t do that but, to be fair, you can’t do it with file_download_list or linklist either. Again, I’ve solved it in the past with a trigger word — which is fine for a plugin because you have the plugin prefix to fall back on — but it makes me nervous to do that kind of thing in the core!

Last edited by Bloke (2010-01-27 21:30:51)

jsoo · 2010-01-27 21:47:36

Bloke wrote:

It’s partly because of the way the internal lAtts() TXP function works, that 99% of plugins use. By the time that function has done its stuff and extracted/checked/processed all attributes there is no way to tell (programmatically) if the user specified a value that was empty or the default empty value has been used. They are identical.

For some plugins I have set one or more attributes’ default value to null, so that a user-specified attribute="" means something different. But it would be confusing to introduce that kind of behavior in core, when so many other tags don’t work that way.

Bloke · 2010-01-27 22:11:40

I agree, jsoo. Nice trick with the null attribute value, btw. I’ll have to remember that one for future plugins.

I’ve just been testing the latest changes so this is now the defined behaviour for <txp:image_list>:

If id, category or name are used, they (cumulatively) determine the images in the gallery pool
If any of the above are not used (or they all happen to equate to empty) AND we’re in an article context, the article_image field is checked, like this:
- If article_image is empty it is ignored
- If article_image contains a URL or some non-numeric reference to an image, it is ignored
- If article_image contains one or more image IDs they will be added to the pool
If all the above result in a valid pool, it is used and passed to the Form/container for processing
If all the above result in no images being returned, nothing is output

Note that although the article_image field can contain a list of image IDs here, it cannot be used like this with the good old <txp:article_image /> tag. So you make your choice at the outset: to use article_image to render one image or use image_list to have the option of rendering one or more. It may be possible to retrofit article_image with the ability to understand a list of IDs, but it’s not high priority.

As a side benefit, when and if the file_image and link_image tags come into existence, image_list could easily (and automatically) take advantage of them to allow you to show a gallery of images associated with a file or link!

Does that sound logical to you all, or should I try harder? :-)

I’ve also had a sudden pang of guilt over image_size. Although it does seem to cover some of the same ground as image_info and the break attribute isn’t quite the same as other tags, image_size does (currently) do something different to <txp:image_info type="thumb_w | thumb_h" />. Namely, if the sizes are not defined in the database’s thumb_w and thumb_h fields (for whatever reason) the tag will go and check the actual thumbnail file and try to read its dimensions. Conversely, image_info looks in the database for the size information and if it’s not set it returns nothing.

This behaviour is perhaps superfluous now, since the reading of the file’s dimensions was a hangover from the days when thumb_w and thumb_h didn’t exist in the DB. So I may still axe the attributes or the tag entirely. If anyone has any preference either way, speak now.

I have, however, altered the default break to an x so something like 1024x768 is the default output of the tag.

Any comments on the above considered. Otherwise I’ll commit this change and wait for the next batch of fallout :-)

jsoo · 2010-01-27 22:44:37

Bloke wrote:

Note that although the article_image field can contain a list of image IDs here, it cannot be used like this with the good old <txp:article_image /> tag. So you make your choice at the outset: to use article_image to render one image or use image_list to have the option of rendering one or more. It may be possible to retrofit article_image with the ability to understand a list of IDs, but it’s not high priority.

~~@intval()@ will return the first ID in the list, so article_image will return the first article image even as is.~~ Edit: Oops, see below.

Last edited by jsoo (2010-01-29 13:41:56)

Bloke · 2010-01-29 13:27:40

jsoo wrote:

intval() will return the first ID in the list, so article_image will return the first article image even as is.

It makes sense that it does, but it doesn’t on my system. If I put 2, 3, 4 in my article_image field I get nothing from the <txp:article_image /> tag :-s That doesn’t mean it isn’t true on other systems of course. Perhaps article_image should be modified so it only picks up the 1st image in all cases? To avoid any unexpected behaviour that abusing the field (or variations in PHP version) might incur.

Anyway, I’ve relented a little in other matters. r3306 works like this:

<txp:image_list> now returns nothing if id/category/name and article_image field match nothing. The article image is of course ignored if it is empty or the tag is used in a list context
The article image field is only checked if id/name/category are all empty. This is probably logical given that specifically requesting certain images at the tag level would override any at the article level. For more complex cases where you want to cumulatively work with both an attribute-specified set and the article_image data, you’re best off with a plugin anyway
You may use id="*" to signify you want all images (thanks jsoo)
You can, in fact, get at the uncategorized images already like this: category="," (show all uncategorized images) or category="cat1, cat2," (show cat1, cat2, and all uncategorized images)
The <txp:image_size> tag has been removed completely, because everything it did can be obtained far more simply with a couple of <txp:image_info /> tags. Thanks to ruud / Gocom / jsoo / maniqui / et al for making me see the light

Last edited by Bloke (2010-01-29 13:32:15)

wet · 2010-01-29 13:34:10

Bloke wrote:

It’s partly because of the way the internal lAtts() TXP function works, that 99% of plugins use. By the time that function has done its stuff and extracted/checked/processed all attributes there is no way to tell (programmatically) if the user specified a value that was empty or the default empty value has been used. They are identical.

I beg to differ, as there’s both isset($atts['value']) and isset($value) at your disposal.

Textpattern CMS

Textpattern CMS support forum

#25 2010-01-27 20:31:07

Re: Images are 1st class citizens of TXP

#26 2010-01-27 20:51:29

Re: Images are 1st class citizens of TXP

#27 2010-01-27 20:53:43

Re: Images are 1st class citizens of TXP

#28 2010-01-27 21:03:40

Re: Images are 1st class citizens of TXP

#29 2010-01-27 21:09:46

Re: Images are 1st class citizens of TXP

#30 2010-01-27 21:14:16

Re: Images are 1st class citizens of TXP

#31 2010-01-27 21:28:55

Re: Images are 1st class citizens of TXP

#32 2010-01-27 21:47:36

Re: Images are 1st class citizens of TXP

#33 2010-01-27 22:11:40

Re: Images are 1st class citizens of TXP

#34 2010-01-27 22:44:37

Re: Images are 1st class citizens of TXP

#35 2010-01-29 13:27:40

Re: Images are 1st class citizens of TXP

#36 2010-01-29 13:34:10

Re: Images are 1st class citizens of TXP

Board footer