| 2015-05-04 05:52:18 |
Aaron Wells |
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 four 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. |
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. |
|
