Item14070: Improve "Create a link to the attached file" formatting options and defaults.

There's a few things I'd like to change in how ATTACHEDFILELINKFORMAT preference is interpreted, the formatting tokens/variables it has, and the default formatting. Namely:

1. $comment defaults to $filename: I think the documentation [1] is right saying it's "the file comment from the upload dialog" (and not mentioning it defaulting to $filename). The code [2], however, defaults it to the attachment file name. It should default to '' (the empty string) instead.
2. The default format [3] is "\n   * [[%ATTACHURL%/$fileurl][$filename]]: $comment", which renders as:
   no comment given --> * someattachedfile.pdf: someattachedfile.pdf
   some comment given --> * someattachedfile.pdf: some comment
   The second case is okay, but the first case is pointless, redundant and generally ugly. I think it qualifies as an issue since most people wouldn't give a comment when attaching a file (but would tick the add link option). That is what I observe in a 10+ years old, 100+ user, heavily used Foswiki installation (well, it was the other wiki in the beginning.. ).
   I'd prefer that this renders as:
   no comment given --> * someattachedfile.pdf:
   some comment given --> * someattachedfile.pdf: some comment
   Unfortunately there is no way to get rid of the ":" in the no-comment case.
3. To support this behaviour I would like to add a $commentorfilename token/variable that expands to the attachment comment if available and falls back to the attachment name and change the default for ATTACHEDFILELINKFORMAT accordingly.
4. ATTACHEDIMAGEFORMAT also uses $comment as a label for the inlined image. Here's it's fine, and changing the default format to $commentorfilename will retain the behaviour.
5. The only problem I can see with this are users relying on the $comment token defaulting to $filename in their custom format preferences.

I have cleaned up the code for this that I've been using for a while and added the documentation changes ready for a pull request.

Have a look here: https://github.com/phkehl/Foswiki-distro/commit/7a4b84869369512f9624bbf5c82e4d0048e4f53d

What do you think? Good idea? Really stupid?

References:
[1] FileAttachment
[2] Foswiki::Attach::getAttachmentLink
[3] DefaultPreferences

-- PhilippeKehl - 13 May 2016

Philippe, I'll add this to the list to discuss in the next release meeting. Indeed I've noticed the annoying somefile: somefile inserts, but it never bothered me enough to go figure it out.

Looking through the code and your examples:

• The test for $att->{comment} || ''; means you could never have a comment of zero - as that would evaluate to false. Probably better to test for defined. (The original code had the same issue).
• I wonder if you could use the IF macro in the link format to test for an empty comment and that way fix the extraneous trailing : as well. In that case, it might be better to eliminate the default for comment altogether and resolve it in the IF.

   * [[%ATTACHURL%/$fileurl][$filename]]%IF{"'$comment'=''" else=": $comment"}%

Yes indeed this works, And the following restores the prior behavior:

[[%ATTACHURL%/$fileurl][$filename]]%IF{"'$comment'=''" then=": $filename" else=": $comment"}%

So, I'd say, very good idea, but this implementation is a bit simpler, and doesn't require either defaults in the code, or a new token.

• patchItem14070: Patch for alternate solution.

-- GeorgeClark - 25 Sep 2016

Except .. this leaves the %IF in the generated link. And we can't expand the IF easily without also expanding the %ATTACHURL ... Need to think about this a bit more.

-- GeorgeClark - 25 Sep 2016

George, thanks for looking into it. I've had this patch in my installation for years now. The extra ":" is still a bit annoying, indeed. I'd usually edit the page after uploading and move the link somehere else (and remove the extra ": $filename").

I also don't like the IF solution too much as it makes the inserted text much more complex (it renders fine, but the source code becomes more complex/unreadable, as you say, "it leaves the IF in the generated link"). Could we expand/evaluate the IF and insert the resulting text? Perhaps only for IFs that have no further macros in them? E.g. expand [[%ATTACHURL%/$fileurl][$filename]]%IF{"'$comment'=''" else=": $comment"}% to [[%ATTACHURL%/foo.pdf][foo.pdf]] or [[%ATTACHURL%/foo.pdf][foo.pdf]]: foo comment.

I don't know what you mean by "we can't expand the IF easily without also expanding the %ATTACHURL". Do you mean that we cannot limit the expansion of a string to certain macros only?

Or perhaps we could have two formats:

   * Set ATTACHEDFILELINKFORMAT = \n   * [[%ATTACHURL%/$fileurl][$filename]]: $comment
   * Set ATTACHEDFILELINKFORMATNOCOMMENT = \n   * [[%ATTACHURL%/$fileurl][$filename]]

(and also ATTACHEDIMAGEFORMAT and ATTACHEDIMAGEFORMATNOCOMMENT)

Btw, can we use modern Perl stuff? Such as my $fileComment = $att->{comment} // '';" instead of my $fileComment = $att->{comment} || '';".

Slightly off-topic: in one of my plugins I have more complex format tokens: E.g. you could say $token[, ](: ) which would expand to , tokentext: for $token = "tokentext" and expand to the empty string if $token = "". I.e. you can give prefix and suffix to a token that are suppressed if the token is empty.

-- PhilippeKehl - 25 Sep 2016

I've updated the attached patch. It resolves the issue with my patch.

• Uses $percntATTACHURL$percnt in the ATTACHEDFILELINKFORMAT
• Runs expandCommonVariables, so the IF expands, but ATTACHURL is ignored.
• Then replaces the "standard escapes" changing $percntA.. to %A

Use of $percnt to "delay expansion" is a common syntax, so this should be relatively understandable to experienced Foswiki users.

That way, then inserted link is completely clean - the %IF never makes it into the topic. And the %ATTACHURL% remains as a macro, preserving portability. Also, this lets other macros be used, such as "%WIKINAME% attached $filename: $comment"

The downside is that if a site has overridden the ATTACHEDFILE* settings, then unless they apply the $percnt, the URL gets expanded inline during the link insertion. Not really an awful side effect but could impact portability.

• patchItem14070: Updated patch - expands macros during insert.

re: modern Perl syntax ... not quite yet. We are looking at a possible jump to perl 5.14 or 5.18 in the "Object Oriented Foswiki" - Item13897 branch. Still rather experimental, but will let us jump to a newer perl and drop some backwards compatibility. For best performance Perl 5.22 should be the minimum perl for Foswiki now, but we are still keeping compatibility with 5.8.8.

-- GeorgeClark - 26 Sep 2016

Ideally the attachment link format ought to move into a template, so that these file and image links can be skinned as well. But that's for another task I think.

-- GeorgeClark - 26 Sep 2016

I like your patch. I've replaced my initial solution with that and updated the documentation: https://github.com/foswiki/distro/compare/master...phkehl:Item14070

I've done some tests (old formats, new formats, some custom formats with %WIKIUSER%). All ok.

Upgrading for users that have the ATTACHEDFILELINKFORMAT/ATTACHEDIMAGEFORMAT customised should work with no changes (at the cost of expanding %ATTACHURL% for future uploads as you mention). The required changes to the customised formats are minimal (replace %ATTACHURLPATH% with $percntATTACHURLPATH$percnt in the formats).

I've created a pull-request: https://github.com/foswiki/distro/pull/15

-- PhilippeKehl - 07 Oct 2016

We are not ignoring you. I've been focused on getting 2.1.3 beta out. your pull request will be picked up in master - for inclusion in the Foswiki 2.2 release.

-- GeorgeClark - 24 Jan 2017

These changes break ATTACHEDIMAGEFORMAT as it is already being used such as in

   * Set ATTACHEDIMAGEFORMAT = %IMAGE{"$name" size="200"}%

Markup is now expanded before it is inserted into the text. Practically, speaking this is breaking backwards compatibility. Reopening.

-- MichaelDaum - 04 Apr 2017

Uh... See https://trunk.foswiki.org/System/ReleaseNotes02x02#The_default_Attachment_Link_formats_have_changed.

When upgrading to Release 2.2, you need to escape macros in the ATTACHEDIMAGEFORMAT.

   * Set ATTACHEDIMAGEFORMAT = $percentIMAGE{"$name" size="200"}$percent

-- GeorgeClark - 04 Apr 2017

Okay. But then I would have expected a more standard escaping as in

   * Set ATTACHEDIMAGEFORMAT = $percentIMAGE{\"$name\" size=\"200\"}$percent

-- MichaelDaum - 04 Apr 2017

I think you only need to escape the quotes when the macro is itself contained within quotes. I guess it depends on how ATTACHEDIMAGEFORMAT is used. If it is itself expanded within another quote-delimited macro, then the quotes need escaping. I've not come across any cases like that, but indeed it might be needed.

-- GeorgeClark - 04 Apr 2017

So a "ACTION REQUIRED" block to the release notes or is backwards compatibility on this an absolute requirement?

I don't understand the concern about the need to escape the quotes (in the above example).

-- PhilippeKehl - 04 Apr 2017

The release notes are done. It's mentioned hopefully in sufficient detail.

The quotes don't really need to be escaped (or I don't understand it either). Normally when you have a %MACRO inside another %MACRO, quotes don't need escaping. That's because any quotes on the "inner" macro will be expanded away before the outer macro expands

But if you "delay" the inner macro with $percnt, you typically have to escape the quotes, so that they don't break the outer macro. However in this particular case, there really is no outer macro. The ATTACHEDIMAGEFORMAT is just expanded and rendered, so there are no outer quotes to be broken.

So unless Michael has some usage that I'm not familiar with, the escaped quotes are not needed.

-- GeorgeClark - 04 Apr 2017

That's also my understanding (re. macro in macro, delayed macro in macro).

I've seen that you've mentioned it in the release notes. I wondered if it should be stressed in a "action required" block. It's fine for me but I know people who only read the highlighted stuff..

-- PhilippeKehl - 05 Apr 2017

Added a more visible note to the ReleaseNotes.

-- Main.GeorgeClark - 11 Apr 2017 - 01:57

Commenting is disabled while not logged in