mahara-contributors team mailing list archive

[Aaron Wells <1451329@xxxxxxxxxxxxxxxxxx>]

** Tags added: behat-needed

-- 
You received this bug notification because you are a member of Mahara
Contributors, which is subscribed to Mahara.
Matching subscriptions: Subscription for all Mahara Contributors -- please ask on #mahara-dev or mahara.org forum before editing or unsubscribing it!
https://bugs.launchpad.net/bugs/1451329

Title:
  Refactor block title methods to allow empty titles and make everything
  less confusing

Status in Mahara ePortfolio:
  Confirmed

Bug description:
  I initially thought Bug 1451324 would be a bite-sized bug, but after
  looking into it I realized that our system for determining default &
  instance block titles is a mess, and it doesn't allow for a blank
  default block title.

  There are currently five methods involved in determining block titles.
  I'll use "PluginBlocktype{Name}" to refer to a subclass of
  PluginBlocktype (like PluginBlocktypeImage)

  1. PluginBlocktype->get_title(): This method is called to find out the
  block title to display when the block is in "view" mode, and it
  determines the title of the config popup when the block is in editing
  mode. It's sorta meant to be a "final" method, because it contains
  logic that checks other parts of the public block API. First, it
  checks PluginBlocktype{Name}::override_instance_title(). If that's
  false, it checks $this->get('title') (which is block_instance.title
  from the DB), and if that's false then it checks
  PluginBlocktype{Name}::get_instance_title().

  2. PluginBlocktype{Name}::get_title(): This method confusingly has the
  same name as number 1, but is static instead of instance. It's also
  mandatory for every blocktype to implement this. The value returned
  does double-duty. It is the name that is displayed for this blocktype
  in the block picker, and it is also the default name of new block
  instances (unless the blocktype implements override_instance_title()
  or get_instance_title()

  3. PluginBlocktype{Name}::override_instance_title($instance): This
  method is meant to be used for blocks where the title is hard-coded,
  like "My Friends" and "My Groups". If it is present, and it returns
  non-empty, the return value of this is stored in block_instance.title,
  and displayed as the title in view and edit mode, and the "block"
  field is hidden from the block config form.

  4. PluginBlocktype{Name}::get_instance_title($instance): This method
  is meant to be used for blocks where the title defaults to the title
  of the artefact chosen for display in the block. For instance, the
  Blog block displays the title of the selected blog. If this method is
  implemented, the title for new block instances defaults to empty, and
  the block config shows a message that says "If the block title is
  empty it'll default to XXX" (which must be stored in the blocktype's
  "defaulttitledescription" string). And then, when you display the
  block, we check to see whether block_instance.title is empty, and if
  it is, we call this method and use its return value as the title.

  5. View->addblocktype($values): This is the method that contains the
  logic that determines what a new block's default title will be. First
  it checks to see whether the blocktype implements
  get_instance_title(). If it does, it sets the title to an empty
  string. Otherwise, it sets the title to the return value of the
  blocktype's static get_title() method (function number 2 above)

  So you can see, the problem here is that:

  1. The method names are confusing and don't explain what they do and
  how they're different from each other (and there are not sufficient
  comments in the code to explain this either.)

  2. We force the default block title for new block instances to be the
  same as the string used in the block picker.

  3. The only way to make a block title empty by default, would be to
  implement get_instance_title(), and set the blocktype's
  "defaulttitledescription" to an empty string. Which would be pretty
  hacky.

  I'll have to give some thought to the best way to refactor this. It
  might also be worthwhile doing a quick review of 3rd-party blocks to
  see how badly they'd be impacted if we made some big changes to this.
  One possibility that springs to mind, is to move the logic for
  determining the default title, out of View->addblocktype() and into
  the PluginBlocktype class, where it could maybe be overridden by child
  classes.

To manage notifications about this bug go to:
https://bugs.launchpad.net/mahara/+bug/1451329/+subscriptions