Last modified: 2010-05-15 15:38:44 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T2689, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 689 - reorganise and tidy up parsing of extended image syntax
reorganise and tidy up parsing of extended image syntax
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
File management (Other open bugs)
1.5.x
All All
: Normal normal with 2 votes (vote)
: ---
Assigned To: Rowan Collins [IMSoP]
:
Depends on:
Blocks: 178 368 539 648 3608 5568
  Show dependency treegraph
 
Reported: 2004-10-11 16:58 UTC by Rowan Collins [IMSoP]
Modified: 2010-05-15 15:38 UTC (History)
2 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
basically finished but not polished patch (23.82 KB, patch)
2005-04-29 21:59 UTC, Rowan Collins [IMSoP]
Details

Description Rowan Collins [IMSoP] 2004-10-11 16:58:18 UTC
This bug is a meta/tracking bug for all bugs concerning the parsing of the
"extended image syntax" ([[Image:Foo|param|param|param|caption]]) which I intend
to rewrite soon. Rewriting won't necessarily fix all of these, but will go some
way towards it, so I'm labelling this as blocking them, not vice-versa.

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()
* rewrite the recently-added manual thumbnail code to use a "thumb=$1" MagicWord
rather than splitting the text (depends on committing a fixed version of
MagicWord.php; i.e. bug 688).
Comment 1 Wil Mahan 2004-10-13 06:06:19 UTC
(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.
Comment 2 Rowan Collins [IMSoP] 2004-10-13 12:04:58 UTC
(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. 
Comment 3 Rowan Collins [IMSoP] 2005-02-18 15:51:09 UTC
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.
Comment 4 Rowan Collins [IMSoP] 2005-04-29 21:59:13 UTC
Created attachment 480 [details]
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!
Comment 5 lɛʁi לערי ריינהארט 2005-10-05 08:46:34 UTC
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]]
Comment 6 omniplex 2006-04-14 08:49:12 UTC
688 marked as resolved, so why should it block 689?
Comment 7 Brion Vibber 2008-03-14 23:13:52 UTC
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.
Comment 8 Brion Vibber 2008-12-30 02:32:22 UTC
All tracked bugs are currently resolved, so considering this issue closed. Resolving as fixed.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links