Go to main content

Textpattern CMS support forum

You are not logged in. Register | Login | Help

#97 2010-07-20 01:17:46

gomedia
Plugin Author
Registered: 2008-06-01
Posts: 1,373

Re: Images are 1st class citizens of TXP

A quick tangent …

Bloke wrote:

… except the widespread limit attribute (also of type integer). Specifying a limit of 0 doesn’t mean “show me no results” it means “show me all results” (i.e. no limit).

Gocom wrote:

Meaning that limit=“0” translates into limit is false.

Er, I wanted there to be a no limit option for article tags but I don’t think it ever happened.

Offline

#98 2010-09-28 11:40:16

jsoo
Plugin Author
From: NC, USA
Registered: 2004-11-15
Posts: 1,793
Website

Re: Images are 1st class citizens of TXP

Bloke wrote:

to turn off both height and width you use width="0" height="0". If either are omitted you get the size from the database being output.

Reviving this now that 4.3.0 rc1 is out. I’m not entirely clear from the old discussion what the final decision on intent was to be, but I see that in rc1 width="0" height="0" does indeed suppress those attributes in the final HTML output. In image and article_image. But not in thumbnail. Any reason these shouldn’t all behave the same way?

[Code-wise, reason for the difference is that thumbnail uses an empty($width) check, whereas the other two use $width=="" checks.]


Code is topiary

Offline

#99 2010-09-28 11:47:35

jsoo
Plugin Author
From: NC, USA
Registered: 2004-11-15
Posts: 1,793
Website

Re: Images are 1st class citizens of TXP

Great to have pagination with image_list. And the new image context, hence cool category URLs such as:

http://example.foo/category/image/bar/

But there’s a wee issue with pagination with URLs such as the above. Actually, it’s a more general issue than that. The above URL is image context, hence an article tag returns nothing. But you’ll get a does not contain a txp:article tag notice if you don’t have an article tag for that context. You can suppress the notice by sticking in the article tag anyway. But then pagination doesn’t work.

I haven’t looked into it enough to glean what might be a reasonable solution.


Code is topiary

Offline

#100 2010-09-28 12:24:10

Bloke
Developer
From: Leeds, UK
Registered: 2006-01-29
Posts: 11,250
Website GitHub

Re: Images are 1st class citizens of TXP

jsoo wrote:

You can suppress the notice by sticking in the article tag anyway. But then pagination doesn’t work.

Hmmm, yes that could be a potential problem. But I just tried this on my default Page and removed all txp:article tags:

<txp:if_category type="image">
   <txp:image_list limit="3" pageby="limit">
      <txp:thumbnail />
   </txp:image_list>
</txp:if_category>

I didn’t get the warning — even in debugging mode — which is curious as I’d have expected it to do so. Maybe it’s just my installation though and I need to try it on a clean install. Does that work for you or do you still get the warning?

width="0" height="0" … Any reason these shouldn’t all behave the same way?

Erm, nope. I thought I’d made them all the same, d’oh. Good catch, thanks I’ll fix that.


The smd plugin menagerie — for when you need one more gribble of power from Textpattern. Bleeding-edge code available on GitHub.

Txp Builders – finely-crafted code, design and Txp

Offline

#101 2010-09-28 13:02:00

jsoo
Plugin Author
From: NC, USA
Registered: 2004-11-15
Posts: 1,793
Website

Re: Images are 1st class citizens of TXP

Bloke wrote:

I didn’t get the warning — even in debugging mode — which is curious as I’d have expected it to do so. Maybe it’s just my installation though and I need to try it on a clean install. Does that work for you or do you still get the warning?

Reeeeally stupid question, but did you check it in a suitable image-contexty URL? Because I just copied the exact code above into a template and triggered the error.


Code is topiary

Offline

#102 2010-09-28 13:17:44

Bloke
Developer
From: Leeds, UK
Registered: 2006-01-29
Posts: 11,250
Website GitHub

Re: Images are 1st class citizens of TXP

jsoo wrote:

did you check it in a suitable image-contexty URL?

Yep. site.com/category/image/my-cat. Must be my install then, being a mashup of about 50 different environments. Curious why it’s suppressing the error: that’s good, but I don’t know why!

I’ll try again in a clean installation. Perhaps it’s time to lose that silly warning, but that in itself unfortunatley doesn’t solve the problem of what happens if you have an article tag on the same page as a context list. My guess is the problem stems from the fact that pagination is on by default for articles but off by default for everything else, and articles take precedence over other content types. Pants.


The smd plugin menagerie — for when you need one more gribble of power from Textpattern. Bleeding-edge code available on GitHub.

Txp Builders – finely-crafted code, design and Txp

Offline

#103 2010-09-28 13:34:20

Bloke
Developer
From: Leeds, UK
Registered: 2006-01-29
Posts: 11,250
Website GitHub

Re: Images are 1st class citizens of TXP

Unsatisfactory solution #1: if you put <txp:article pgonly="1" /> after the <txp:image_list /> tag, the error is suppressed and the navigation works.

I lied when I said articles take precendence, the first pagination context encountered on the page takes precedence: every time the paging array is updated it is first checked to see if pagination has already been set up: it’s bypassed if so.

Longer term I kinda half figured that a single pagination array would bite us. You can see how I got round it in smd_gallery by assigning a unique code to each gallery instance so you can page independently of other galleries on the same page, and indeed of TXP. We could probably do with either making discrete pagination arrays for each context type or one multi-dimensioned array — with a dimension for each type. Or something like that. Ideas on a postcard to the usual address…


The smd plugin menagerie — for when you need one more gribble of power from Textpattern. Bleeding-edge code available on GitHub.

Txp Builders – finely-crafted code, design and Txp

Offline

#104 2010-09-28 13:41:12

jsoo
Plugin Author
From: NC, USA
Registered: 2004-11-15
Posts: 1,793
Website

Re: Images are 1st class citizens of TXP

Bloke wrote:

Unsatisfactory solution #1: if you put <txp:article pgonly="1" /> after the <txp:image_list /> tag, the error is suppressed and the navigation works.

I tried that earlier but had no success; will try again with a more stripped-down template to see what’s going on.

Edit: yep, works. I don’t know what I did wrong the first time.

I lied when I said articles take precendence, the first pagination context encountered on the page takes precedence: every time the paging array is updated it is first checked to see if pagination has already been set up: it’s bypassed if so.

Thought so; I had to dig into this a while back w/r/t my pagination plugin.

Longer term I kinda half figured that a single pagination array would bite us.

Maybe, but this is a pretty small problem. Off the top of my head I think just adding a context check to /publish.php:537 (where my error message arose) would do it:

if (!$has_article_tag and $pretext['context']=='article' and  // ...

Last edited by jsoo (2010-09-28 13:43:45)


Code is topiary

Offline

#105 2010-09-28 15:38:26

Bloke
Developer
From: Leeds, UK
Registered: 2006-01-29
Posts: 11,250
Website GitHub

Re: Images are 1st class citizens of TXP

jsoo wrote:

adding a context check to /publish.php:537 (where my error message arose) would do it:

Makes a lot of sense. Committed, along with the thumbnail width/height fix. Many thanks, sir.


The smd plugin menagerie — for when you need one more gribble of power from Textpattern. Bleeding-edge code available on GitHub.

Txp Builders – finely-crafted code, design and Txp

Offline

#106 2010-09-28 19:49:14

the_ghost
Plugin Author
From: Minsk, The Republic of Belarus
Registered: 2007-07-26
Posts: 907
Website

Re: Images are 1st class citizens of TXP

If images are first class citizens, as articles, may its worth to add notification “no txp:image/file tag found” in dependency of current context (if it can be found)?

Or most of developers think, that this message is more annoying then helpful?


Providing help in hacking ATM! Come to courses and don’t forget to bring us notebook and hammer! What for notebook? What a kind of hacker you are without notebok?

Offline

#107 2010-09-29 10:39:59

jsoo
Plugin Author
From: NC, USA
Registered: 2004-11-15
Posts: 1,793
Website

Re: Images are 1st class citizens of TXP

Preserving arbitrary sort order for multiple article images has been requested again, as has been discussed earlier in this thread. It’s time to take a stand on:

Should multiple article images be a supported core feature? If so;
  1. What should article_image do?
  2. And should we use the FIELD() hack, or let users do this themselves?

If the answer is no, then image_list has no business trying to parse csv in the article-image field. Either it’s a supported feature or it isn’t.

If the answer is yes, first priority is to adapt article_image. Mustn’t break existing tags. The fix is trivial: change the if_numeric check to intval. The only conceivable downside is that a URL beginning with a digit would go the wrong way here, but I can’t see how that would ever be an issue.

What about FIELD()? It’s currently in use elsewhere in core. If you want to implement it in image_list, set sort to empty in lAtts(), then later set it to its default value if it is still empty after the article-image context check. But I don’t think Sam will let you get away with it :)

If no FIELD(), then leave it to a plugin. Yes, you could easily add a boolean attribute to article_image to get it to show the raw field contents, but doesn’t smell right.

Yet another route would be for image_list to loop through the results set and order it then. It would do this if sort is empty and the list is based on the article-image field. Quite a bit of code to avoid FIELD().


Code is topiary

Offline

#108 2010-09-29 14:25:53

Bloke
Developer
From: Leeds, UK
Registered: 2006-01-29
Posts: 11,250
Website GitHub

Re: Images are 1st class citizens of TXP

jsoo wrote:

Should multiple article images be a supported core feature?

I, of course, say yes. Because I’m like that.

Doing so is not, unfortunately, as trivial as it first seems. Unless I missed something; which is entirely possible1. Here’s my stab. It adds break functionality to <txp:article_image /> so if you put 17, 10, 42 in your article image field you get three images out, one after the other. wraptag/break to taste. Thus it’s a bit like image_list but without the ability to output arbitrary fields; more of a short-circuited image_list. Not sure if it’s of any use because of the functionality overlap.

    function article_image($atts)
    {
        global $thisarticle;

        assert_article();

        extract(lAtts(array(
            'align'     => '', // deprecated in 4.2.0
            'class'     => '',
            'escape'    => 'html',
            'html_id'   => '',
            'style'     => '',
            'width'     => '',
            'height'    => '',
            'thumbnail' => 0,
            'wraptag'   => '',
            'break'     => '',
        ), $atts));

        if ($align)
            trigger_error(gTxt('deprecated_attribute', array('{name}' => 'align')), E_USER_NOTICE);

        if ($thisarticle['article_image'])
        {
            $image = $thisarticle['article_image'];
        }

        else
        {
            return;
        }

        // These atts are extracted later so need preserving
        $a_thumb = $thumbnail;
        $a_width = $width;
        $a_height = $height;

        if (intval($image))
        {
            $rs = safe_rows('*', 'txp_image', 'id IN ('.join(',', do_list(doSlash($image))).')');

            if ($rs)
            {
                foreach ($rs as $row)
                {
                    $width = ($a_width=='') ? (($a_thumb) ? $row['thumb_w'] : $row['w']) : $a_width;
                    $height = ($a_height=='') ? (($a_thumb) ? $row['thumb_h'] : $row['h']) : $a_height;

                    if ($a_thumb)
                    {
                        if ($row['thumbnail'])
                        {
                            extract($row);

                            if ($escape == 'html')
                            {
                                $alt = htmlspecialchars($alt);
                                $caption = htmlspecialchars($caption);
                            }

                            $out[] = '<img src="'.imagesrcurl($id, $ext, true).'" alt="'.$alt.'"'.
                                ($caption ? ' title="'.$caption.'"' : '').
                                ( ($html_id and !$wraptag) ? ' id="'.$html_id.'"' : '' ).
                                ( ($class and !$wraptag) ? ' class="'.$class.'"' : '' ).
                                ($style ? ' style="'.$style.'"' : '').
                                ($align ? ' align="'.$align.'"' : '').
                                ($width ? ' width="'.$width.'"' : '').
                                ($height ? ' height="'.$height.'"' : '').
                                ' />';
                        }

                        else
                        {
                            return '';
                        }
                    }

                    else
                    {
                        extract($row);

                        if ($escape == 'html')
                        {
                            $alt = htmlspecialchars($alt);
                            $caption = htmlspecialchars($caption);
                        }

                        $out[] = '<img src="'.imagesrcurl($id, $ext).'" alt="'.$alt.'"'.
                            ($caption ? ' title="'.$caption.'"' : '').
                            ( ($html_id and !$wraptag) ? ' id="'.$html_id.'"' : '' ).
                            ( ($class and !$wraptag) ? ' class="'.$class.'"' : '' ).
                            ($style ? ' style="'.$style.'"' : '').
                            ($align ? ' align="'.$align.'"' : '').
                            ($width ? ' width="'.$width.'"' : '').
                            ($height ? ' height="'.$height.'"' : '').
                            ' />';
                    }
                }
            }

            else
            {
                trigger_error(gTxt('unknown_image'));
                return;
            }
        }

        else
        {
            $out[] = '<img src="'.$image.'" alt=""'.
                ( ($html_id and !$wraptag) ? ' id="'.$html_id.'"' : '' ).
                ( ($class and !$wraptag) ? ' class="'.$class.'"' : '' ).
                ($style ? ' style="'.$style.'"' : '').
                ($align ? ' align="'.$align.'"' : '').
                ($width ? ' width="'.$width.'"' : '').
                ($height ? ' height="'.$height.'"' : '').
                ' />';
        }

        return ($wraptag) ? doWrap($out, $wraptag, $break, $class, '', '', '', $html_id) : join('', $out);
    }

This does not preserve ID order. I don’t like using the FIELD() thing simply because it’s MySQL specific (yes I know it’s in use in the core). But equally I don’t like the kludgery one has to go through in code to re-order things after they’ve been munged by the MySQL order filter.

So I say leave Field() order to plugins or manually.

Incidentally, the above implementation has an unexpected annoyance but I don’t really know how to get round it (and is probably the reason I abandoned it first time round):

  • if you put a single ID in your article_image and that image doesn’t exist you get one warning
  • if you put a list of IDs in your article_image and some of the images don’t exist, the missing ones are silently skipped
  • if you put a list of IDs in your article_image and none of the images exist you get one warning

I tried getting round this in various ways but it’s nasty. As it stands it’s perhaps less than desirable too.

Dunno. Thoughts anyone?

1 EDIT: or you simply use article_image to always grab the first image and ignore the rest, which is what I think you meant with the intval comment ,yes? Question: is that true ‘support’ for multiple article images or a kludge to allow image_list and article_image to co-exist?

EDIT2: Simply permitting article_image to silently ignore multiple images by using intval() is fine with me, btw. But I just don’t know if that’s confusing to be able to list them in your article image field and then not see them on the rendered page unless you use a specific tag.

Last edited by Bloke (2010-09-29 14:38:51)


The smd plugin menagerie — for when you need one more gribble of power from Textpattern. Bleeding-edge code available on GitHub.

Txp Builders – finely-crafted code, design and Txp

Offline

Board footer

Powered by FluxBB