[TYPO3-core] RFC: #10853: [Feature] New Multimedia CE

[Steffen Kamper]

Hi Susanne,

Susanne Moog schrieb:
> Hi Steffen,
> 
> now I finally tested the patch, some more remarks:
> 
> From testing:
> - "Mode: Auto" does not work for wmv and swf files (probably because
> they aren't mentioned in tt_content.media.20.fileExtHandler)
>

yes, correct. I added them to TS

> From reading:
> 
> - What are those templavoila tags used for?:
> 
> <tx_templavoila>				
> <title>LLL:EXT:cms/locallang_ttc.xml:media.params.customEntry</title> 	
> </tx_templavoila>
> 

this is used in core for section title in flexforms. I have a pending 
patch for this in core.

> 
> - In your tslib_content patch there are some cleanup places like the
> following, which make it a bit harder to review the functional ones (but
> it's ok I think):
> 
> -		$content='';
> +		$content='';
>

this could happen with such big patch, but there is no functional change 
there.

> 
> - Isn't that the same as else if?:
> +	} else {
> +  	if (isset($value['mmParamSet'])) { [...] } }
> 

sure, but wasn't sure if something else would happen in else. but i 
don't see a problem here.

> - And a point not really relevant for this patch, but to be perhaps
> generally discussed on dev list later. How to make error messages look
> the same and able to be styled?:
> 
> return '<p style="background-color: yellow;">' .
> $GLOBALS['TSFE']->sL('LLL:EXT:cms/locallang_ttc.xml:media.noFile', true)
> . '</p>';
>

this is the way it's done now at other places too. When change that in 
general this part will be changed too.

> 
> - Nitpicking: I personally dislike multiple "ifs" in short notation in
> one line, as they are - for me - harder to understand. For example:
> 
> $flashvars = 'var flashvars = {' . ($flashvars ? (substr($flashvars, -1)
> == ',' ? substr(trim($flashvars), 0, -1) : $flashvars) : '') . '};';
> 

it's kind of taste. These expressions are often used within 
concatenating strings.

> 
> - This looks like it can't work (as you set layout and use
> alternativeContent in it, before alternativeContent is set) function
> SWFOBJECT:
> + $layout = str_replace('###SWFOBJECT###', '<div id="' .
> $replaceElemIdStr . '">' . $alternativeContent . '</div>', $layout);
> + $alternativeContent = $this->stdWrap($conf['alternativeContent'],
> $conf['alternativeContent.']);
>

yep, messed the order, thx.

> 
> - Why do you use different ways to set $layout in SWFOBJECT and
> QTOBJECT? Shouldn't you use the way in QTOBJECT in SWFOBJECT, too?
> 

thx, i use now same method in QTOBJECT
> 
> Sorry for this rather long mail, only the last two things are really
> relevant ;) If you answer those two (and perhaps add swf and wmv to
> fileExtHandler by default): +1 by reading and testing.
> 

great, thx. I added the updated patch with added both extensions to TS 
(also changed the order for better readability) . With this long patch i 
expected a long mail :-)

vg Steffen
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: new_media_v17.diff
Url: http://lists.netfielders.de/pipermail/typo3-team-core/attachments/20090531/678622ac/attachment-0001.txt