Moodle
  1. Moodle
  2. MDL-38856 META: Issues around Bootstrapbase and Clean theme
  3. MDL-40065

Bootstrap Theme always sends content to "side-pre", even when there isn't one

    Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5, 2.6
    • Fix Version/s: 2.5.1
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Select the 'Clean' theme.
      2. Logout and go to the login screen.
      3. Confirm that the layout is correct with no blocks and one content area that fills the screen 100% - FireBug should show that the area does. This is a test for 'column1.php'.
      4. Login and go to the 'Purge All Caches' screen because it uses the 'admin' layout as defined in 'bootstrapbase/config.php' which uses 'column2.php'.
      5. Confirm that with a LTR language the blocks are on the left.
      6. Switch to a RTL language and confirm the blocks are the same but are now on the right.
      7. Temporarily edit 'bootstrapbase/config.php' and change the 'side-pre' to 'side-post' value for the 'admin' layout for both the 'regions' and 'defaultregion' array keys (save changes).
      8. Refresh the 'Purge All Caches' page, still with the RTL language enabled, confirm that the blocks are now on the left and no errors occur.
      9. Switch back to a LTR language and confirm the same blocks are now on the right and no errors occur.
        END TEST
      Show
      Select the 'Clean' theme. Logout and go to the login screen. Confirm that the layout is correct with no blocks and one content area that fills the screen 100% - FireBug should show that the area does. This is a test for 'column1.php'. Login and go to the 'Purge All Caches' screen because it uses the 'admin' layout as defined in 'bootstrapbase/config.php' which uses 'column2.php'. Confirm that with a LTR language the blocks are on the left. Switch to a RTL language and confirm the blocks are the same but are now on the right. Temporarily edit 'bootstrapbase/config.php' and change the 'side-pre' to 'side-post' value for the 'admin' layout for both the 'regions' and 'defaultregion' array keys (save changes). Refresh the 'Purge All Caches' page, still with the RTL language enabled, confirm that the blocks are now on the left and no errors occur. Switch back to a LTR language and confirm the same blocks are now on the right and no errors occur. END TEST
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.5 Branch:
      wip-MDL-40065-m25_4
    • Pull Master Branch:
      wip-MDL-40065-master_4
    • Rank:
      384

      Description

      As requested by Damyon Wiese, this is now a late addition to MDL-39824 as it ha been integrated. It does solve the original issue too and impoves the solution in MDL-39824 with less code changes and smaller layout files.

      Old description:

      There is an error in the logic of bootstrapbase and clean themes layout. general.php which results in side-pre always being output, even if the $THEME->layouts for the page affected excludes side-pre.

      Lines 143-162 have the error in bootstrapbase/layout/general.php, and lines 172 to 202 have the error in the clean theme.

      The logic error is that the code tests to see if $layout is not content-only. If true then the code renders the html for pre-and-post or for side-pre-only, and drops by error into "if (!right_to_left())" regardless of what $layout is set to.

      Therefore if $layout is set to side-post-only there is no HTML code generated, but "echo $OUTPUT->blocks_for_region('side-pre')" is executed regardless.

      One possible fix is to change

                      if (!right_to_left()) {
                          echo $OUTPUT->blocks_for_region('side-pre');
                      } else if ($hassidepost) {
                          echo $OUTPUT->blocks_for_region('side-post');
                      } ?>
      

      to

                      if(!$layout === 'side-post-only') {
                          if (!right_to_left()) {
                              echo $OUTPUT->blocks_for_region('side-pre');
                          } else if ($hassidepost) {
                              echo $OUTPUT->blocks_for_region('side-post');
                          }
                      }
      

      However, it is probably better to fix the logic completely so that the echo $OUTPUT->blocks_for_region only occurs when $layout is 'pre-and-post' or 'side-pre-only'

      How to test:

      Turn on Debug to Developer and Theme Design Mode on

      Copy the entire section of $THEME->layouts from bootstrapbase/config.php and paste into config.php of clean/config.php

      In config.php of theme standard, and theme clean (please alter both!), change the front page layout to:

          // The site home page.
          'frontpage' => array(
              'file' => 'general_frontpage.php',
              'regions' => array('side-pre'),
              'defaultregion' => 'side-pre',
              'options' => array('nonavbar'=>true),
      

      Purge caches after saving the two config.php files

      Select Site Admininstration / Appearance / Theme / Theme Selector and select Clean theme. Go to the front page (ALL should be OK)

      Select Site Admininstration / Appearance / Theme / Theme Selector and select Standard theme. Go to the home page (ALL should be OK)

      Re edit the two config.php $THEME->layout to:

          // The site home page.
          'frontpage' => array(
              'file' => 'general_frontpage.php',
              'regions' => array('side-post'),
              'defaultregion' => 'side-post',
              'options' => array('nonavbar'=>true),
      

      Purge caches again

      Go to front page - all is still OK

      Select Site Admininstration / Appearance / Theme / Theme Selector and select Clean theme. Go to the front page

      Now you get the Theme header but before you get to the main body Moodle throws an exception - "Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre"

      Fix the logic error in clean/layout/general.php

      Purge all caches

      The error should be corrected.

      NOTE: There appears to be another bug but I will raise a second issue for that one. The problem is, if blocks are configured to appear in side-post, then when the $THEME-layout for a page says only use side-pre, the blocks disappear when using theme Clean. This doesn't happen with theme Standard. Please link this other issue as it may be related.

      1. MDL-40065_2cols.jpg
        14 kB
      2. mdl-40065-tt.png
        13 kB
      3. spost-ltr.png
        56 kB
      4. spost-rtl.png
        52 kB
      5. spre-ltr.png
        54 kB
      6. spre-rtl.png
        51 kB

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          Thanks again Brian. I've set this as a "blocker" so that it gets fixed befor MDL-40067. I'll also add a few watchers including Nadav Kavalerchik who introduced the ltr/rtl side swap in Base theme last year. The only other theme in Moodle to use that 'side swap' is Afterburner, so you might like to see how that theme's logic works to compare.

          Cheers
          Mary

          Show
          Mary Evans added a comment - Thanks again Brian. I've set this as a "blocker" so that it gets fixed befor MDL-40067 . I'll also add a few watchers including Nadav Kavalerchik who introduced the ltr/rtl side swap in Base theme last year. The only other theme in Moodle to use that 'side swap' is Afterburner, so you might like to see how that theme's logic works to compare. Cheers Mary
          Hide
          Mary Evans added a comment -

          Adding watcher as this looks to be a bug, I have replicated this problem so needs fixing fast.

          I have added FRONTEND to fix version so hope this message has not duplicated emails for you all.

          Show
          Mary Evans added a comment - Adding watcher as this looks to be a bug, I have replicated this problem so needs fixing fast. I have added FRONTEND to fix version so hope this message has not duplicated emails for you all.
          Hide
          Brian Merritt added a comment -

          Note - the code change I suggested does NOT fix the problem entirely. The problem is still there, it just needs a better fix. When testing, it is necessary to test combinations of pre only, post only, pre and post, neither.

          Show
          Brian Merritt added a comment - Note - the code change I suggested does NOT fix the problem entirely. The problem is still there, it just needs a better fix. When testing, it is necessary to test combinations of pre only, post only, pre and post, neither.
          Hide
          Mary Evans added a comment -

          That's OK Brian. Hopefully someone from FRONTEND team will spot this and take a look as there are lots of changes going one with block regions and such.

          Show
          Mary Evans added a comment - That's OK Brian. Hopefully someone from FRONTEND team will spot this and take a look as there are lots of changes going one with block regions and such.
          Hide
          Brian Merritt added a comment -

          No problem - technically my code did fix the problem, but doesn't deal with issues if future releases create new page layout options - but in that case the theme would probably have to be re-written anyway.

          Show
          Brian Merritt added a comment - No problem - technically my code did fix the problem, but doesn't deal with issues if future releases create new page layout options - but in that case the theme would probably have to be re-written anyway.
          Hide
          moodle.com added a comment -

          This may be fixed by another issue Sam is working on. FrontEnd will investigate.

          Show
          moodle.com added a comment - This may be fixed by another issue Sam is working on. FrontEnd will investigate.
          Hide
          Gareth J Barnard added a comment - - edited

          Does this just need a simple guard like:

          if ($showsidepre) {
              echo $OUTPUT->blocks_for_region('side-pre');
          }
          

          and vice versa for 'side-post'? Where '$showsidepre' etc. are already in 'general.php'.

          Show
          Gareth J Barnard added a comment - - edited Does this just need a simple guard like: if ($showsidepre) { echo $OUTPUT->blocks_for_region('side-pre'); } and vice versa for 'side-post'? Where '$showsidepre' etc. are already in 'general.php'.
          Hide
          Gareth J Barnard added a comment -

          Dear Brian Merritt,

          I've been staring at he code for a while to spot the logic error as despite:

                          if (!right_to_left()) {
                              echo $OUTPUT->blocks_for_region('side-pre');
                          } else if ($hassidepost) {
                              echo $OUTPUT->blocks_for_region('side-post');
                          } ?>
          

          there is flip flop RTL code above for '$layout':

          $layout = 'pre-and-post';
          if ($showsidepre && !$showsidepost) {
              if (!right_to_left()) {
                  $layout = 'side-pre-only';
              } else {
                  $layout = 'side-post-only';
              }
          } else if ($showsidepost && !$showsidepre) {
              if (!right_to_left()) {
                  $layout = 'side-post-only';
              } else {
                  $layout = 'side-pre-only';
              }
          } else if (!$showsidepost && !$showsidepre) {
              $layout = 'content-only';
          }
          

          Which negates the 'regardless of what $layout' is set to assertion.

          Show
          Gareth J Barnard added a comment - Dear Brian Merritt , I've been staring at he code for a while to spot the logic error as despite: if (!right_to_left()) { echo $OUTPUT->blocks_for_region('side-pre'); } else if ($hassidepost) { echo $OUTPUT->blocks_for_region('side-post'); } ?> there is flip flop RTL code above for '$layout': $layout = 'pre-and-post'; if ($showsidepre && !$showsidepost) { if (!right_to_left()) { $layout = 'side-pre-only'; } else { $layout = 'side-post-only'; } } else if ($showsidepost && !$showsidepre) { if (!right_to_left()) { $layout = 'side-post-only'; } else { $layout = 'side-pre-only'; } } else if (!$showsidepost && !$showsidepre) { $layout = 'content-only'; } Which negates the 'regardless of what $layout' is set to assertion.
          Hide
          Nadav Kavalerchik added a comment -

          That's my code. Hope it is not causing too much trouble
          If so, please suggest a better way to implement the "block region switch" (needed in RTL mode)

          Show
          Nadav Kavalerchik added a comment - That's my code. Hope it is not causing too much trouble If so, please suggest a better way to implement the "block region switch" (needed in RTL mode)
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Brian Merritt and Nadav Kavalerchik,

          I think I've found the issue - the layout code:

          <?php if ($layout !== 'content-only') {
                    if ($layout === 'pre-and-post') { ?>
                      <aside class="span4 desktop-first-column">
              <?php } else if ($layout === 'side-pre-only') { ?>
                      <aside class="span3 desktop-first-column">
              <?php } ?>
                    <div id="region-pre" class="block-region">
                    <div class="region-content">
                    <?php
                          if (!right_to_left()) {
                              echo $OUTPUT->blocks_for_region('side-pre');
                          } else if ($hassidepost) {
                              echo $OUTPUT->blocks_for_region('side-post');
                          } ?>
                    </div>
                    </div>
                    </aside>
              <?php if ($layout === 'pre-and-post') {
                    ?></div></div><?php // Close row-fluid and span9.
              }
          

          needs changing to:

          <?php if ($layout !== 'content-only') { 
                    if ($layout !== 'side-post-only') {
                        if ($layout === 'pre-and-post') { ?>
                          <aside class="span4 desktop-first-column">
                  <?php } else if ($layout === 'side-pre-only') { ?>
                          <aside class="span3 desktop-first-column">
                  <?php } ?>
                        <div id="region-pre" class="block-region">
                        <div class="region-content">
                        <?php
                              if (!right_to_left()) {
                                  echo $OUTPUT->blocks_for_region('side-pre');
                              } else if ($hassidepost) {
                                  echo $OUTPUT->blocks_for_region('side-post');
                              } ?>
                        </div>
                        </div>
                        </aside>
                  <?php if ($layout === 'pre-and-post') {
                        ?></div></div><?php // Close row-fluid and span9.
                  }
              }
          

          because lower down we have:

          if ($layout === 'side-post-only' OR $layout === 'pre-and-post')
          

          and that is not logically mirrored above for the 'pre' block code. So need 'if ($layout !== 'side-post-only')'.

          Tested with both LTR and RTL languages and a 'config.php' (snippet) of:

              'frontpage' => array(
                  'file' => 'general.php',
                  'regions' => array('side-pre'),
                  'defaultregion' => 'side-pre',
                  'options' => array('nonavbar'=>true),
              ),
          

          Interestingly, when I have both 'side-pre' and 'side-post' my calendar block is set to appear in 'side-post' but when I have 'side-pre' only as in the test it is rendered in 'side-pre' despite 'echo $OUTPUT->blocks_for_region('side-pre');' happening only.

          Plus, I think there needs to be:

          //$showsidepre = ($hassidepre && !$PAGE->blocks->region_completely_docked('side-pre', $OUTPUT));
          //$showsidepost = ($hassidepost && !$PAGE->blocks->region_completely_docked('side-post', $OUTPUT));
          // TODO: Fix when docking fixed.
          $showsidepre = $hassidepre;
          $showsidepost = $hassidepost;
          

          because of the no-dock issue.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Brian Merritt and Nadav Kavalerchik , I think I've found the issue - the layout code: <?php if ($layout !== 'content-only') { if ($layout === 'pre-and-post') { ?> <aside class= "span4 desktop-first-column" > <?php } else if ($layout === 'side-pre-only') { ?> <aside class= "span3 desktop-first-column" > <?php } ?> <div id= "region-pre" class= "block-region" > <div class= "region-content" > <?php if (!right_to_left()) { echo $OUTPUT->blocks_for_region('side-pre'); } else if ($hassidepost) { echo $OUTPUT->blocks_for_region('side-post'); } ?> </div> </div> </aside> <?php if ($layout === 'pre-and-post') { ?></div></div><?php // Close row-fluid and span9. } needs changing to: <?php if ($layout !== 'content-only') { if ($layout !== 'side-post-only') { if ($layout === 'pre-and-post') { ?> <aside class= "span4 desktop-first-column" > <?php } else if ($layout === 'side-pre-only') { ?> <aside class= "span3 desktop-first-column" > <?php } ?> <div id= "region-pre" class= "block-region" > <div class= "region-content" > <?php if (!right_to_left()) { echo $OUTPUT->blocks_for_region('side-pre'); } else if ($hassidepost) { echo $OUTPUT->blocks_for_region('side-post'); } ?> </div> </div> </aside> <?php if ($layout === 'pre-and-post') { ?></div></div><?php // Close row-fluid and span9. } } because lower down we have: if ($layout === 'side-post-only' OR $layout === 'pre-and-post') and that is not logically mirrored above for the 'pre' block code. So need 'if ($layout !== 'side-post-only')'. Tested with both LTR and RTL languages and a 'config.php' (snippet) of: 'frontpage' => array( 'file' => 'general.php', 'regions' => array('side-pre'), 'defaultregion' => 'side-pre', 'options' => array('nonavbar'=> true ), ), Interestingly, when I have both 'side-pre' and 'side-post' my calendar block is set to appear in 'side-post' but when I have 'side-pre' only as in the test it is rendered in 'side-pre' despite 'echo $OUTPUT->blocks_for_region('side-pre');' happening only. Plus, I think there needs to be: //$showsidepre = ($hassidepre && !$PAGE->blocks->region_completely_docked('side-pre', $OUTPUT)); //$showsidepost = ($hassidepost && !$PAGE->blocks->region_completely_docked('side-post', $OUTPUT)); // TODO: Fix when docking fixed. $showsidepre = $hassidepre; $showsidepost = $hassidepost; because of the no-dock issue. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear moodle.com, Mary Evans, Brian Merritt, Nadav Kavalerchik et al,

          I think I have a fix as stated above, I can supply a patch and assign to me if desired as it was stated 'FrontEnd will investigate' so don't want to tread on anybodies toes.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear moodle.com , Mary Evans , Brian Merritt , Nadav Kavalerchik et al, I think I have a fix as stated above, I can supply a patch and assign to me if desired as it was stated 'FrontEnd will investigate' so don't want to tread on anybodies toes. Cheers, Gareth
          Hide
          Nadav Kavalerchik added a comment -

          You have my blessing
          (Thank you)

          Show
          Nadav Kavalerchik added a comment - You have my blessing (Thank you)
          Hide
          Mary Evans added a comment - - edited

          @Gareth,

          $showsidepre and $showsidepost should never have been added in Bootstrapbase, I have mentioned this before elsewhere.

          The logic should follow base theme as Nadav had it originally, which is, I think, how you have it set out in your example now. However, FRONTEND team are developing a hew version of Bootstrapbase for 2.6 but as I reported yesterday there is a problem with their logic too by the look of things.

          Have you checked your fix with what is currently being used in the new version that Sam is building Gareth?

          Show
          Mary Evans added a comment - - edited @Gareth, $showsidepre and $showsidepost should never have been added in Bootstrapbase, I have mentioned this before elsewhere. The logic should follow base theme as Nadav had it originally, which is, I think, how you have it set out in your example now. However, FRONTEND team are developing a hew version of Bootstrapbase for 2.6 but as I reported yesterday there is a problem with their logic too by the look of things. Have you checked your fix with what is currently being used in the new version that Sam is building Gareth?
          Hide
          Mary Evans added a comment -

          Go for it Gareth, the worst that can happen is that it is rejected!

          Show
          Mary Evans added a comment - Go for it Gareth, the worst that can happen is that it is rejected!
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans and Nadav Kavalerchik,

          Thanks and will do. Not checked with Sam's code yet so will look at now and implement this tomorrow

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans and Nadav Kavalerchik , Thanks and will do. Not checked with Sam's code yet so will look at now and implement this tomorrow Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          At a cursory glance, Sam's new 'blocks' method does not have block existence detection and there is nothing in the new replacement 'columns' layout files. But then it is a work in progress and I've not read it all.

          Show
          Gareth J Barnard added a comment - At a cursory glance, Sam's new 'blocks' method does not have block existence detection and there is nothing in the new replacement 'columns' layout files. But then it is a work in progress and I've not read it all.
          Hide
          Gareth J Barnard added a comment -

          Assigned to me, I intend to supply a patch within twenty hours. If I don't please feel free to take over.

          Show
          Gareth J Barnard added a comment - Assigned to me, I intend to supply a patch within twenty hours. If I don't please feel free to take over.
          Hide
          Gareth J Barnard added a comment -

          Possible conflict with MDL-39824 - need to see what happens before proceeding. MDL-39824 may fix this.

          Show
          Gareth J Barnard added a comment - Possible conflict with MDL-39824 - need to see what happens before proceeding. MDL-39824 may fix this.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          MDL-39824 is up for integration review now.
          For those who haven't seen it introduces several layout files to bootstrapbase and clean and greatly simplifies the layout by removing all of this PHP logic.
          The new layouts always render block regions and use CSS to hide the empty regions. This works around the issues that come about when editing blocks on a page and resolves several issues commonly encountered when using alternative block regions.

          Gareth, I imagine in the process of this clean up that I have resolved this issue.
          I gave it a quick test earlier after Mary commented there about MDL-40067. That showed up what I believe was this issue and I've since addressed it with another simple commit.
          This week I am tasked with integration reviewing and as we're out of our sync period it is going to be a busy week. I'll not likely get a chance to give this a proper test unfortunately.
          Perhaps you could have a look at MDL-39824 and see what you think, its still at a point where we can change things easily if the issue is still present.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, MDL-39824 is up for integration review now. For those who haven't seen it introduces several layout files to bootstrapbase and clean and greatly simplifies the layout by removing all of this PHP logic. The new layouts always render block regions and use CSS to hide the empty regions. This works around the issues that come about when editing blocks on a page and resolves several issues commonly encountered when using alternative block regions. Gareth, I imagine in the process of this clean up that I have resolved this issue. I gave it a quick test earlier after Mary commented there about MDL-40067 . That showed up what I believe was this issue and I've since addressed it with another simple commit. This week I am tasked with integration reviewing and as we're out of our sync period it is going to be a busy week. I'll not likely get a chance to give this a proper test unfortunately. Perhaps you could have a look at MDL-39824 and see what you think, its still at a point where we can change things easily if the issue is still present. Many thanks Sam
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          Thank you and will do / am doing. It's late here so will have a test of MDL-39824 tomorrow too (I think Mary is checking now).

          My +1 as a humble contributor for your work on this

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , Thank you and will do / am doing. It's late here so will have a test of MDL-39824 tomorrow too (I think Mary is checking now). My +1 as a humble contributor for your work on this Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Hi all,

          This is now pending the outcome of MDL-39824.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi all, This is now pending the outcome of MDL-39824 . Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Is this a sub-task of MDL-38856 ?

          Show
          Gareth J Barnard added a comment - Is this a sub-task of MDL-38856 ?
          Hide
          Mary Evans added a comment -

          It wasn't but is now!

          Show
          Mary Evans added a comment - It wasn't but is now!
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Peer reviewer / Integrator.

          This is a late addition to MDL-39824 and at the request of Damyon Wiese on that issue has been put here because that issue had already been integrated. I have supplied two branches because it enhances the MDL-39824 improvement which has just been integrated during the on-sync period. I realise that this may not be integrated during the on-sync period but request for consistency of the MDL-39824 solution that it is in both branches.

          Also it does fix the issue of having:

              'admin' => array(
                  'file' => 'columns2.php',
                  'regions' => array('side-post'),
                  'defaultregion' => 'side-post',
              ),
          

          In 'config.php' which causes 'Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre' for LTR and 'تم الكشف عن خطأ بالكود، يجب إصلاحه من قبل مبرمج: Trying to reference an unknown block region side-pre' for RTL.

          Kind regards,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Peer reviewer / Integrator. This is a late addition to MDL-39824 and at the request of Damyon Wiese on that issue has been put here because that issue had already been integrated. I have supplied two branches because it enhances the MDL-39824 improvement which has just been integrated during the on-sync period. I realise that this may not be integrated during the on-sync period but request for consistency of the MDL-39824 solution that it is in both branches. Also it does fix the issue of having: 'admin' => array( 'file' => 'columns2.php', 'regions' => array('side-post'), 'defaultregion' => 'side-post', ), In 'config.php' which causes 'Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-pre' for LTR and 'تم الكشف عن خطأ بالكود، يجب إصلاحه من قبل مبرمج: Trying to reference an unknown block region side-pre' for RTL. Kind regards, Gareth
          Hide
          Mary Evans added a comment -

          Gareth, I'm not altogether sure the way you have done this commit is the correct way to go about it.

          As I see it all of Sam's commits are in your patch. These have been integrated. Would it not have been better if you created a branch based on Sams changes to add your changes?

          Show
          Mary Evans added a comment - Gareth, I'm not altogether sure the way you have done this commit is the correct way to go about it. As I see it all of Sam's commits are in your patch. These have been integrated. Would it not have been better if you created a branch based on Sams changes to add your changes?
          Hide
          Stuart Lamour added a comment -

          as commented in https://tracker.moodle.org/browse/MDL-39824 the right to left display can be solved in quite a simple way outside the display logic.

          try something like :
          /* Order pre/post block regions for left to right. */
          if ($hassidepre)

          { $side_pre = $OUTPUT->blocks_for_region('side-pre'); }

          if($hassidepost)

          { $side_post = $OUTPUT->blocks_for_region('side-post'); }

          if (right_to_left()) {
          if($hassidepost)

          { $side_pre = $OUTPUT->blocks_for_region('side-post');}

          if ($hassidepre)

          { $side_post = $OUTPUT->blocks_for_region('side-pre'); }

          }
          before setting the layout var.

          This lets you take out all the ltr checks later.
          https://github.com/stuartlamour/moodle/blob/master/theme/clean/layout/vars.php

          Show
          Stuart Lamour added a comment - as commented in https://tracker.moodle.org/browse/MDL-39824 the right to left display can be solved in quite a simple way outside the display logic. try something like : /* Order pre/post block regions for left to right. */ if ($hassidepre) { $side_pre = $OUTPUT->blocks_for_region('side-pre'); } if($hassidepost) { $side_post = $OUTPUT->blocks_for_region('side-post'); } if (right_to_left()) { if($hassidepost) { $side_pre = $OUTPUT->blocks_for_region('side-post');} if ($hassidepre) { $side_post = $OUTPUT->blocks_for_region('side-pre'); } } before setting the layout var. This lets you take out all the ltr checks later. https://github.com/stuartlamour/moodle/blob/master/theme/clean/layout/vars.php
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          I did create branchs based on Sams changes! I checked out his branch and then made a branch from that. The reason they are temporarily showing up here is because Sam's changes are in the integration area and not the 'MOODLE_25_STABLE' and 'master' branches and hence GitHub will change when Sam's commits show up in those branches. You will see my single commit at the bottom of the list. Sam's commits in the list are one and the same version hashes with his branches in MDL-39824 and hence will be no conflicts between the two. So if the URL's are different you get:

          https://github.com/gjb2048/moodle/compare/samhemelryk:wip-MDL-39824-m25...wip-MDL-40065-m25_2

          and

          https://github.com/gjb2048/moodle/compare/samhemelryk:wip-MDL-39824-m26...wip-MDL-40065-master_2

          but these would be invalid URL's to place into the tracker as they do not refer to the main Moodle repository and could not be rationally pulled from.

          I hope this makes sense, cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , I did create branchs based on Sams changes! I checked out his branch and then made a branch from that. The reason they are temporarily showing up here is because Sam's changes are in the integration area and not the 'MOODLE_25_STABLE' and 'master' branches and hence GitHub will change when Sam's commits show up in those branches. You will see my single commit at the bottom of the list. Sam's commits in the list are one and the same version hashes with his branches in MDL-39824 and hence will be no conflicts between the two. So if the URL's are different you get: https://github.com/gjb2048/moodle/compare/samhemelryk:wip-MDL-39824-m25...wip-MDL-40065-m25_2 and https://github.com/gjb2048/moodle/compare/samhemelryk:wip-MDL-39824-m26...wip-MDL-40065-master_2 but these would be invalid URL's to place into the tracker as they do not refer to the main Moodle repository and could not be rationally pulled from. I hope this makes sense, cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Stuart Lamour,

          But! That solution is using 'blocks_for_region' and the simplification uses 'blocks' which incorporates the swapping code internally. And so unfortunately the solution:

          /* Order pre/post block regions for left to right. */
          if ($hassidepre)
          { $side_pre = $OUTPUT->blocks_for_region('side-pre'); }
          if($hassidepost)
          { $side_post = $OUTPUT->blocks_for_region('side-post'); }
          if (right_to_left()) {
          if($hassidepost)
          { $side_pre = $OUTPUT->blocks_for_region('side-post');}
          if ($hassidepre)
          { $side_post = $OUTPUT->blocks_for_region('side-pre'); }
          }
          

          is for an older version of the layout files before 'columnsX.php' was implemented. And I came up with a two line fix in https://github.com/gjb2048/moodle/commit/a3427f408da32ae0c1d152db330f67aad275e34c

          I hope this makes sense, cheers,

          Gareth

          P.S.

          I have to admit that https://github.com/stuartlamour/moodle/blob/master/theme/clean/layout/vars.php looks very neat

          Show
          Gareth J Barnard added a comment - - edited Dear Stuart Lamour , But! That solution is using 'blocks_for_region' and the simplification uses 'blocks' which incorporates the swapping code internally. And so unfortunately the solution: /* Order pre/post block regions for left to right. */ if ($hassidepre) { $side_pre = $OUTPUT->blocks_for_region('side-pre'); } if ($hassidepost) { $side_post = $OUTPUT->blocks_for_region('side-post'); } if (right_to_left()) { if ($hassidepost) { $side_pre = $OUTPUT->blocks_for_region('side-post');} if ($hassidepre) { $side_post = $OUTPUT->blocks_for_region('side-pre'); } } is for an older version of the layout files before 'columnsX.php' was implemented. And I came up with a two line fix in https://github.com/gjb2048/moodle/commit/a3427f408da32ae0c1d152db330f67aad275e34c I hope this makes sense, cheers, Gareth P.S. I have to admit that https://github.com/stuartlamour/moodle/blob/master/theme/clean/layout/vars.php looks very neat
          Hide
          Mary Evans added a comment -

          @Gareth,

          I'm not convinced, but will accept your explanation and wait and see, as the proof of the pudding, is in the eating.

          Show
          Mary Evans added a comment - @Gareth, I'm not convinced, but will accept your explanation and wait and see, as the proof of the pudding, is in the eating.
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          To be honest as this is the first time I have done this I'm not 100% sure but the way I did it, what is showing up on GitHub, the current understanding of the state of things and the fact that 'integration' is a non-public area for testing leads me to be fairly hopeful .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , To be honest as this is the first time I have done this I'm not 100% sure but the way I did it, what is showing up on GitHub, the current understanding of the state of things and the fact that 'integration' is a non-public area for testing leads me to be fairly hopeful . Cheers, Gareth
          Hide
          Mary Evans added a comment -

          Well if you tested this using Hebrew (he) (I find that easier on the eye than Arabic) when editing is on and moving blocks and no errors show up then it must be right. But I don't have time to test as am not Moodling today...well not fixing bugs. I have a thousand other things to do.

          Show
          Mary Evans added a comment - Well if you tested this using Hebrew (he) (I find that easier on the eye than Arabic) when editing is on and moving blocks and no errors show up then it must be right. But I don't have time to test as am not Moodling today...well not fixing bugs. I have a thousand other things to do.
          Hide
          Gareth J Barnard added a comment -

          Umm, I tested in Arabic

          Show
          Gareth J Barnard added a comment - Umm, I tested in Arabic
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans, Sam Hemelryk, Damyon Wiese and Dan Poltawski,

          Now that MDL-39824 has been integrated is this now ready to go? In testing with MDL-39824 I did find that the 'side-post' issue still existed and this I believe fixes it but obviously I need a peer to confirm.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Sam Hemelryk , Damyon Wiese and Dan Poltawski , Now that MDL-39824 has been integrated is this now ready to go? In testing with MDL-39824 I did find that the 'side-post' issue still existed and this I believe fixes it but obviously I need a peer to confirm. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Both branches rebased. Now a clean single commit on both accounts.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Both branches rebased. Now a clean single commit on both accounts. Cheers, Gareth
          Hide
          Mary Evans added a comment - - edited

          Just looking at the code for columns2.php

          There is something not quite right with it. In the Test Instructions you say that if you set Admin layout to side-post for both 'array' and 'default' region, then when in RTL it should stay on the right? That can't be right as Side-post to RTL community is on the left.

          Show
          Mary Evans added a comment - - edited Just looking at the code for columns2.php There is something not quite right with it. In the Test Instructions you say that if you set Admin layout to side-post for both 'array' and 'default' region, then when in RTL it should stay on the right? That can't be right as Side-post to RTL community is on the left.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          I know it does not sound right, but I found that even before the change using 'general.php' etc. that in the administration layout both the side-pre and side-post blocks are combined. For example, the calendar block is set up to be in 'side-post' but in the administration layout it appears in 'side-pre'.

          So, are you saying that when set to 'side-post' that the block should be on the right for LTR and left for RTL? And vice-versa for 'side-pre'? I think it already does this in this fix but need to do a quick test - humm, does not even for the 'report' layout. I think you could be right and there is a small bit of logic missing here. Need to think.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , I know it does not sound right, but I found that even before the change using 'general.php' etc. that in the administration layout both the side-pre and side-post blocks are combined. For example, the calendar block is set up to be in 'side-post' but in the administration layout it appears in 'side-pre'. So, are you saying that when set to 'side-post' that the block should be on the right for LTR and left for RTL? And vice-versa for 'side-pre'? I think it already does this in this fix but need to do a quick test - humm, does not even for the 'report' layout. I think you could be right and there is a small bit of logic missing here. Need to think. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          Yes, you are right. Working on a fix now.

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Yes, you are right. Working on a fix now. Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          Both branches updated with the fix and screen shots produced to illustrate. I cannot control the output of 'blocks' but it is being supplied with the correct parameter. This is 'side-pre' for 'side-pre' in the config.php file for LTR, 'side-post' for 'side-post' in the config.php file for LTR, 'side-post' for 'side-pre' in the config.php file for RTR (a swap is made in 'blocks()' to swap it back to 'side-pre') and 'side-pre' for 'side-post' in the config.php file for RTR (a swap is made in 'blocks()' to swap it back to 'side-post').

          Now this all is potentially getting out of hand and I could remove the code:

          if (!$ltr) { // In RTL the sides are reversed, so swap the 'blocks' method parameter.
              // Swap....
              $temp = $pre;
              $pre = $post;
              $post = $temp;
          }
          

          If I used the method 'blocks_for_region()' which is called by 'blocks()' but this would be inconsistent with 'columns3.php' and I'm not sure from memory what 'blocks()' does beyond swapping and calling 'blocks_for_region()'.

          There must be an easier way to do this!!

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Both branches updated with the fix and screen shots produced to illustrate. I cannot control the output of 'blocks' but it is being supplied with the correct parameter. This is 'side-pre' for 'side-pre' in the config.php file for LTR, 'side-post' for 'side-post' in the config.php file for LTR, 'side-post' for 'side-pre' in the config.php file for RTR (a swap is made in 'blocks()' to swap it back to 'side-pre') and 'side-pre' for 'side-post' in the config.php file for RTR (a swap is made in 'blocks()' to swap it back to 'side-post'). Now this all is potentially getting out of hand and I could remove the code: if (!$ltr) { // In RTL the sides are reversed, so swap the 'blocks' method parameter. // Swap.... $temp = $pre; $pre = $post; $post = $temp; } If I used the method 'blocks_for_region()' which is called by 'blocks()' but this would be inconsistent with 'columns3.php' and I'm not sure from memory what 'blocks()' does beyond swapping and calling 'blocks_for_region()'. There must be an easier way to do this!!
          Hide
          Gareth J Barnard added a comment -

          Additional:

          The code:

          if ($hassidepre) {
              $useblock = $pre;
              /*
               This deals with the side to show the blocks on.
               If we have a 'side-pre' then the blocks are on the left for LTR and right for RTL.
              */
              if ($ltr) {
                  $left = true;
              } else {
                  $left = false;
              }
          } else {
              $useblock = $post;
              /*
               This deals with the side to show the blocks on.
               If we have a 'side-post' then the blocks are on the right for LTR and left for RTL.
              */
              if ($ltr) {
                  $left = false;
              } else {
                  $left = true;
              }
          }
          

          Just works out where the blocks should be positioned, so on the left for 'side-pre' in LTR and 'side-post' in RTL and on the right for 'side-pre' in LTR and 'side-post' in RTL.

          Show
          Gareth J Barnard added a comment - Additional: The code: if ($hassidepre) { $useblock = $pre; /* This deals with the side to show the blocks on. If we have a 'side-pre' then the blocks are on the left for LTR and right for RTL. */ if ($ltr) { $left = true ; } else { $left = false ; } } else { $useblock = $post; /* This deals with the side to show the blocks on. If we have a 'side-post' then the blocks are on the right for LTR and left for RTL. */ if ($ltr) { $left = false ; } else { $left = true ; } } Just works out where the blocks should be positioned, so on the left for 'side-pre' in LTR and 'side-post' in RTL and on the right for 'side-pre' in LTR and 'side-post' in RTL.
          Hide
          Mary Evans added a comment -

          I've not tested this yet, but just wanting to say that as this layout is only for 2 columns, which is either side-pre-only or side-post-only.

          For side-pre-only, the class selector for region-main needs to be 'pull-right' whereas for side-post-only we cannot assume it is float: left so we need to tell it. So for side-post-only the class selector for region-main needs to be 'pull-left'.

          So contrary to your logic, the state of side-pre-only or side-post-only is NOT totally reliant on which language you are using but which side is the default. The state changes only in the direction of region-main when in RTL. The content of the blocks has to remain the same, so it is just the position which is KEY.

          Let me explain: The blocks for RTL are still side-pre blocks the only difference is that they need to be displayed on the right.

          In a side-post-only page the blocks which are set to be displayed in side-post will be on the right by default. In RTL the same column should contain side-post blocks but the region main needs to be set to pull-right, so that the blocks are on the left.

          You could set up two states, then reverse them for RTL, which is the way Nadav did it in base theme.

          Hope that helps?

          Show
          Mary Evans added a comment - I've not tested this yet, but just wanting to say that as this layout is only for 2 columns, which is either side-pre-only or side-post-only. For side-pre-only, the class selector for region-main needs to be 'pull-right' whereas for side-post-only we cannot assume it is float: left so we need to tell it. So for side-post-only the class selector for region-main needs to be 'pull-left'. So contrary to your logic, the state of side-pre-only or side-post-only is NOT totally reliant on which language you are using but which side is the default. The state changes only in the direction of region-main when in RTL. The content of the blocks has to remain the same, so it is just the position which is KEY. Let me explain: The blocks for RTL are still side-pre blocks the only difference is that they need to be displayed on the right. In a side-post-only page the blocks which are set to be displayed in side-post will be on the right by default. In RTL the same column should contain side-post blocks but the region main needs to be set to pull-right, so that the blocks are on the left. You could set up two states, then reverse them for RTL, which is the way Nadav did it in base theme. Hope that helps?
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          Cheers:

          "So contrary to your logic, the state of side-pre-only or side-post-only is NOT totally reliant on which language you are using but which side is the default. The state changes only in the direction of region-main when in RTL. The content of the blocks has to remain the same, so it is just the position which is KEY.

          Let me explain: The blocks for RTL are still side-pre blocks the only difference is that they need to be displayed on the right.

          In a side-post-only page the blocks which are set to be displayed in side-post will be on the right by default. In RTL the same column should contain side-post blocks but the region main needs to be set to pull-right, so that the blocks are on the left."

          This is what I have now done, so in a LTR language 'side-pre' is on the left and 'side-post' on the right and vice versa with RTL. The question is the use of the 'blocks()' method instead of 'blocks_for_region()'.

          And I know it's for two columns hence the '2' in the name of the file and the screen shots I have uploaded ( wink ). And it is for 'side-pre' or 'side-post', but for some reason with some blocks like 'Calendar' they decide of their own accord to change regions if the region they belong to is not there.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , Cheers: "So contrary to your logic, the state of side-pre-only or side-post-only is NOT totally reliant on which language you are using but which side is the default. The state changes only in the direction of region-main when in RTL. The content of the blocks has to remain the same, so it is just the position which is KEY. Let me explain: The blocks for RTL are still side-pre blocks the only difference is that they need to be displayed on the right. In a side-post-only page the blocks which are set to be displayed in side-post will be on the right by default. In RTL the same column should contain side-post blocks but the region main needs to be set to pull-right, so that the blocks are on the left." This is what I have now done, so in a LTR language 'side-pre' is on the left and 'side-post' on the right and vice versa with RTL. The question is the use of the 'blocks()' method instead of 'blocks_for_region()'. And I know it's for two columns hence the '2' in the name of the file and the screen shots I have uploaded ( wink ). And it is for 'side-pre' or 'side-post', but for some reason with some blocks like 'Calendar' they decide of their own accord to change regions if the region they belong to is not there. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Ok, I think the logic is getting lost here, so created and uploaded a truth table 'mdl-40065-tt.png'

          Show
          Gareth J Barnard added a comment - Ok, I think the logic is getting lost here, so created and uploaded a truth table 'mdl-40065-tt.png'
          Hide
          Mary Evans added a comment -

          I've just been making an image to describe what I wrote.

          Like I said I have not tested your revised work, or even looked at the code, other than what you have written here. I was just saying in case you were interpreting rtl swap sides wrongly. I'll go and L@^@K at the code and see what sort of mess you have made (wink, wink).

          By the way, I am assuming that Sam's simplified theme wasn't working correctly before your intended revised patch?

          Show
          Mary Evans added a comment - I've just been making an image to describe what I wrote. Like I said I have not tested your revised work, or even looked at the code, other than what you have written here. I was just saying in case you were interpreting rtl swap sides wrongly. I'll go and L@^@K at the code and see what sort of mess you have made (wink, wink). By the way, I am assuming that Sam's simplified theme wasn't working correctly before your intended revised patch?
          Hide
          Mary Evans added a comment -

          Oh...I see you have posted images too?

          Show
          Mary Evans added a comment - Oh...I see you have posted images too?
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          His code was working for 'side-pre' but with a change to the grid in 'moodle/core.less' so to reduce this which I thought was a bit obscure, did not quite look right and cause theme designers headaches when changing the layout I came up with the logic. Plus then realised that the code falls over if you use 'side-post' only in the 'config.php' file so this actually fixes a bug too .

          Cheers,

          Gareth

          P.S. I had "I was just saying in case you were interpreting rtl swap sides wrongly." and had not done this until the latest change

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , His code was working for 'side-pre' but with a change to the grid in 'moodle/core.less' so to reduce this which I thought was a bit obscure, did not quite look right and cause theme designers headaches when changing the layout I came up with the logic. Plus then realised that the code falls over if you use 'side-post' only in the 'config.php' file so this actually fixes a bug too . Cheers, Gareth P.S. I had "I was just saying in case you were interpreting rtl swap sides wrongly." and had not done this until the latest change
          Hide
          Gareth J Barnard added a comment -

          I see you've added 'MDL-40065_2cols.jpg' - this looks like it matches the images I've already uploaded

          Show
          Gareth J Barnard added a comment - I see you've added ' MDL-40065 _2cols.jpg' - this looks like it matches the images I've already uploaded
          Hide
          Mary Evans added a comment - - edited

          I'm very, very, very impressed with this Gareth. So much so I think that Admin should be made Side-Post as the default side as all the grader pages where all the scrolling is needed is done in the report layout.

          If you check this out side-post works better with Form pages as they look better, presentation wise, more aesthetic, at least I think so. To see what I mean, using your Shoelace theme, after making Admin default region 'side-post' checkout the Shoelace custom settings page. It seems to be easier to read. Also the Theme selector seems to look better that way round too. Your eye is drawn to the left naturally and there you find the themes.

          Also you have done a great job with this.

          Thanks

          Show
          Mary Evans added a comment - - edited I'm very, very, very impressed with this Gareth. So much so I think that Admin should be made Side-Post as the default side as all the grader pages where all the scrolling is needed is done in the report layout. If you check this out side-post works better with Form pages as they look better, presentation wise, more aesthetic, at least I think so. To see what I mean, using your Shoelace theme, after making Admin default region 'side-post' checkout the Shoelace custom settings page. It seems to be easier to read. Also the Theme selector seems to look better that way round too. Your eye is drawn to the left naturally and there you find the themes. Also you have done a great job with this. Thanks
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Mary Evans,

          Why thank you . I'll have a bash at adapting some layouts in Shoelace .

          Cheers,

          Gareth

          P.S. Loving the new concept of 'theme_clean_get_html_for_settings()'

          Show
          Gareth J Barnard added a comment - - edited Dear Mary Evans , Why thank you . I'll have a bash at adapting some layouts in Shoelace . Cheers, Gareth P.S. Loving the new concept of 'theme_clean_get_html_for_settings()'
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've just been unravelling this now.
          After looking at it for a while it looks like you may be working around a bug that I introduced in MDL-39824.
          With my changes the block regions were always swapped over, however really if only one of the regions exist (in this case side-pre) there should be no switching done.
          I've created a branch based upon what you've done here Gareth that fixes the above (switching only happens if both regions are present) and have tweaked things based upon that.
          Could you please have a look at it and see what you think?
          Diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-40065-m26
          Repo: git://github.com/samhemelryk/moodle.git
          Branch: wip-MDL-40065-m26

          At the moment I am reopening this, itd be really great to determine if this was a bug in my previous work and if so whether that branch above solves the remaining problems for this issue.
          With the switching bug resolved things look simplier and we don't need nearly as much code coming back into the 2 column layout.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've just been unravelling this now. After looking at it for a while it looks like you may be working around a bug that I introduced in MDL-39824 . With my changes the block regions were always swapped over, however really if only one of the regions exist (in this case side-pre) there should be no switching done. I've created a branch based upon what you've done here Gareth that fixes the above (switching only happens if both regions are present) and have tweaked things based upon that. Could you please have a look at it and see what you think? Diff: https://github.com/samhemelryk/moodle/commit/wip-MDL-40065-m26 Repo: git://github.com/samhemelryk/moodle.git Branch: wip- MDL-40065 -m26 At the moment I am reopening this, itd be really great to determine if this was a bug in my previous work and if so whether that branch above solves the remaining problems for this issue. With the switching bug resolved things look simplier and we don't need nearly as much code coming back into the 2 column layout. Many thanks Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Nadav Kavalerchik added a comment -

          Please check that when DND blocks from left (side-pre) column to right (side-post) column, position is saved after turning editing mode. (I remember we had issues with that in the past. lib/ajax/blocks.php , action=="move")

          Show
          Nadav Kavalerchik added a comment - Please check that when DND blocks from left (side-pre) column to right (side-post) column, position is saved after turning editing mode. (I remember we had issues with that in the past. lib/ajax/blocks.php , action=="move")
          Hide
          Gareth J Barnard added a comment -

          Dear Nadav Kavalerchik,

          Umm, there will not be a drag and drop position for the other block in editing mode because this is a TWO COLUMN LAYOUT!!!!! Drag and drop positions for both block columns only exist in a three column layout even when one is hidden and therefore this test needs to be on the MDL-40204 issue and not this.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Nadav Kavalerchik , Umm, there will not be a drag and drop position for the other block in editing mode because this is a TWO COLUMN LAYOUT!!!!! Drag and drop positions for both block columns only exist in a three column layout even when one is hidden and therefore this test needs to be on the MDL-40204 issue and not this. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          Thank you for your input. With reference to the truth table in this issue - https://tracker.moodle.org/secure/attachment/33087/mdl-40065-tt.png - and your proposed changes - https://github.com/samhemelryk/moodle/commit/wip-MDL-40065-m26:

          1. Having two blocks defined was not the issue because it was a two column layout and having two defined in 'config.php' would have been silly on the part of the theme designer. The issue is that 'blocks()' swaps regardless for RTL - please see the truth table - but using the negated XOR gate logic and eliminating the false-false row to become the 'AND' that you have leads to issues with mistakes in 'config.php' - see below.
          2. Currently I believe that your patch will fail with 'side-post' only in LTR because of:

          echo $OUTPUT->blocks('side-pre', 'span3'.$classextra);
          

          where 'side-pre' is fixed and no 'side-post' is swapped by:

          if ($this->blocks->is_known_region($regionwas) && $this->blocks->is_known_region($regionnow)) {
          

          when you have something like:

              'admin' => array(
                  'file' => 'columns2.php',
                  'regions' => array('side-post'),
                  'defaultregion' => 'side-post',
              ),
          

          3. But, you are posing an interesting thought that 'blocks()' should only be used for RTL swapping in a three column layout - see MDL-40204 and in fact to reduce the logic of this (using the truth table as a guide and the fact that still need to have the '$useblock' logic because of the need to support 'side-post' as mentioned in '2') then given the code of 'blocks()' then this needs to use 'blocks_for_region()' instead, but I'm loathed to do that as the code does other stuff too with the attributes which falls in line with MDL-39824:

              public function blocks($region, $classes = array(), $tag = 'aside') {
                  $displayregion = $this->page->apply_theme_region_manipulations($region);
                  $classes = (array)$classes;
                  $classes[] = 'block-region';
                  $attributes = array(
                      'id' => 'block-region-'.preg_replace('#[^a-zA-Z0-9_\-]+#', '-', $displayregion),
                      'class' => join(' ', $classes),
                      'data-blockregion' => $displayregion,
                      'data-droptarget' => '1'
                  );
                  return html_writer::tag($tag, $this->blocks_for_region($region), $attributes);
              }
          

          So coming back full circle the method 'apply_theme_region_manipulations()' does this:

              public function apply_theme_region_manipulations($region) {
                  if ($this->blockmanipulations && isset($this->blockmanipulations[$region])) {
                      $regionwas = $region;
                      $regionnow = $this->blockmanipulations[$region];
                      if ($this->blocks->is_known_region($regionwas) && $this->blocks->is_known_region($regionnow)) {
                          // Both the before and after regions are known so we can swap them over.
                          return $regionnow;
                      }
                      // We didn't know about both, we won't swap them over.
                      return $regionwas;
                  }
                  return $region;
              }
          

          so that no swapping is performed on a two column layout when only one region exists. However, I think having this buried here will cause issues when theme designers do things like:

              'frontpage' => array(
                  'file' => 'columns2.php',
                  'regions' => array('side-pre', 'side-post'),
                  'defaultregion' => 'side-pre',
                  'options' => array('nonavbar'=>true),
              ),
          

          which I think with my solution will only show 'side-pre' for both LTR and RTL but with your solution will show 'side-pre' for LTR and 'side-post' for RTL and therefore harder to debug.

          So, three possible alternatives:

          1. Leave this fix as is.
          2. Change 'blocks()' to be two functions 'three_column_blocks()' and 'two_column_blocks()' the former does swapping and the latter does not (reducing logic in column2.php).
          3. Push some of the attribute logic of 'blocks()' into 'blocks_for_region()' and call that instead for 'column2.php' but that messes up other themes + base.

          More thoughts to come...

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , Thank you for your input. With reference to the truth table in this issue - https://tracker.moodle.org/secure/attachment/33087/mdl-40065-tt.png - and your proposed changes - https://github.com/samhemelryk/moodle/commit/wip-MDL-40065-m26: 1. Having two blocks defined was not the issue because it was a two column layout and having two defined in 'config.php' would have been silly on the part of the theme designer. The issue is that 'blocks()' swaps regardless for RTL - please see the truth table - but using the negated XOR gate logic and eliminating the false-false row to become the 'AND' that you have leads to issues with mistakes in 'config.php' - see below. 2. Currently I believe that your patch will fail with 'side-post' only in LTR because of: echo $OUTPUT->blocks('side-pre', 'span3'.$classextra); where 'side-pre' is fixed and no 'side-post' is swapped by: if ($ this ->blocks->is_known_region($regionwas) && $ this ->blocks->is_known_region($regionnow)) { when you have something like: 'admin' => array( 'file' => 'columns2.php', 'regions' => array('side-post'), 'defaultregion' => 'side-post', ), 3. But, you are posing an interesting thought that 'blocks()' should only be used for RTL swapping in a three column layout - see MDL-40204 and in fact to reduce the logic of this (using the truth table as a guide and the fact that still need to have the '$useblock' logic because of the need to support 'side-post' as mentioned in '2') then given the code of 'blocks()' then this needs to use 'blocks_for_region()' instead, but I'm loathed to do that as the code does other stuff too with the attributes which falls in line with MDL-39824 : public function blocks($region, $classes = array(), $tag = 'aside') { $displayregion = $ this ->page->apply_theme_region_manipulations($region); $classes = (array)$classes; $classes[] = 'block-region'; $attributes = array( 'id' => 'block-region-'.preg_replace('#[^a-zA-Z0-9_\-]+#', '-', $displayregion), 'class' => join(' ', $classes), 'data-blockregion' => $displayregion, 'data-droptarget' => '1' ); return html_writer::tag($tag, $ this ->blocks_for_region($region), $attributes); } So coming back full circle the method 'apply_theme_region_manipulations()' does this: public function apply_theme_region_manipulations($region) { if ($ this ->blockmanipulations && isset($ this ->blockmanipulations[$region])) { $regionwas = $region; $regionnow = $ this ->blockmanipulations[$region]; if ($ this ->blocks->is_known_region($regionwas) && $ this ->blocks->is_known_region($regionnow)) { // Both the before and after regions are known so we can swap them over. return $regionnow; } // We didn't know about both, we won't swap them over. return $regionwas; } return $region; } so that no swapping is performed on a two column layout when only one region exists. However, I think having this buried here will cause issues when theme designers do things like: 'frontpage' => array( 'file' => 'columns2.php', 'regions' => array('side-pre', 'side-post'), 'defaultregion' => 'side-pre', 'options' => array('nonavbar'=> true ), ), which I think with my solution will only show 'side-pre' for both LTR and RTL but with your solution will show 'side-pre' for LTR and 'side-post' for RTL and therefore harder to debug. So, three possible alternatives: 1. Leave this fix as is. 2. Change 'blocks()' to be two functions 'three_column_blocks()' and 'two_column_blocks()' the former does swapping and the latter does not (reducing logic in column2.php). 3. Push some of the attribute logic of 'blocks()' into 'blocks_for_region()' and call that instead for 'column2.php' but that messes up other themes + base. More thoughts to come... Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          Ok, I've been thinking and the thought has struck me that the code needs to be as simple as possible for the 'correct' values of the inputs. So, therefore taking the:

              'frontpage' => array(
                  'file' => 'columns2.php',
                  'regions' => array('side-pre', 'side-post'),
                  'defaultregion' => 'side-pre',
                  'options' => array('nonavbar'=>true),
              ),
          

          situation aside and on the basis of reducing the logic of 'columns2.php' to that rationally and visibly required to place the right block in the correct area on the page - nice and visible to the theme designer - I propose using:

              public function apply_theme_region_manipulations($region) {
                  if ($this->blockmanipulations && isset($this->blockmanipulations[$region])) {
                      $regionwas = $region;
                      $regionnow = $this->blockmanipulations[$region];
                      if ($this->blocks->is_known_region($regionwas) && $this->blocks->is_known_region($regionnow)) {
                          // Both the before and after regions are known so we can swap them over.
                          return $regionnow;
                      }
                      // We didn't know about both, we won't swap them over.
                      return $regionwas;
                  }
                  return $region;
              }
          

          But still retaining '$useblock' to cope with the 'side-post' only issue. I'll come up with branches that have this. Also, this should not affect the operation of MDL-40204.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , Ok, I've been thinking and the thought has struck me that the code needs to be as simple as possible for the 'correct' values of the inputs. So, therefore taking the: 'frontpage' => array( 'file' => 'columns2.php', 'regions' => array('side-pre', 'side-post'), 'defaultregion' => 'side-pre', 'options' => array('nonavbar'=> true ), ), situation aside and on the basis of reducing the logic of 'columns2.php' to that rationally and visibly required to place the right block in the correct area on the page - nice and visible to the theme designer - I propose using: public function apply_theme_region_manipulations($region) { if ($ this ->blockmanipulations && isset($ this ->blockmanipulations[$region])) { $regionwas = $region; $regionnow = $ this ->blockmanipulations[$region]; if ($ this ->blocks->is_known_region($regionwas) && $ this ->blocks->is_known_region($regionnow)) { // Both the before and after regions are known so we can swap them over. return $regionnow; } // We didn't know about both, we won't swap them over. return $regionwas; } return $region; } But still retaining '$useblock' to cope with the 'side-post' only issue. I'll come up with branches that have this. Also, this should not affect the operation of MDL-40204 . Cheers, Gareth
          Hide
          Nadav Kavalerchik added a comment -

          Gareth J Barnard, I have moved my previous comment about DND to issue MDL-40204 as you suggested. Thanks.

          And I would like to raise another issue...

          How about adding to the theme configuration a new setting that will hold a "swap table" (array) of the block rigions that need to be swapped in RTL/LTR mode switch?

          $THEME->rtl_ltr_swap_table =  array('side-pre' => 'side-post', 
          'side-post'=>'side-pre', 
          'some-ltr-region1' => 'some-rtl-region2', ... );
          

          The theme designer will decide what needs to be swapped with what.
          I have already made a theme with more block regions then "side-pre" and "side-post" that might be needed to swap too.
          So I think we should consider a more generic solution. What do you think?

          Show
          Nadav Kavalerchik added a comment - Gareth J Barnard , I have moved my previous comment about DND to issue MDL-40204 as you suggested. Thanks. — And I would like to raise another issue... How about adding to the theme configuration a new setting that will hold a "swap table" (array) of the block rigions that need to be swapped in RTL/LTR mode switch? $THEME->rtl_ltr_swap_table = array('side-pre' => 'side-post', 'side-post'=>'side-pre', 'some-ltr-region1' => 'some-rtl-region2', ... ); The theme designer will decide what needs to be swapped with what. I have already made a theme with more block regions then "side-pre" and "side-post" that might be needed to swap too. So I think we should consider a more generic solution. What do you think?
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          I've made some changes that you have suggested and submitted new branches for review. I am aware that you are not directly credited with the 'pagelib.php' code but do not know how to do this in branch - are you able to have a separate commit pulled for this?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , I've made some changes that you have suggested and submitted new branches for review. I am aware that you are not directly credited with the 'pagelib.php' code but do not know how to do this in branch - are you able to have a separate commit pulled for this? Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Nadav Kavalerchik,

          Thank you, to be honest my head is about to explode with the logic of this. I'll test the issue you raised on MDL-40204 and then come back to you .

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Nadav Kavalerchik , Thank you, to be honest my head is about to explode with the logic of this. I'll test the issue you raised on MDL-40204 and then come back to you . Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Nadav Kavalerchik,

          Ok, the solution you are proposing of:

          $THEME->rtl_ltr_swap_table =  array('side-pre' => 'side-post', 
          'side-post'=>'side-pre', 
          'some-ltr-region1' => 'some-rtl-region2', ... );
          

          now conflicts with the solution that Sam would now like and would need to do more than a one class swap as the swapping code in 'column2.php' does more than one class alteration. Also having classes in the 'config.php' file rather than the html layout / css files pulls the CSS concept from two areas to three areas potentially causing upgrade issues. So, I think we should leave CSS out of 'config.php' and in effect 'pagelib.php' and keep only Moodle region logic as with 'column2.php' you are looking at the Bootstrap classes of 'pull-right' etc. in the place of 'some-ltr-region1' in your example.

          Will think some more about this.

          Cheers,

          Gareth

          P.S.

          I assume you are taking about adding to:

          $THEME->blockrtlmanipulations = array(
              'side-pre' => 'side-post',
              'side-post' => 'side-pre'
          );
          
          Show
          Gareth J Barnard added a comment - - edited Dear Nadav Kavalerchik , Ok, the solution you are proposing of: $THEME->rtl_ltr_swap_table = array('side-pre' => 'side-post', 'side-post'=>'side-pre', 'some-ltr-region1' => 'some-rtl-region2', ... ); now conflicts with the solution that Sam would now like and would need to do more than a one class swap as the swapping code in 'column2.php' does more than one class alteration. Also having classes in the 'config.php' file rather than the html layout / css files pulls the CSS concept from two areas to three areas potentially causing upgrade issues. So, I think we should leave CSS out of 'config.php' and in effect 'pagelib.php' and keep only Moodle region logic as with 'column2.php' you are looking at the Bootstrap classes of 'pull-right' etc. in the place of 'some-ltr-region1' in your example. Will think some more about this. Cheers, Gareth P.S. I assume you are taking about adding to: $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' );
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Nadav Kavalerchik,

          In thinking more about your solution (which is interesting ) I think it goes beyond the scope of both this issue and MDL-40204. But combines with the thoughts Mary and I have commented (even though they differ) on MDL-40204 in terms of how layouts should work in general. So, I propose taking the debate to the theme's forum / Damyon Wiese's CSS draft framework (http://docs.moodle.org/dev/User:Damyon_Wiese/Draft_CSS_Framework).

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Nadav Kavalerchik , In thinking more about your solution (which is interesting ) I think it goes beyond the scope of both this issue and MDL-40204 . But combines with the thoughts Mary and I have commented (even though they differ) on MDL-40204 in terms of how layouts should work in general. So, I propose taking the debate to the theme's forum / Damyon Wiese 's CSS draft framework ( http://docs.moodle.org/dev/User:Damyon_Wiese/Draft_CSS_Framework ). Cheers, Gareth
          Hide
          Nadav Kavalerchik added a comment -

          Dear Gareth J Barnard,
          You suggestion (two comments above) is exactly what I meant.

          I do think it is something beyond the scope of bootstrap and that it should be considered as part of the theme's framework. This should be solve on the PHP level and not manipulated on the CSS level.
          But since we (you) are "touching" this issue here, I though I mention it.

          Indeed, following the comments on this issue, It seems like what you are doing can melt a nuclear reactor core
          Please do not kill yourself over this, we need you alive

          Show
          Nadav Kavalerchik added a comment - Dear Gareth J Barnard , You suggestion (two comments above) is exactly what I meant. I do think it is something beyond the scope of bootstrap and that it should be considered as part of the theme's framework. This should be solve on the PHP level and not manipulated on the CSS level. But since we (you) are "touching" this issue here, I though I mention it. Indeed, following the comments on this issue, It seems like what you are doing can melt a nuclear reactor core Please do not kill yourself over this, we need you alive
          Hide
          Gareth J Barnard added a comment -

          Dear Nadav Kavalerchik,

          Thank you, currently melting.... Pragmatically this and MDL-40204 solve justify-able issues. Things that are 'Moodle' and Moodle CSS should be in PHP but CSS that is in Bootstrap etc. should stay in CSS and layout files only. I think bringing them into 'config.php' is a bad idea. It's all about nuclear containment .

          I think what you are proposing would be a really good debate and design improvement on the theme's forum

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Nadav Kavalerchik , Thank you, currently melting.... Pragmatically this and MDL-40204 solve justify-able issues. Things that are 'Moodle' and Moodle CSS should be in PHP but CSS that is in Bootstrap etc. should stay in CSS and layout files only. I think bringing them into 'config.php' is a bad idea. It's all about nuclear containment . I think what you are proposing would be a really good debate and design improvement on the theme's forum Cheers, Gareth
          Hide
          Mary Evans added a comment -

          @Nadav, Not sure if you are aware but since Moodle 2.5 we now use HTML5 page layouts so would be good to suggest that the <bdi></bdi> tag could be added to help with RTL.
          More about <bdi> tag HERE

          Show
          Mary Evans added a comment - @Nadav, Not sure if you are aware but since Moodle 2.5 we now use HTML5 page layouts so would be good to suggest that the <bdi></bdi> tag could be added to help with RTL. More about <bdi> tag HERE
          Hide
          Nadav Kavalerchik added a comment -

          Mary Evans, thanks. I was not aware of it. I will read into it

          Show
          Nadav Kavalerchik added a comment - Mary Evans , thanks. I was not aware of it. I will read into it
          Hide
          Nadav Kavalerchik added a comment -

          Ok. I read it through, And I think it will help solve the breadcrumbs visual mixed Hebrew and English navigation issues.
          I will open a separate Issue about it. Thanks

          Show
          Nadav Kavalerchik added a comment - Ok. I read it through, And I think it will help solve the breadcrumbs visual mixed Hebrew and English navigation issues. I will open a separate Issue about it. Thanks
          Hide
          Mary Evans added a comment - - edited

          I have just discovered that Afterburner theme has the same problem as this bug, in that if you are in moodle/my page, which is a side-post only layout in Afterburner, when you want to 'customise this page' and go to move a block from side-post to the main-content area, the whole of side-post moves to the centre and an area on the left opens up, so in effect you have three columns when in actual fact it should only be two. Although it is a little bit worse than that as the blocks are sat on top of the centre column.

          I have replicated this in http://qa.moodle.net so going to file a bug report.

          Show
          Mary Evans added a comment - - edited I have just discovered that Afterburner theme has the same problem as this bug, in that if you are in moodle/my page, which is a side-post only layout in Afterburner, when you want to 'customise this page' and go to move a block from side-post to the main-content area, the whole of side-post moves to the centre and an area on the left opens up, so in effect you have three columns when in actual fact it should only be two. Although it is a little bit worse than that as the blocks are sat on top of the centre column. I have replicated this in http://qa.moodle.net so going to file a bug report.
          Hide
          Sam Hemelryk added a comment -

          Hi Gareth,

          This patch is still a bit off sorry.

          The 2 column layout doesn't know about side-post and the noblocks option is no longer used (instead we'd use the 1 column layout file).
          If we do trigger the code that you've added to the 2 column layout https://github.com/gjb2048/moodle/compare/master...wip-MDL-40065-master_3#L6R34 you'll get an error about an unknown block region (because it doesn't know about side-post).

          Looking at the logic in that file I'm pretty sure the only thing that is needed there is the pull-right if using an rtl language.
          That layout file does not know about or need to know about side-post.

          I'll leave this in integration review and we can discuss it if you like.

          Sorry I didn't get time to read your last set of comments, I'll try harder to get back to this issue this time.
          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Gareth, This patch is still a bit off sorry. The 2 column layout doesn't know about side-post and the noblocks option is no longer used (instead we'd use the 1 column layout file). If we do trigger the code that you've added to the 2 column layout https://github.com/gjb2048/moodle/compare/master...wip-MDL-40065-master_3#L6R34 you'll get an error about an unknown block region (because it doesn't know about side-post). Looking at the logic in that file I'm pretty sure the only thing that is needed there is the pull-right if using an rtl language. That layout file does not know about or need to know about side-post. I'll leave this in integration review and we can discuss it if you like. Sorry I didn't get time to read your last set of comments, I'll try harder to get back to this issue this time. Many thanks Sam
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          The logic around 'side-post' is based upon the outcome of:

          $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT));
          

          So, if the designer has specified 'side-pre' in the 'config.php' file as the block region to use will be 'side-pre' and don't need to worry that 'side-post' does not exist. However if '$hassidepre' is 'false' then 'config.php' must have 'side-post' otherwise logically with a two column format it's a coding fault and therefore the error is valid. I hope this makes sense .

          Plus if both 'side-pre' and 'side-post' in the 'config.php' file then only 'side-pre' will be shown, but then it is a two column format!

          There is already 'pull-right' selector injection for LTR 'side-pre' in the code:

          <section id="region-main" class="span9<?php if ($left) { echo ' pull-right'; } ?>">
          

          There is additionally 'desktop-first-column' lower down that operates off the same variable.

          In RTL 'side-pre' both 'pull-right' and 'desktop-first-column' are not required because the ordering of the elements in the page puts them in the right place.

          That is how it works for 'side-pre' with 'side-post' the logic is reversed and you get the correct layout for both LTR and RTL with the blocks being on the right for the former and the left for the latter.

          So, that just leaves the inclusion of the 'empty($PAGE->layout_options['noblocks'])' logic. I can either sort by removing within around twenty four hours or if you wish add an extra commit to remove it.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , The logic around 'side-post' is based upon the outcome of: $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT)); So, if the designer has specified 'side-pre' in the 'config.php' file as the block region to use will be 'side-pre' and don't need to worry that 'side-post' does not exist. However if '$hassidepre' is 'false' then 'config.php' must have 'side-post' otherwise logically with a two column format it's a coding fault and therefore the error is valid. I hope this makes sense . Plus if both 'side-pre' and 'side-post' in the 'config.php' file then only 'side-pre' will be shown, but then it is a two column format! There is already 'pull-right' selector injection for LTR 'side-pre' in the code: <section id= "region-main" class= "span9<?php if ($left) { echo ' pull-right'; } ?>" > There is additionally 'desktop-first-column' lower down that operates off the same variable. In RTL 'side-pre' both 'pull-right' and 'desktop-first-column' are not required because the ordering of the elements in the page puts them in the right place. That is how it works for 'side-pre' with 'side-post' the logic is reversed and you get the correct layout for both LTR and RTL with the blocks being on the right for the former and the left for the latter. So, that just leaves the inclusion of the 'empty($PAGE->layout_options ['noblocks'] )' logic. I can either sort by removing within around twenty four hours or if you wish add an extra commit to remove it. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          P.S.

          So in summary the 'columns2.php' provides the theme designer the following combinations:

          LTR Content (right) with side-pre (left).
          LTR Content (left) with side-post (right).
          RTL Content (left) with side-pre (right).
          RTL Content (right) with side-post (left).

          Perfect mirrored logic!

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited P.S. So in summary the 'columns2.php' provides the theme designer the following combinations: LTR Content (right) with side-pre (left). LTR Content (left) with side-post (right). RTL Content (left) with side-pre (right). RTL Content (right) with side-post (left). Perfect mirrored logic! Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          P.P.S.

          Please test it with combinations in 'config.php' and LTR and RTL languages. You'll love it, slap your thigh and say 'ding dang do, that's brilliant'

          Show
          Gareth J Barnard added a comment - P.P.S. Please test it with combinations in 'config.php' and LTR and RTL languages. You'll love it, slap your thigh and say 'ding dang do, that's brilliant'
          Hide
          Gareth J Barnard added a comment -

          P.P.P.S.

          Ok, I will admit that given what I have said that:

          $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT));
          

          actually needs to be:

          $hassidepre = $PAGE->blocks->is_known_region('side-pre');
          

          So if you want to add as a single commit, then please do so as that would balance the commit of code I owe you credit wise. Otherwise, I can do later on Monday BST.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - P.P.P.S. Ok, I will admit that given what I have said that: $hassidepre = (empty($PAGE->layout_options['noblocks']) && $PAGE->blocks->region_has_content('side-pre', $OUTPUT)); actually needs to be: $hassidepre = $PAGE->blocks->is_known_region('side-pre'); So if you want to add as a single commit, then please do so as that would balance the commit of code I owe you credit wise. Otherwise, I can do later on Monday BST. Cheers, Gareth
          Hide
          Sam Hemelryk added a comment -

          Hiya,

          I honestly do not think we should support two block regions in the two column layout.
          The two column layout should only have a single block region, because that block region + the content make two columns. If the user is using a ltr language the one block region is shown on the left, if they're using a rtl language its shown on the right.
          There's no need to introduce a second region (in this case side-post) under some circumstances.

          The error that the current code introduces is entirely not acceptable and not expected. Run through the following for instance:

          1. Log in as an admin using English as your language (or any ltr language)
          2. Change your theme to bootstrapbase
          3. Browse to course
          4. Turn on editing
          5. Browse to Settings > Course administration > Reports > Course participation.
          6. Edit every blocks setting on the report page and set "Visible" to "No"
          7. Browse to the course and turn off editing.
          8. Browse back to the course participation report.
          9. Boom "Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-post"

          You end up on the two column layout but with no visible blocks. Because side-pre contains no content the code has switched to side-post and now we're using a block region that config doesn't know about (expect) and as such you an exception being thrown.
          Don't think about side-pre belonging to the left and side-post belonging to the right. The names only hold as much relevance to the position of the region as the layout gives them and the layout should certainly not alter because of the name of the region. Displaying a region on the right instead of the left because of the language being used is fine, whereas using a different region because of the language the user is using is not a good solution.

          We need an outcome on this issue, but I'm sorry I don't feel that this solution works.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hiya, I honestly do not think we should support two block regions in the two column layout. The two column layout should only have a single block region, because that block region + the content make two columns. If the user is using a ltr language the one block region is shown on the left, if they're using a rtl language its shown on the right. There's no need to introduce a second region (in this case side-post) under some circumstances. The error that the current code introduces is entirely not acceptable and not expected. Run through the following for instance: Log in as an admin using English as your language (or any ltr language) Change your theme to bootstrapbase Browse to course Turn on editing Browse to Settings > Course administration > Reports > Course participation. Edit every blocks setting on the report page and set "Visible" to "No" Browse to the course and turn off editing. Browse back to the course participation report. Boom "Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-post" You end up on the two column layout but with no visible blocks. Because side-pre contains no content the code has switched to side-post and now we're using a block region that config doesn't know about (expect) and as such you an exception being thrown. Don't think about side-pre belonging to the left and side-post belonging to the right. The names only hold as much relevance to the position of the region as the layout gives them and the layout should certainly not alter because of the name of the region. Displaying a region on the right instead of the left because of the language being used is fine, whereas using a different region because of the language the user is using is not a good solution. We need an outcome on this issue, but I'm sorry I don't feel that this solution works. Cheers Sam
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          Ok, the situation:

          1. Log in as an admin using English as your language (or any ltr language)
          2. Change your theme to bootstrapbase -> Clean
          3. Browse to course
          4. Turn on editing
          5. Browse to Settings > Course administration > Reports > Course participation.
          6. Edit every blocks setting on the report page and set "Visible" to "No"
          7. Browse to the course and turn off editing.
          8. Browse back to the course participation report.
          9. Boom "Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-post".

          Can be fixed by using the replacement line:

          $hassidepre = $PAGE->blocks->is_known_region('side-pre');
          

          Which I will update on the patch later today.

          I really do not understand why you are saying "Don't think about side-pre belonging to the left and side-post belonging to the right." as in English the word 'pre' means 'before' and the word 'post' means 'after'. Your asking me to think that my left arm is not actually on the left but can be detached and put anywhere.

          Regarding "I honestly do not think we should support two block regions in the two column layout. The two column layout should only have a single block region, because that block region + the content make two columns" - I am only supporting a single block region as it's a two column theme, that being in the 'config.php' file as either 'side-pre' or 'side-post'. If both regions are defined, then only 'side-pre' is used.

          So are you saying that the two column layout should do one of the following:
          1. Only use 'side-pre'.
          2. Combine 'side-pre' and 'side-post' into one column?

          If so then this restricts theme designer choice and flexibility in using the layout.

          I think if we are coming to an impasse on this then we need second opinions from others such as Mary Evans, David Scotson, Bas Brands, certainly Michael de Raadt and possibly Martin Dougiamas.

          Kind regards,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , Ok, the situation: 1. Log in as an admin using English as your language (or any ltr language) 2. Change your theme to bootstrapbase -> Clean 3. Browse to course 4. Turn on editing 5. Browse to Settings > Course administration > Reports > Course participation. 6. Edit every blocks setting on the report page and set "Visible" to "No" 7. Browse to the course and turn off editing. 8. Browse back to the course participation report. 9. Boom "Coding error detected, it must be fixed by a programmer: Trying to reference an unknown block region side-post". Can be fixed by using the replacement line: $hassidepre = $PAGE->blocks->is_known_region('side-pre'); Which I will update on the patch later today. I really do not understand why you are saying "Don't think about side-pre belonging to the left and side-post belonging to the right." as in English the word 'pre' means 'before' and the word 'post' means 'after'. Your asking me to think that my left arm is not actually on the left but can be detached and put anywhere. Regarding "I honestly do not think we should support two block regions in the two column layout. The two column layout should only have a single block region, because that block region + the content make two columns" - I am only supporting a single block region as it's a two column theme, that being in the 'config.php' file as either 'side-pre' or 'side-post'. If both regions are defined, then only 'side-pre' is used. So are you saying that the two column layout should do one of the following: 1. Only use 'side-pre'. 2. Combine 'side-pre' and 'side-post' into one column? If so then this restricts theme designer choice and flexibility in using the layout. I think if we are coming to an impasse on this then we need second opinions from others such as Mary Evans , David Scotson , Bas Brands , certainly Michael de Raadt and possibly Martin Dougiamas . Kind regards, Gareth
          Hide
          Mary Evans added a comment - - edited

          Dear Gareth,

          I just want to point out that the reason a 2 column page is being used for a specific layout as in Admin or Report where we see the following in bootstrapbase/config.php:

              // Server administration scripts.
              'admin' => array(
                  'file' => 'columns2.php',
                  'regions' => array('side-pre'),
                  'defaultregion' => 'side-pre',
              ),
          

          In this case the 2 column page will NEVER have side-post. Same goes for the report layout.

          As we have discussed before the LTR -> RTL shift is only a matter of the page being a 'mirror image' achieved with the pull-right in the main content.

          I propose we look at redoing the layouts and making them simple.

          So in simple terms we only need this for side-pre-only layout...

          
          <div class="container-fluid">
          
              <div class="row-fluid">
                  <div class="span3">
                      <div class="well sidebar-nav">
                          <?php echo $OUTPUT->blocks('side-pre'); ?>
                      </div><!--/.well -->
                  </div><!--/span3-->
          
                  <div class="span9">
                  <?php echo $OUTPUT->main_content(); ?>
                  </div><!--/span9-->
              </div>
              
              <footer>
                  <p>&copy; Company 2012</p>
              </footer>
          
          </div><!--/.fluid-container--> 
             
          

          Perhaps we should think about creating:

          sidepreonly.php
          sidepostonly.php
          contentonly.php
          general.php
          frontpage.php

          Show
          Mary Evans added a comment - - edited Dear Gareth, I just want to point out that the reason a 2 column page is being used for a specific layout as in Admin or Report where we see the following in bootstrapbase/config.php: // Server administration scripts. 'admin' => array( 'file' => 'columns2.php', 'regions' => array('side-pre'), 'defaultregion' => 'side-pre', ), In this case the 2 column page will NEVER have side-post. Same goes for the report layout. As we have discussed before the LTR -> RTL shift is only a matter of the page being a 'mirror image' achieved with the pull-right in the main content. I propose we look at redoing the layouts and making them simple. So in simple terms we only need this for side-pre-only layout... <div class= "container-fluid" > <div class= "row-fluid" > <div class= "span3" > <div class= "well sidebar-nav" > <?php echo $OUTPUT->blocks('side-pre'); ?> </div><!--/.well --> </div><!--/span3--> <div class= "span9" > <?php echo $OUTPUT->main_content(); ?> </div><!--/span9--> </div> <footer> <p>&copy; Company 2012</p> </footer> </div><!--/.fluid-container--> Perhaps we should think about creating: sidepreonly.php sidepostonly.php contentonly.php general.php frontpage.php
          Hide
          Mary Evans added a comment -

          Sorry I had to leave off finishing what I was saying in my last comment as real life took over, I'm doing my Florence Nightingale act looking after my poorly hubby.

          So getting back to the layouts, side-pre and side-post are only regions that is true. However the fact we load blocks for side-pre and blocks for side-post suggests there is a link as you are so rightly pointing out Gareth.

          I have lost touch with how the blocks are configured now, but I suspect they work the same as they have always done and those blocks which take their lead from the default region specified in the theme's config.php will always end up in that default block region regardless of whether it was in position LEFT or RIGHT previously.

          If a theme designer wants a theme to be a 2 coll side-post-only like Brick theme then this is specified in config.php and so would use the columns2.php for all the layouts. It would then be the task for the Administrator to decide which blocks go where in the different pages.

          Show
          Mary Evans added a comment - Sorry I had to leave off finishing what I was saying in my last comment as real life took over, I'm doing my Florence Nightingale act looking after my poorly hubby. So getting back to the layouts, side-pre and side-post are only regions that is true. However the fact we load blocks for side-pre and blocks for side-post suggests there is a link as you are so rightly pointing out Gareth. I have lost touch with how the blocks are configured now, but I suspect they work the same as they have always done and those blocks which take their lead from the default region specified in the theme's config.php will always end up in that default block region regardless of whether it was in position LEFT or RIGHT previously. If a theme designer wants a theme to be a 2 coll side-post-only like Brick theme then this is specified in config.php and so would use the columns2.php for all the layouts. It would then be the task for the Administrator to decide which blocks go where in the different pages.
          Hide
          Mary Evans added a comment -

          JUST FOR FUN Here is something to try out.

          In the columns2.php change '$hassidepre' to '$hassidegjb' and wherever pre is listed change that from 'pre' to 'gjb'. So in effect you have created a new block region.

          Then in your theme's config specify side-gjb as the only region and default region for admin layout.

          Purge all cache and test to see what happens and which blocks get loaded. Technically Admin and Nav should be the only two.

          Show
          Mary Evans added a comment - JUST FOR FUN Here is something to try out. In the columns2.php change '$hassidepre' to '$hassidegjb' and wherever pre is listed change that from 'pre' to 'gjb'. So in effect you have created a new block region. Then in your theme's config specify side-gjb as the only region and default region for admin layout. Purge all cache and test to see what happens and which blocks get loaded. Technically Admin and Nav should be the only two.
          Hide
          Gareth J Barnard added a comment -

          Get well soon Mr Evans

          Show
          Gareth J Barnard added a comment - Get well soon Mr Evans
          Hide
          Gareth J Barnard added a comment -

          I've rebased and fixed both branches - I intend to do Mary's suggestion later as a new branch - sorry but have a big issue to deal with and have to halt with this for a while.

          Show
          Gareth J Barnard added a comment - I've rebased and fixed both branches - I intend to do Mary's suggestion later as a new branch - sorry but have a big issue to deal with and have to halt with this for a while.
          Hide
          Gareth J Barnard added a comment -

          Dear all, Mary Evans's suggestion implemented in version 4 of the patch branches and tracker details updated.

          Show
          Gareth J Barnard added a comment - Dear all, Mary Evans 's suggestion implemented in version 4 of the patch branches and tracker details updated.
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          This sounds like it is really progressing now.

          Re: side-pre/side-post left-arm/right-arm it is true that the names can be taken to mean left and right. But that is not what they mean. Pre = before the content, and Post = after the content. In a rtl lang like English pre would be on the left and in a ltr lang like Hebrew pre would be on the right. In fact the names have nothing to do with left and right, they describe position in relation to the content.
          But that is still aside the point.
          Mary picked it up in one of her comments, its about having just a single block region. The names are unfortunate. They always have been, the block regions in the original themes were named to reflect their positions only to try to make the situation more understandable for those learning to theme. In reality it has caused a lot of confusion because the name is just a name, it doesn't have to reflect anything.
          Side-pre and side-post could have easily been named "primary" and "secondary", if that had been the case only the "primary" block region would have been used in this layout.

          A couple of things about how the block system works:

          • When you first install a site, or first create a course the default blocks are added to 'side-pre' and 'side-post'. This has nothing to do with block regions made available by the theme.
          • When blocks are added to the page if the page contains blocks belonging to a region that doesn't exist (the theme layout doesn't specify it) then the blocks are instead displayed in the default region after the blocks belonging to that region. If a theme defines just side-pre and the page has side-pre and side-post block then the side-post blocks will be displayed below the side-pre blocks.

          And now I'll take a look at your changes Gareth.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, This sounds like it is really progressing now. Re: side-pre/side-post left-arm/right-arm it is true that the names can be taken to mean left and right. But that is not what they mean. Pre = before the content, and Post = after the content. In a rtl lang like English pre would be on the left and in a ltr lang like Hebrew pre would be on the right. In fact the names have nothing to do with left and right, they describe position in relation to the content. But that is still aside the point. Mary picked it up in one of her comments, its about having just a single block region. The names are unfortunate. They always have been, the block regions in the original themes were named to reflect their positions only to try to make the situation more understandable for those learning to theme. In reality it has caused a lot of confusion because the name is just a name, it doesn't have to reflect anything. Side-pre and side-post could have easily been named "primary" and "secondary", if that had been the case only the "primary" block region would have been used in this layout. A couple of things about how the block system works: When you first install a site, or first create a course the default blocks are added to 'side-pre' and 'side-post'. This has nothing to do with block regions made available by the theme. When blocks are added to the page if the page contains blocks belonging to a region that doesn't exist (the theme layout doesn't specify it) then the blocks are instead displayed in the default region after the blocks belonging to that region. If a theme defines just side-pre and the page has side-pre and side-post block then the side-post blocks will be displayed below the side-pre blocks. And now I'll take a look at your changes Gareth. Many thanks Sam
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Gareth, changes looked flawless and I've integrated them now.

          Thanks for persisting and seeing this issue through to the end.

          Show
          Sam Hemelryk added a comment - Awesome thanks Gareth, changes looked flawless and I've integrated them now. Thanks for persisting and seeing this issue through to the end.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Sam Hemelryk,

          No problem. It is true that 'pre' means before and 'post' means after hence the original solution supporting both of them in the right context. Where I think things fall down and progression reduced in the provision of solutions is understanding of the overall design and documented intent of decisions that have been made in the development process. Do you have a link to such a document or is it simply a matter of me reading through the developer documentation? Have I missed something?

          Therefore to prevent further prevarication, how should MDL-40204 be implemented? As there could be a means through pure CSS if '.rtl #region-bs-main-and-pre' was defined as actually implementing '#region-bs-main-and-post'.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Sam Hemelryk , No problem. It is true that 'pre' means before and 'post' means after hence the original solution supporting both of them in the right context. Where I think things fall down and progression reduced in the provision of solutions is understanding of the overall design and documented intent of decisions that have been made in the development process. Do you have a link to such a document or is it simply a matter of me reading through the developer documentation? Have I missed something? Therefore to prevent further prevarication, how should MDL-40204 be implemented? As there could be a means through pure CSS if '.rtl #region-bs-main-and-pre' was defined as actually implementing '#region-bs-main-and-post'. Cheers, Gareth
          Hide
          Sam Hemelryk added a comment -

          Have commented on MDL-40204 now decision is all yours.

          Show
          Sam Hemelryk added a comment - Have commented on MDL-40204 now decision is all yours.
          Hide
          Sam Hemelryk added a comment -

          In regards to the documentation I'm afraid there is no really good documentation on the overall design and resulting intents and decisions.
          The theme documentation in the dev docs probably contains some but I don't imagine it contains all. Like may other areas in Moodle better documentation is required, or at least a place where things have been summarised.

          Show
          Sam Hemelryk added a comment - In regards to the documentation I'm afraid there is no really good documentation on the overall design and resulting intents and decisions. The theme documentation in the dev docs probably contains some but I don't imagine it contains all. Like may other areas in Moodle better documentation is required, or at least a place where things have been summarised.
          Hide
          Gareth J Barnard added a comment -

          Dear Sam Hemelryk,

          Thanks - I totally understand - I'm way behind with my Collapsed Topics page on Moodle docs. Perhaps its a question to ask about at a Frontend team meeting on how it can be done without placing a huge burden on development / time? For example, even a glossary of definitions like we have been discussing about the meaning of 'side-pre' and 'side-post' being completely positional and on dual column pages should be combined. Overall architecture diagrams are good too even if they do not give the detail (as that will change over time and get out of date on the diagram) as they provide a framework upon which to navigate around the code.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Sam Hemelryk , Thanks - I totally understand - I'm way behind with my Collapsed Topics page on Moodle docs. Perhaps its a question to ask about at a Frontend team meeting on how it can be done without placing a huge burden on development / time? For example, even a glossary of definitions like we have been discussing about the meaning of 'side-pre' and 'side-post' being completely positional and on dual column pages should be combined. Overall architecture diagrams are good too even if they do not give the detail (as that will change over time and get out of date on the diagram) as they provide a framework upon which to navigate around the code. Cheers, Gareth
          Hide
          Sam Hemelryk added a comment -

          Tested and passed thanks Gareth.

          Show
          Sam Hemelryk added a comment - Tested and passed thanks Gareth.
          Hide
          Mary Evans added a comment -

          Thanks Sam! Good news to end the day!

          Show
          Mary Evans added a comment - Thanks Sam! Good news to end the day!
          Hide
          Mary Evans added a comment -

          Good work Gareth...thanks!

          Show
          Mary Evans added a comment - Good work Gareth...thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for giving me joys and smiles
          Thanks for sharing my trouble's pile

          Thanks for wipeing the tears of my eye
          Thanks for showing me the glad view of sky

          Thanks for lending me your shoulders to lean
          Thanks for giving my words a proper mean

          Thanks for telling me the value of life
          Thanks for showing me the rules to survive

          Thanks for lending me the sympathetic ears
          Thanks for showing how much you care

          From all this what I mean in the end
          Is thanks for being my special friend.

          – Seema Chowdhury

          Sent upstream so... closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: