davidherney / moodle-format_onetopic

Course format Onetopic to LMS Moodle
GNU General Public License v3.0
19 stars 44 forks source link

Inherit hidden 2 #107

Closed james-cnz closed 2 years ago

james-cnz commented 2 years ago

This is another attempt to close #83. It's smaller than #98, and it seems to work, including for activity navigation, which was a problem before. It probably needs more testing though. I'm also not sure if performance will be OK on large courses.

james-cnz commented 2 years ago

I tried a bit of performance testing on this. I did 20 page loads in my Moodle sandbox of a course with 50 sections, with and without the change, with performance info turned on. With the change averaged only about 1% slower. I don't know how accurate this is, but it seems promising.

davidherney commented 2 years ago

Thanks @james-cnz ... I am going to test it in order to integrate in the new release.

davidherney commented 2 years ago

Hi @james-cnz ...

I did merge your code with some little changes in the next branch: https://github.com/davidherney/moodle-format_onetopic/tree/MOODLE_39_inherit_hidden

Please, help me to test before upgrade the new official version.

Saludos

james-cnz commented 2 years ago

I think there is an error. (I don't think it's my change though, I think it's related to #97.) If a first child tab is hidden, no children are shown when the parent tab is selected. It looks like this is due to renderer.php line: if ($displaysection == $section - 1) { this would be better as: if ($displaysection == $parentsection->section) { But this is still not quite right, because there can be an error when section 0 is displayed before the tabs and section 1 is set to be a child tab. Also this needs to be changed: } while ($parentformatoptions['level'] == 1 && $prevsectionindex >= 0); to, e.g.: } while ($parentformatoptions['level'] == 1 && $prevsectionindex >= $firstsection); and add: $firstsection = ($course->realcoursedisplay == COURSE_DISPLAY_MULTIPAGE) ? 1 : 0;

davidherney commented 2 years ago

Not really because the first tab always is a parent tab, the level parameter is omitted in this case with the condition: $parenttab == null in line: if ($formatoptions['level'] == 0 || $parenttab == null) {

I was testing both cases and not found error. Please, test it in a course.

Gracias

Saludos

james-cnz commented 2 years ago

I did test, but I guess I didn't explain very well. Steps are: 1/ Create new Onetopic course with 3 topics. 2/ Set hidden sections completely invisible. 3/ Set Topic 2 to child and hidden. 4/ Set Topic 3 to child. 5/ Open Topic 1. 6/ Switch role to student. Topic 3 should be visible, but isn't. 7/ change renderer.php line 369 from: if ($displaysection == $section - 1) { to: if ($displaysection == $parentsection->section) { Problem is fixed (maybe need to purge caches to see). Except now there is a new problem instead: 8/ Set Section 0 before the tabs. 9/ Set topic 1 to child. Topics 2 and 3 should be visible, but aren't. 10/ Change renderer.php line 359 from: } while ($parentformatoptions['level'] == 1 && $prevsectionindex >= 0); to: } while ($parentformatoptions['level'] == 1 && $prevsectionindex >= $firstsection); and add before: $firstsection = ($course->realcoursedisplay == COURSE_DISPLAY_MULTIPAGE) ? 1 : 0; New problem is fixed (again, maybe need to purge caches to see).

davidherney commented 2 years ago

ah... ok... you have all the reason. The step by step example is useful to test. Thanks for it.

I fixed it now as you suggest.

Saludos