Moodle
  1. Moodle
  2. MDL-30600

MyMobile theme throws renderer error in mod choice activity

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.1
    • Component/s: Themes
    • Labels:
    • Rank:
      33400

      Description

      One of the renderers used in the mymobile theme to display the mod choice activity is outdated, causing a coding error.

        Activity

        Hide
        Michael de Raadt added a comment -

        I've put this on the STABLE backlog, but please keep working on it.

        Show
        Michael de Raadt added a comment - I've put this on the STABLE backlog, but please keep working on it.
        Hide
        John Stabinger added a comment -

        To test, visit a choice activity while using the MyMobile theme and confirm that no coding errors are reported.

        Show
        John Stabinger added a comment - To test, visit a choice activity while using the MyMobile theme and confirm that no coding errors are reported.
        Hide
        Rossiani Wijaya added a comment -

        Hi John,

        Thank you for supplying patch to improve Mobile theme.

        However I could not apply the patch due to trailing whitespaces.

        Could you update and resubmit the patch?

        Thanks

        Rosie.

        Show
        Rossiani Wijaya added a comment - Hi John, Thank you for supplying patch to improve Mobile theme. However I could not apply the patch due to trailing whitespaces. Could you update and resubmit the patch? Thanks Rosie.
        Hide
        John Stabinger added a comment -

        Could you be more specific? I just copied the code (exactly) from the renderer of the mod choice activity... Is the problem there?

        Show
        John Stabinger added a comment - Could you be more specific? I just copied the code (exactly) from the renderer of the mod choice activity... Is the problem there?
        Hide
        Rossiani Wijaya added a comment - - edited

        When I tried to apply your patch to my local machine it showed the following error message:

        @desktop$ git apply ~/patches_git/review/MDL-30600_2.diff
        /home/patches_git/review/MDL-30600_2.diff:9: trailing whitespace.
        
        /home/patches_git/review/MDL-30600_2.diff:10: trailing whitespace.
        
        /home/patches_git/review/MDL-30600_2.diff:27: trailing whitespace.
        
        /home/patches_git/review/MDL-30600_2.diff:35: trailing whitespace.
            
        /home/patches_git/review/MDL-30600_2.diff:194: trailing whitespace.
            
        warning: 5 lines add whitespace errors.
        
        
        Show
        Rossiani Wijaya added a comment - - edited When I tried to apply your patch to my local machine it showed the following error message: @desktop$ git apply ~/patches_git/review/MDL-30600_2.diff /home/patches_git/review/MDL-30600_2.diff:9: trailing whitespace. /home/patches_git/review/MDL-30600_2.diff:10: trailing whitespace. /home/patches_git/review/MDL-30600_2.diff:27: trailing whitespace. /home/patches_git/review/MDL-30600_2.diff:35: trailing whitespace. /home/patches_git/review/MDL-30600_2.diff:194: trailing whitespace. warning: 5 lines add whitespace errors.
        Hide
        John Stabinger added a comment -

        I'm sorry, but I'm just not seeing this. Could you provide line numbers or something? Again, the code was just copied from an existing moodle core module.

        Show
        John Stabinger added a comment - I'm sorry, but I'm just not seeing this. Could you provide line numbers or something? Again, the code was just copied from an existing moodle core module.
        Hide
        Rossiani Wijaya added a comment -

        Hi John,

        I fixed the trailing whitespaces error on the patch.

        Submitting this for integration review.

        Show
        Rossiani Wijaya added a comment - Hi John, I fixed the trailing whitespaces error on the patch. Submitting this for integration review.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        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

        Show
        Eloy Lafuente (stronk7) added a comment - 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
        Hide
        Sam Hemelryk added a comment -

        Hi Rosie,

        Is this change needed in the MOODLE_22_STABLE branch by chance?
        I am thinking that given master and MOODLE_22_STABLE have not diverged yet that it is, but could you please check.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Rosie, Is this change needed in the MOODLE_22_STABLE branch by chance? I am thinking that given master and MOODLE_22_STABLE have not diverged yet that it is, but could you please check. Cheers Sam
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        Yes, the change is needed for MOODLE_22_STABLE.

        Thank you.

        Show
        Rossiani Wijaya added a comment - Hi Sam, Yes, the change is needed for MOODLE_22_STABLE. Thank you.
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        I'm sending this back presently.
        The level of duplication here is not great and can be avoided.
        At a glance theme_mymobile_mod_choice_renderer extending plugin_renderer_base isn't a great solution, by the looks if it extends mod_choice_renderer instead this sort of duplication can be avoided. (calls to this would default to the parent if there was nothing overridden).

        Show
        Sam Hemelryk added a comment - Hi guys, I'm sending this back presently. The level of duplication here is not great and can be avoided. At a glance theme_mymobile_mod_choice_renderer extending plugin_renderer_base isn't a great solution, by the looks if it extends mod_choice_renderer instead this sort of duplication can be avoided. (calls to this would default to the parent if there was nothing overridden).
        Hide
        Rossiani Wijaya added a comment -

        Hi Sam,

        I updated the patch according to you suggestion.

        Re-submitting for integration review.

        Show
        Rossiani Wijaya added a comment - Hi Sam, I updated the patch according to you suggestion. Re-submitting for integration review.
        Hide
        Aparup Banerjee added a comment -

        grabbing this as requested by Rosie/Sam

        Show
        Aparup Banerjee added a comment - grabbing this as requested by Rosie/Sam
        Hide
        Aparup Banerjee added a comment -

        Thanks, thats been integrated into master and 2.2.x

        Tester, please report any other notices too (it would be helpful towards getting all these sorted in the mymobile theme)

        Show
        Aparup Banerjee added a comment - Thanks, thats been integrated into master and 2.2.x Tester, please report any other notices too (it would be helpful towards getting all these sorted in the mymobile theme)
        Hide
        Aparup Banerjee added a comment -

        works for me - no notices under choice in my iphone + mymobile theme

        Show
        Aparup Banerjee added a comment - works for me - no notices under choice in my iphone + mymobile theme
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The git and cvs repositories are happy receiving your very first contribution to Moodle for 2012. Happy new year! Closing, ciao

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: