Moodle

MDL-10021 is the bug where these settings were last looked at

Same behavior as before the fix of MDL-10021
I guess it's just a wording error, the option should be named:
"Keep page navigation visible on the same page for html file"

Here is the situation:

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.

Thanks Jerome!

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.

no worries I'll do the change, add a patch here and ask the community on the forums.

it is not as simple as it, remove embedded = true will display the OS media player for MP3 file for example, we will lose all specific tags to the different extension, I need to refactor a bit the code...

just a reminder to update: http://docs.moodle.org/en/File_or_website_link#Keep_page_navigation_visible_on_the_same_page

I attached a patch. We still need to edit resource db/upgrade.php in order to manage different 1.9 config upgrade. I'll do it later however you can test the patch now.

I tested it quickly... The original issue was about showing pdf files like html pages - not the other embedded files - but even after this patch PDF (Keep page navigation visible on the same page - Yes, without frame) does not look "correct" (whole width and height) if $embedded is true for pdf because resource.class.php will skip over

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?

If javascript is disabled result is of course different...

attached new patch

I attached a fix only PDF patch

If we decide to fix only PDF, as a dev I'm happy there will be less potential bug. However "Keep navigation" still need some rewording explaining that it doesn't work with some specific files (MP3,...). Some suggestions?

http://docs.moodle.org/en/File_or_website_link#Keep_page_navigation_visible_on_the_same_page says:
"Note that this option is normally not necessary for media types such as movies, audio files and flash files, as without this option turned on they will be embedded within a navigable page."
so we probably should only fix PDF (at this moment PDF, ZIP and image are also considered as media).

OK, after discussion .... we might create more issues if we tinker too much with how 1.9.x works.

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"

Attached another patch:
1. Fixed PDF only
2. Added comment under the setting saying that some media file ignore this setting
3. Renamed 'Keep navigation visible on the same page' into 'Show navigation'

committed into 1.9
Thank you.

We should set the option to "yes, without frame" for people upgrading otherwise they will have to change all their PDF resource settings... fixing it

Hi Eloy,
can you review the upgrade database patch and commit it?

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

The patch looks ok but:

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.

2.0 has a branch new code that should not have any problem like this, of course if there is any need for DB upgrades the existing migration code should be updated.

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.

About 2.0 Petr, is it a matter of "cloning" the upgrade block in 2.0 mod/resource (before the migration happens, of course). Or is it a matter of changing the migration itself to fix those resources ?

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

I looks like this new upgrade step could be implemented as some extra ifs during migration of both 'url' and 'resource' module.

Hi all,
thanks for the reviews

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

quickly add a new patch for Martin

Hi I'm on my iPhone at the hospital. I've just tested the patch I attached before to get my cab, it works well, ready for reviewing and being committed hopefully. Thanks Martin

A new optimized patch for site with Zillion of PDF

+1 here

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

Petr came back with the best solution, attached what I hope to be the last patch

hehe, this ain't gonna work for bloody oracle, remember that ugly one space hack for empty strings there

another patch to review

+1

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

Resolving thanks everybody

good evolution! Tested all combinations seems to work.

You have a hard-coded mdl_

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.

OK. Re-fixed.

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

Re-rolled ! Thanks Tim, I missed that point!

Thanks Tim