mod/resource/type/file/resource.class.php:

• mp3, flv, mediaplayer, quicktime, flash, mpeg, zip, pdf, rm are always embedded in an object tag with a menu displayed at the top of the page . The option "Keep page navigation visible on the same page" doesnt affect them.
• html and other (like mp4) are affected by the option.

The MDL-10021 fix didn't change the behaviors, it just added the possibility to choose between frame, noframe and object for HTML/unknown files

The resource code has been refactored in HEAD.
To fix this problem 1.9 does need to be refactored too. It will probably take a day of work so it's ok, but my main concerns are:

• It could bring some new bugs (for finally fixing a behavior that in fact didn't change). The code is messy.
• We probably would have a new wave of complains. (mp3, flv,... could be displayed differently, we need to alert people again,...)

If there is no mandatory needs to have mp3, pdf, displaying no menu or a frame, I'm really for doing no change, and just fixing the wording.

I'd really like to see all those types behaving as described in the current menu items ASAP. People should be able to use frames when they want to (as long as it is explicitly described so they know the pitfalls wrt accessibility).

I think fixing this is pretty simple, just removing the $embedded = true from those items in the switch statement to make those media types act like HTML and undefined media types do.

The main problem is going to be the change of behaviour in 1.9 stable.

1) Sites already in 1.9 are going to see slight changes in how courses are displaying.
2) Sites upgrading to 1.9 from earlier branches could see wierd changes in display ... not sure.

I'd like to see some community testing of all the upgrade permutations to make sure that we aren't going to introduce any major regressions here before we make these changes in 1.9.

if (empty($frameset) and !$embedded and !$inpopup and ($resource->options == "frame" || $resource->options == "objectframe") and empty($USER->screenreader)) {...}

and source code will have something like

<div class="resourcepdf"><object data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf"><param name="src" value="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" />To open this document, click on this link: <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf">PDF - Same Window - Nav no Frame</a></object></div>

If $embedded is false result seems to be more correct and moodle will use Yahoo javascript libaries for updating embedded object size with source code

<p><object id="embeddedhtml" data="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf" type="application/pdf" width="800" height="600">
alt : <a href="http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf">http://127.0.0.1/moodle/file.php?file=%2F3%2FTest_Page.pdf</a>
</object></p><script type="text/javascript">
//<![CDATA[
function resizeEmbeddedHtml() {
//calculate new embedded html height size
objectheight = YAHOO.util.Dom.getViewportHeight() - 120;
//the object tag cannot be smaller than a human readable size
if (objectheight < 200) { objectheight = 200; }
//resize the embedded html object
YAHOO.util.Dom.setStyle("embeddedhtml", "height", objectheight+"px");
YAHOO.util.Dom.setStyle("embeddedhtml", "width", "100%");
}
resizeEmbeddedHtml();
YAHOO.widget.Overlay.windowResizeEvent.subscribe(resizeEmbeddedHtml);
//]]>
</script>

Should not default value for $embedded for pdf be false like Jon in Jon's demo?

So let's do this for 1.9.6:

1) Remove the embedded=true setting for PDFs
2) Add a description text in the form near the "show navigation" menu to say "Some media types may ignore this setting"

Explantion of this issue and the two patches:
In resource settings page, the ' Show navigation' setting (previously called 'Keep navigation on the same page') had no effect on PDF files. PDF files were always displayed into an object tag.
The first patch fixed it. This patch has been committed.

Now PDF files can be displayed into a frame ('Yes, with frame'), into an object tag ('Yes, without frame') or just displayed without navigation ('No').

We need a DB upgrade patch because on previous installation, PDF were always displayed as 'Yes, without frame'. The default being 'No', they will be displayed without navigation now. We need to update all PDF entries into the database to put them back to 'Yes, without frame'.

Thanks a lot

1) Instead of getting all records with all columns, I'd get only those needed (id, type, reference, popup, options). And perhaps I'd use a recordset (I can imagine one site with zillions of pdfs).

2) As it's only a matter of updating one field, perhaps set_field() is better than update_record (far less to write).

3) IMO, you also need to check that popup is empty, because resources being showed in new window must not be updated to "objectframe". So condition must be:

if (empty($resource->popup && (empty($resource->options) || $resource->options == 'frame' )) {

4) Finally, IMO this should ALSO be merged into 2.0 resources (before the split into new modules happen). Please confirm this with Petr.

5) Edited: ".PDF" files aren't processed. Need to support them too.

Looking at the code the update_record('resource',$resource); is a SQL injection - there would have to be addslashes_recursive(). I agree with Eloy, this would definitely run out of memory on most sites - you have to use recordsets and specify the fetched fields.

Jerome, just noticed another minor thing: ".PDF" (uppercase) isn't fixed by the patch. (I've edited my original comment).

I tested that popup were not impacted but I'll add the test for security to a new patch.
I'll also add addslashes_recursive for security (I will unset everything except 'options' and 'id' so I think we should not have any need of slashes formatting anyway)
I'll use get_recordset instead of get_records. Thanks for the advice, I only had a quick look to recordset_to_array() though that get_records had a good name to be picked safetly

The new patch will be committed today after review, then the weekly build (on hold at this moment) will be rebuild and released.

Thanks

I think you could have saved the 2nd loop by doing all in the 1st one but your solution should work fine too (I hope nobody will have really zillions of resources or that $resources array will boom too, hehe).

Ciao

Also, from HQ Chat:

[11:18:20] <Eloy> btw.. that transformation should also be applied somewhere in the 1.9 => 2.0 migration (uhm... although it's destructive, replacing empties by "objectframe" twice. Uhm)
[11:18:57] <skodak> please file a separate issue for this - it will be in "url" and "resource" modules

Ciao

And you are not checking for SQL errors, so after the table does not exist error, it says 'upgrade completed successfully'

I am about to commit a fix.

I guess someone needs to re-roll the weekly build.