wmahan_04 wrote:

(In reply to comment #0)

Additional tasks of the rewrite (for which there is currently no seperate bug):

• move the part of makeImageLinkObj() which *parses* the image parameters from

Skin.php to Parser.php, since this seems to make more sense, structurally;
instead, consider merging makeImageLinkObj() and makeThumbLinkObj()

Sounds OK.

Another minor bug that I haven't filed a report for: for images like
[[Image:foo|right]], the title and alt attributes are set to "right",
but they ought to be blank.

rowan.collins wrote:

(In reply to comment #1)

Another minor bug that I haven't filed a report for: for images like
[[Image:foo|right]], the title and alt attributes are set to "right",
but they ought to be blank.

Heh. That's funny, I was considering exactly the opposite behaviour: treat the
last argument as the "caption", and don't match it against any magic words. So
[[Image:foo|right]] would have title&alt="right", and align to the left. I guess
it depends which is more likely to be the intention; certainly treating it as
both parsed option *and* plain text is a bit silly.

Hmmm... "[[Image:my_left_thumb.jpeg|thumb]]" should that be a thumbnail, or have
text set to "thumb", or (as currently) both? I'm inclined to agree that most
likely it should be a thumbnail, and have a default text.

2 minor problems with treating the last "argument" as an option:

1. it makes for a slightly ugly test: after trying out all the magic words, we'd

have to have an "elseif(is_last_argument) {set_as_caption/alt/title/whatever}".
As opposed to just popping the last argument off the end of the array before we
even start testing.

2. we'd have to decide what the default caption for something like

[[Image:foo|thumb|right]] would be; at the moment, we assume the caption is
"right" (although we also align the image to the right) so there *is* no default
caption.

rowan.collins wrote:

I'm going to reword this bug to make it clear what it is I'm working on, and
avoid people dumping dependencies on it - my own fault for using a better
summary to start off with. Also note that I'll be working this in HEAD for
hopeful inclusion in 1.5, as 1.4 is likely too frozen by now.

Like I say, my basic aim is to rearrange the image syntax code (which is now in
Linker.php, but still split rather arbitrarily between <making thumbnails> and
<everything else>) so that the bits to do with parsing are in Parser.php. As a
side effect, I hope some of the more awkward bits of that code will become
easier to tidy up (e.g. how to handle links inside the label-type attributes, etc).

Meanwhile, it looks like the behaviour's been changed to treat anything that's
not an option, regardless of position, as the label/alt text, and anything that
*is* recognised as an option is *never* the caption. This seems pretty sane to
me, so I guess I'll leave it that way.

rowan.collins wrote:

basically finished but not polished patch

I fugured it was about time I showed that there really is something here, so
here's an initial version of my patch (agains HEAD, obviously).

Main changes:

• merged makeImageLinkObj() and makeThumbLinkObj() to avoid horrible

duplication of code; rewrote them in 3 steps: get the appropriate URLs and
dimensions, generate the central <img> etc, and then wrap it in appropriate
formatting

• split the parsing part into Parser.php (OK, so Tim did this already, but he

hadn't when I started)

• parameters are passed from one to the other in an associative array, to save

dealing with horrible numbers of function parameters (if this turns out to be a
Bad Idea, I'll change it)

• manual thumbnails have their own magic word rather than doing crazy text

splitting (this patch includes my bug 688 patch to make this possible)

Minor changes:

• re-added the display of a caption underneath unframed missing images (it

disappeared because of the new makeBrokenImageLinkObj()); maybe we should make
it look even more like the old one, which was more obviously not just text)

• images now always have width and height attributes; this is a side-effect of

the way I've ordered the code, but I see no harm in it, other than the need to
rewrite a lot of parser tests

• a few other side-effects of rewriting the functions, although my main aim was

to make future changes easier

To-do:

• testing - I've run through parserTests and only "broke" things because of

trivial markup changes, but there's more needs trying

• fix parserTests to reflect trivial changes
• profiling etc; work out if I'm doing something really inefficient
• possibly move some of the code that handles things like "if it's a thumb,

find a default width" back to Linker

• more testing - I think there are still situations where the code won't fail

gracefully, involving non-existent images, and particularly non-existent manual
thumbnails

• clean up - there may still be the odd comment, debug statement, or

now-redundant code lying about

Anyone that fancies applying this patch and seeing what happens, and/or telling
me anything that's really stupid about it, is more than welcome to do so!

Attached:

gangleri wrote:

in response to comment #0

This bug is a meta/tracking bug for all bugs concerning the parsing of the

"extended image syntax" ([[Image:Foo|param|param|param|caption]])

Dear Rowan,

if this still is a *tracking bug* it should probably block
bug 2007: Tracking bug (tracking)

Regards Reinhardt [[user:gangleri]]

omniplex wrote:

688 marked as resolved, so why should it block 689?

This patch is reallllly old and isn't going to apply, since a lot of changes have happened.

I'm removing the patch and need-review keywords, and will consider this just a tracking bug. If specific issues above remain, please make sure they're split off and marked as blocking.

All tracked bugs are currently resolved, so considering this issue closed. Resolving as fixed.