Sam, please could you take a look at this and let me know if you think it should go into 2.1 and/or master.

If it is conceptually OK, is there anything in the code you would like changed before this is submitted for integration?

Hi Tim,

You code changes look good

There was one thing I was looking at though, the way in which the cancel and yes strings are made available.
The strings are only there because other code has requested them. After a bit of debugging I found that they will be available on all pages because the filepicker module is included for ALL pages (which includes all of its sub modules and strings etc).

Certainly I think there are two bugs here although unrelated to your changes:
1. Filepicker module should be included only where it is needed - not on every page. This presently leads to loading and piles of strings, yui3 modules, and yui2 modules for every page.
2. confirm_action should be requiring its own strings in JS or alternatively sending them after making a get_string call.

And in fact there would be two more tasks to create issues for:
3. Uses of M.str.

.

should be reviewed and converted to M.util.get_string where possible to avoid needless JS errors from missing strings.
4. Find a way to require the dock module ONLY when needed, this would be if $THEME->enable_dock (of whatever it is) is true. This may require smarts as I think JS requirement is initialised before theme.

I think 1, 3, and 4 should most definitely be separate issues.
As for point 2 I will leave that up for you. You code at the moment is perfect to the current situation so feel free to put it up for integration as it is. Or feel free to add in point 2 (if you agree with my thinking on that subject of course) and then put it up for integration

As for the questions about backporting this to 2.1 as well, it is a small improvement and the code is very safe so I think yes it should be backported to 2.1 as well.
We can of course double check with Eloy on that point as well as he certainly has a better understanding or our protocols than I do, however I could just msg him on integration of the issue.

Cheers
Sam

Clicking looks good to me, up to you Tim whether you put it up for integration now or fix point 2 and then put it up

Eloy has been backporting other stuff of mine to 2.1, so I am sure he would say yes to this.

I will do and ammended commit with 2. I think it would be neater to do the null -> default in the PHP code, and always pass the button labels from PHP to JS.

Hmm. I cannot move the null -> default strings into PHP because M.util.show_confirm_dialog is used from a whole bunch of random places, so I cannot suddenly make args.cancellabel and args.continuelabel required.

Also, this mainly gets added using page->requires->event_handler, which does not seem to take a $jsmodule argument - so, no easy way to make strings available here.

So, while I completely agree with you that all this code needs a bit clean-up, I am afraid I cannot do it now. I will leave it to you to create the necessary MDL issues for the cleanup that is required in future. In the mean time, my changes do not make the situation any worse.

I did change the JS code to use an explicit if statement, because even sam (marshall) thought that args.cancellabel || M.str.moodle.cancel was evil, and sam likes the ? : operator.

I was also able to eliminate one direct reference to M.util.show_confirm_dialog.

Please put on both 2.1 and master branches.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Thanks Tim, this has been integrated now. I've merged to master and cherry-picked to MOODLE_21_STABLE as well.
I'll create issues for the points I noted now.

Cheers
Sam

tested and it works great.

Congratulations - this fix has just been released in the weeklies.