added a comment - - edited
I've just been having a look at this and this change seems like an absolutely fantastic idea. The work so far is definatly heading in the right direction.
I've made the following notes while reading over the patch (I've read it through several times now), the top section is trivial to minor things I spotted while reading (it looks like alot but don't worry most of them will take you less than a minute I'm sure), further on there are some general notes I made that represent more significant changes.
- upgrade.php: Is that block setting the version number of auth_manual meant to be there?
- upgrade.php: New upgrade block is reusing version number 2011022100.01 and should be calling upgrade_main_savepoint.
- admin_setting_devicedetectregex should not set the name, desc etc in the constructor, it should require it to be provided like other settings to allow this setting type to be used more than once (should that ever be required... it does look like a cool setting type)
- I think the devicedetectregex setting description needs to be improved. I didn't understand what it was until I read the code. Perhaps an example could be provided... we can always ask Helen to help with this as well, she is very good with this sort of thing. Also document what is to be provided e.g. full regex delimiters inc.
- It would be great if we could validate the regex somehow for the devicedetectregex setting (the first one I tried I didn't put a delimeter in and got a pile of notices). In the interests of getting this in ASAP perhaps we can create a bug report for this to see it improved after it has been integrated.
- Variable names shouldn't contain underscores as per guidelines - http://docs.moodle.org/en/Development:Coding_style#Variables
- That is one massive line in moodlelib with the regex's to pick mobile devices. I think those two regex's should be split out to vars to shorten that line.
- core_renderer::theme_switch_links shouldn't use $PAGE it has $this->page. (the $USER global should be removed as well as it isn't used)
- core_renderer::switchlinkdisplayed should be protected not private so that overriding renderers can control the placement of the switch link as well.
- Looking at moodle_page::legacy_theme_support I am wondering when $CFG->themes will not be set up? (See note c below ... I wrote more later)
- get_device_type You need to check that HTTP_USER_AGENT is set and likely return default if not (A quick search shows code checking if it is empty)
- get_user_switched_theme returns null at one point - should return false just for accuracy sake (I wondered whether this should be renamed as it is more about getting device switches.. food for thought I'll leave that up to you)
- get_selected_theme_for_device_type The check to make sure the theme exists is incorrect, themes can exist within either the theme's directory of $CFG->themedir if set - You either check manually (grep for CFG->themedir) or try using theme_config::find_theme_location.
- switch.php url should be using PARAM_URL
- For switch theme why not use key => value pairs rather than an array of objects? This way you wouldn't need to check if the user pref already exists you would just overwrite it if it does? (I think there were other places where similar thing was being done that could be improved in the same way)
- A quick grep shows there are still a couple of places using $CFG->theme (see note c below)
a/ After looking for a while at _legacythemeinuse I think that it should be removed and that what it was doing should be done for everything but the default.
From what I quickly noted if _legacethemeinuse == true then the following would occur:
- A class was added to the body tag.
- A message was printed in the footer stating that a legacy theme was in use.
I think that it would be nice if we could do that same for all but the default so that you would always have a body class that themer's could utilise (legacytheme, tablettheme, mobiletheme).
As for the notice I see you already have the theme_switch_links function doing that which is cool, perhaps just improve the string.
Actually having looked at that I think it would be cool if the string were 'You are currently using the mobile theme, switch to the <a>standard</a> theme' or something - again Helen is the person to talk to about strings.
Perhaps the best option would be to store the device type being displayed for as a protected moodle_page var and expose it through a magic method so that future code may use it as well (I see this idea was tossed around in the spec blocks etc). It would of course mean initialising it at an appropriate time.
b/ I'm not entirely convinced that changing from $CFG->theme to the json encoded CFG->themes is a good move. I think using separate CFG variables would be a better option e.g. $CFG->theme, themelegacy, thememobile, themetablet.
The advantage of doing this is that it would be backwards compatible with existing code (in that it at least would find the default theme).
Then again that is my personal preference, I will talk to Eloy about it and see what his prefence is (although reading through the bug again after reading it I see Tim raised this same point so perhaps we should go ahead with it).
c/ I don't like legacy_theme_support I think we should try to come up with a way of avoiding that.
Given that the upgrade code is going to remove the current $CFG->theme and set $CFG->themes I think if $CFG->themes wasn't set it would be safe to assume that an upgrade/install was occuring and that it would be fine to fall back to the default.
As I said I certainly think this going in the right direction, and as I'm sure you know Martin is keen to get this in for 2.1.
If you have any questions or want to discuss any of the points go for it (if you think I've missed the point of anything etc let me know!), this is a high priority item on my list now so I'll aim to get back to you with answers etc as quickly as possible.
Otherwise I'm looking forward to seeing how you get along.
I should add this doesn't constitute a full review sorry - I went as far as I could before I kept coming back to the same areas and while I have used the new setting to test the basic operation of things I havn't given it a proper/complete test.