Moodle
  1. Moodle
  2. MDL-25394

Improved Support for Mobile Themes and Browser Detection

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      To test -

      Go to appearance/theme selector
      Specify a default theme
      This will display on any device

      Go to appearance/theme selector
      Specify a different mobile or a tablet theme or legacy (IE6) (depends what you have easily available to test).
      This should appear on the type of device. The default theme will remain on a standard desktop.

      Go to Advanced features and untick enable device detection. The default theme should be used on any device. Go to theme selector and the list of devices will not show - you can only specify a default theme.

      Go Advanced features, tick enable device detection. In Device Detection regular expression add /fox/ and under returns value add firefox. In the theme selector, select a different theme to use for firefox (similar should work for ie, chrome, etc)

      Show
      To test - Go to appearance/theme selector Specify a default theme This will display on any device Go to appearance/theme selector Specify a different mobile or a tablet theme or legacy (IE6) (depends what you have easily available to test). This should appear on the type of device. The default theme will remain on a standard desktop. Go to Advanced features and untick enable device detection. The default theme should be used on any device. Go to theme selector and the list of devices will not show - you can only specify a default theme. Go Advanced features, tick enable device detection. In Device Detection regular expression add /fox/ and under returns value add firefox. In the theme selector, select a different theme to use for firefox (similar should work for ie, chrome, etc)
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-25394
    • Rank:
      6513

      Description

      Please see the attached file for a spec for improved support for mobile themes and mobile browser detection. The Open University will commit to undertake the work involved.

      1. Moodle Mobile Specificatio1.doc
        29 kB
        Anthony Forth
      2. Moodle Mobile Specification.doc
        31 kB
        Anthony Forth
      1. choose_theme.jpg
        148 kB
      2. selector.jpg
        147 kB

        Issue Links

          Activity

          Hide
          Anthony Forth added a comment -

          I am not going to start coding this bug as per the revised spec attached. Further discussion at http://moodle.org/mod/forum/discuss.php?d=162872#p728548

          Show
          Anthony Forth added a comment - I am not going to start coding this bug as per the revised spec attached. Further discussion at http://moodle.org/mod/forum/discuss.php?d=162872#p728548
          Hide
          Anthony Forth added a comment -

          Code is now available for review in GitHub here -

          https://github.com/anthonyforth/moodle/compare/master...WIP-25394

          Show
          Anthony Forth added a comment - Code is now available for review in GitHub here - https://github.com/anthonyforth/moodle/compare/master...WIP-25394
          Hide
          Anthony Forth added a comment -

          Have merged Moodle/master to the branch.

          If anyone is able to look at this in the next couple of weeks, it would be very helpful to me as our local project is pushing ahead on the assumption it will be included.

          Show
          Anthony Forth added a comment - Have merged Moodle/master to the branch. If anyone is able to look at this in the next couple of weeks, it would be very helpful to me as our local project is pushing ahead on the assumption it will be included.
          Hide
          Martin Dougiamas added a comment -

          I had a quick look at the code and spec. In principle I'm for it. +1

          But I've not seen the interface changes yet or looked in detail at the code for security or anything. Can you post some screenshots?

          Show
          Martin Dougiamas added a comment - I had a quick look at the code and spec. In principle I'm for it. +1 But I've not seen the interface changes yet or looked in detail at the code for security or anything. Can you post some screenshots?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, just to say I went straight to install/upgrade code and found:

          • Some strange operations in config table (dropping and creating some columns there).
          • Missing install.xml representing those changes.

          Note I haven't looked at the rest of the patch. Just that thing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, just to say I went straight to install/upgrade code and found: Some strange operations in config table (dropping and creating some columns there). Missing install.xml representing those changes. Note I haven't looked at the rest of the patch. Just that thing. Ciao
          Hide
          Anthony Forth added a comment -

          Thanks to all for comments. I've revised the code...

          https://github.com/anthonyforth/moodle/compare/master...wip-25394

          Show
          Anthony Forth added a comment - Thanks to all for comments. I've revised the code... https://github.com/anthonyforth/moodle/compare/master...wip-25394
          Hide
          Anthony Forth added a comment -

          Screen dumps -
          selector.jpg - is the screen for showing the themes you've selected for each device type.
          choose_theme.jpg - is for selecting a theme for a device

          Hope that's clear.

          Show
          Anthony Forth added a comment - Screen dumps - selector.jpg - is the screen for showing the themes you've selected for each device type. choose_theme.jpg - is for selecting a theme for a device Hope that's clear.
          Hide
          Tim Hunt added a comment -

          Are we sure we want to store the theme settings in mdl_config as JSON?

          Show
          Tim Hunt added a comment - Are we sure we want to store the theme settings in mdl_config as JSON?
          Hide
          Jason Cameron added a comment -

          Just an opinion about the theme selector screen:

          Instead of having a row for simply 'legacy' or 'mobile', perhaps putting them as buttons (as they are now) would work better? What I mean by this is where you see 'select theme' next to Anomaly, have a button for the following:
          'Select Standard Theme'
          'Select Legacy Theme'
          'Select Mobile Theme'

          Upon viewing, and selecting, there can be a SQL query that changes that button text to:
          'Selected Standard Theme'
          'Selected Legacy Theme'
          'Selected Mobile Theme'

          this same query can disable these buttons to make it clear to the user that they already have the selected theme for the selected 'device'. Another option can be to add 'mobiletheme' to the Theme Settings where they select a mobile theme from a dropdown box.

          I am sorry if this is a bit confusing, I will try to get some screenshots as I download the code!

          Thanks for working on all of this!
          Jason Cameron

          Show
          Jason Cameron added a comment - Just an opinion about the theme selector screen: Instead of having a row for simply 'legacy' or 'mobile', perhaps putting them as buttons (as they are now) would work better? What I mean by this is where you see 'select theme' next to Anomaly, have a button for the following: 'Select Standard Theme' 'Select Legacy Theme' 'Select Mobile Theme' Upon viewing, and selecting, there can be a SQL query that changes that button text to: 'Selected Standard Theme' 'Selected Legacy Theme' 'Selected Mobile Theme' this same query can disable these buttons to make it clear to the user that they already have the selected theme for the selected 'device'. Another option can be to add 'mobiletheme' to the Theme Settings where they select a mobile theme from a dropdown box. I am sorry if this is a bit confusing, I will try to get some screenshots as I download the code! Thanks for working on all of this! Jason Cameron
          Hide
          Anthony Forth added a comment -

          Thanks for the feedback, Jason.

          We did some consultation on this when we wrote the original spec. There seemed to be a dislike for the current interface with the buttons, hence the way I've done it. The advantage is you can see all your theme selections in one place more easily.

          I am inclined not to change at this point as it was the result of that discussion rather than just my own personal preference.

          Show
          Anthony Forth added a comment - Thanks for the feedback, Jason. We did some consultation on this when we wrote the original spec. There seemed to be a dislike for the current interface with the buttons, hence the way I've done it. The advantage is you can see all your theme selections in one place more easily. I am inclined not to change at this point as it was the result of that discussion rather than just my own personal preference.
          Hide
          Martin Dougiamas added a comment -

          Anthony, I like the screenshots as I see them and I really want to see this in 2.1!

          Sam Hemelryk is going to do a peer review when he has a minute.

          Show
          Martin Dougiamas added a comment - Anthony, I like the screenshots as I see them and I really want to see this in 2.1! Sam Hemelryk is going to do a peer review when he has a minute.
          Hide
          Anthony Forth added a comment -

          Excellent. Keen to prioritise any work so this goes into 2.1.

          Show
          Anthony Forth added a comment - Excellent. Keen to prioritise any work so this goes into 2.1.
          Hide
          Anthony Forth added a comment -

          Have amended the switch code so that is now possible, but not compulsory, to have a switch link for themes applied to the default device type.

          Show
          Anthony Forth added a comment - Have amended the switch code so that is now possible, but not compulsory, to have a switch link for themes applied to the default device type.
          Hide
          Anthony Forth added a comment -

          ...it is also possible to use a switchdevice optional param to define the device type you wish to switch to (standard behaviour is for default to switch to mobile and all others to switch to default).

          Show
          Anthony Forth added a comment - ...it is also possible to use a switchdevice optional param to define the device type you wish to switch to (standard behaviour is for default to switch to mobile and all others to switch to default).
          Hide
          Sam Hemelryk added a comment - - edited

          Hi Anthony,

          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.

          1. upgrade.php: Is that block setting the version number of auth_manual meant to be there?
          2. upgrade.php: New upgrade block is reusing version number 2011022100.01 and should be calling upgrade_main_savepoint.
          3. 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)
          4. 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.
          5. 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.
          6. Variable names shouldn't contain underscores as per guidelines - http://docs.moodle.org/en/Development:Coding_style#Variables
          7. 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.
          8. 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)
          9. core_renderer::switchlinkdisplayed should be protected not private so that overriding renderers can control the placement of the switch link as well.
          10. 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)
          11. 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)
          12. 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)
          13. 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.
          14. switch.php url should be using PARAM_URL
          15. 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)
          16. A quick grep shows there are still a couple of places using $CFG->theme (see note c below)

          General notes:

          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.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - - edited Hi Anthony, 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) General notes: 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. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hmm sorry that is quite a load when I look at it there - don't hesitate to ask any questions or raise anything to discuss OK!

          Show
          Sam Hemelryk added a comment - Hmm sorry that is quite a load when I look at it there - don't hesitate to ask any questions or raise anything to discuss OK!
          Hide
          Tim Hunt added a comment -

          Sorry, butting in here, and I have not looked at the code. Just a small point. In point 14, don't you mean PARAM_LOCALURL?

          Show
          Tim Hunt added a comment - Sorry, butting in here, and I have not looked at the code. Just a small point. In point 14, don't you mean PARAM_LOCALURL?
          Hide
          Sam Hemelryk added a comment -

          Ohhh good catch! I had completely forgotten we had PARAM_LOCALURL and yes should be PARAM_LOCALURL

          Show
          Sam Hemelryk added a comment - Ohhh good catch! I had completely forgotten we had PARAM_LOCALURL and yes should be PARAM_LOCALURL
          Hide
          Anthony Forth added a comment - - edited

          Thanks for that. I've implemented everything you suggested (bar point 13, can't find another working way to do it).

          I've also implemented all of the general notes so it uses $CFG->theme, drops legacythemeinuse and uses deviceinuse and drops the legacy support code.

          Not sure where you are in the world. We have a public holiday in the UK this weekend and I am away visitng family until Thursday. I probably can't check back before then but will prioritise anything then so we can get this in.

          Show
          Anthony Forth added a comment - - edited Thanks for that. I've implemented everything you suggested (bar point 13, can't find another working way to do it). I've also implemented all of the general notes so it uses $CFG->theme, drops legacythemeinuse and uses deviceinuse and drops the legacy support code. Not sure where you are in the world. We have a public holiday in the UK this weekend and I am away visitng family until Thursday. I probably can't check back before then but will prioritise anything then so we can get this in.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Anthony!
          I'll look at it Monday morning now - if it's suitable for integration as it is I'll out it up for you and we can look to fix any remaining issues the following week.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Awesome thanks Anthony! I'll look at it Monday morning now - if it's suitable for integration as it is I'll out it up for you and we can look to fix any remaining issues the following week. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Alright - I've been looking at this again and have made a few more changes.

          Repository: git://github.com/samhemelryk/moodle.git
          Branch: wip-MDL-25394

          There are two diff URL's, the first shows the difference between your branch Anthony and mine (showing my changes), the second shows the overall changes.
          https://github.com/samhemelryk/moodle/compare/wip-MDL-25394-original...wip-MDL-25394
          https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25394

          Most of the changes I made were minor however worth noting Anthony is that I squashed the commits you had on your branch into a single commit (there were roughly 70 there I think).
          The changes I made were made as just on commit on top.

          I've asked Eloy to have a look at it when he gets online today, if he is happy with it then it will hopefully be integrated immediately ensuring its place in 2.1

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Alright - I've been looking at this again and have made a few more changes. Repository: git://github.com/samhemelryk/moodle.git Branch: wip- MDL-25394 There are two diff URL's, the first shows the difference between your branch Anthony and mine (showing my changes), the second shows the overall changes. https://github.com/samhemelryk/moodle/compare/wip-MDL-25394-original...wip-MDL-25394 https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25394 Most of the changes I made were minor however worth noting Anthony is that I squashed the commits you had on your branch into a single commit (there were roughly 70 there I think). The changes I made were made as just on commit on top. I've asked Eloy to have a look at it when he gets online today, if he is happy with it then it will hopefully be integrated immediately ensuring its place in 2.1 Cheers Sam
          Hide
          Jason Cameron added a comment -

          Out of curiosity, are you using code to change the theme or using the code to create a cookie, which will then change the theme? I am trying to get a system set up to download the code but I figured you could answer here in a more timely matter =)

          Show
          Jason Cameron added a comment - Out of curiosity, are you using code to change the theme or using the code to create a cookie, which will then change the theme? I am trying to get a system set up to download the code but I figured you could answer here in a more timely matter =)
          Hide
          Anthony Forth added a comment -

          it doesn't use a cookie. The code sets a Moodle user preference.

          Show
          Anthony Forth added a comment - it doesn't use a cookie. The code sets a Moodle user preference.
          Hide
          Jason Cameron added a comment -

          Just to play devil's advocate what happens in this situation:

          User has an iPad and does not need nor want the mobile theme. However, they also have an iPhone they use regularly and want to use the mobile theme for that device. Each time they log in, will they have to go into their user preferences and change their mobile preference?

          Show
          Jason Cameron added a comment - Just to play devil's advocate what happens in this situation: User has an iPad and does not need nor want the mobile theme. However, they also have an iPhone they use regularly and want to use the mobile theme for that device. Each time they log in, will they have to go into their user preferences and change their mobile preference?
          Hide
          Anthony Forth added a comment -

          Tablets are treated as a separate type of device, so switching on your phone will not impact your iPad.

          Show
          Anthony Forth added a comment - Tablets are treated as a separate type of device, so switching on your phone will not impact your iPad.
          Hide
          Jason Cameron added a comment -

          Gotcha. So this code is executed each time a page is visited by the mobile/tablet browser? Will this impact the performance/ability to cache the pages?

          Show
          Jason Cameron added a comment - Gotcha. So this code is executed each time a page is visited by the mobile/tablet browser? Will this impact the performance/ability to cache the pages?
          Hide
          Anthony Forth added a comment -

          Well... there's obviously some performance hit because it has to run code it didn't previously. However, it doesn't need any additional db queries or anything else like to make a significant performance hit.

          It doesn;t impact caching beyond the obviously implication of a page being rendered with a different theme if a user switches.

          Show
          Anthony Forth added a comment - Well... there's obviously some performance hit because it has to run code it didn't previously. However, it doesn't need any additional db queries or anything else like to make a significant performance hit. It doesn;t impact caching beyond the obviously implication of a page being rendered with a different theme if a user switches.
          Hide
          Sam Hemelryk added a comment -

          Congratulations Anthony this has been integrated now.
          Thank you for your work here it really is a cool improvement.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Congratulations Anthony this has been integrated now. Thank you for your work here it really is a cool improvement. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sweet!

          I've tested this with Safari, faking the agent info and also with the iphone and seems to work perfectly. Also, I've tested it with user themes and also works ok.

          Just two side comments to consider if that needs issue apart:

          • After selecting any theme, the "continue" button redirects to main admin/index.php. It's a bit annoying (usability) and IMO it should redirect to the themes selection page. It has specially more sense with this improvement applied.
          • the theme/switchdevice.php utility... uhm... it is used by the theme_switch_links() but it can be "abused" by anybody, isn't it? So anybody can, in practice, switch just by knowing the sesskey. Perhaps we should have some setting to enable both that info in footer & the execution of the script? Not sure, just imagining.
          Show
          Eloy Lafuente (stronk7) added a comment - Sweet! I've tested this with Safari, faking the agent info and also with the iphone and seems to work perfectly. Also, I've tested it with user themes and also works ok. Just two side comments to consider if that needs issue apart: After selecting any theme, the "continue" button redirects to main admin/index.php. It's a bit annoying (usability) and IMO it should redirect to the themes selection page. It has specially more sense with this improvement applied. the theme/switchdevice.php utility... uhm... it is used by the theme_switch_links() but it can be "abused" by anybody, isn't it? So anybody can, in practice, switch just by knowing the sesskey. Perhaps we should have some setting to enable both that info in footer & the execution of the script? Not sure, just imagining.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now upstream, yay! Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This is now upstream, yay! Many thanks!
          Hide
          Dan Poltawski added a comment -

          Hi,
          Just FYI - this change included a minor issue with an undefined constant (due to use of DEVELOPER_DEBUG rather than DEBUG_DEVELOPER) as discovered inMDL-28228

          Show
          Dan Poltawski added a comment - Hi, Just FYI - this change included a minor issue with an undefined constant (due to use of DEVELOPER_DEBUG rather than DEBUG_DEVELOPER) as discovered inMDL-28228

            People

            • Votes:
              5 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: