Comments

• 

  David Mudrák

  Sat, 17 May 2014, 5:16 AM

  Many thanks for sharing this plugin with the Moodle community. I've been
  particularly looking forward to review this plugin as I'm personally
  interested in mindmapping software and can see big potential for it within
  Moodle. I really like the idea of the course structure visualisation via
  mindmaps and would like to see this block approved soon. It won't happen today
  though, please see my comments below.
  
  Do I understand it that the mindmap files are being generated into the temp
  folder within the block? Note that many sites have dirroot protected from
  being writable by the web server process for good reasons. The common way in
  Moodle is to store all files in the dataroot or eventually in the file storage
  (pool) accessible via File API. If writable temp folder is a must for your
  plugin functionality, it must be explicitly mentioned in the readme file and
  installation instructions.
  
  Some files use opening header <?PHP, some <?php and db/access.php has even
  just the short tag <?. The short format <? is not supported by all servers
  (and for example, it did not install capabilities for the block at my notebook
  for that reason when I first tried it, as I have short_open_tag disabled in my
  php.ini). Let me suggest to fix and unify all opening tags to <?php and get
  rid of all closing tags to prevent from the risk of extra bytes after it (such
  as in block_mindmap.php) that may cause troubles. While doing that,
  you might consider switching to the standard boilerplate we use in Moodle.
  Please refer to http://docs.moodle.org/dev/Coding_style#Files
  
  The code wwwroot./blocks/mindmap/icons/mmg_logo.gif\"
  alt=\"some_text\">" produces invalid URL (added dot). Let me recommend to use
  standard pix locations and API to display images that would prevent typos like
  this. Also, alt text "some_text" does not really help accessibility for
  obvious reasons.
  
  Visiting view.php with the debugging mode set to DEBUG_DEVELOPER (which is
  more than recommended during development) throws many PHP notices and
  warnings, including "Missing argument 5 for block_mindmap_print_fa()"
  
  The lib.php formatting looks pretty broken with all those extra lines.
  It is encouraged to follow Moodle’s coding style as outlined in:
  http://docs.moodle.org/dev/Coding_style and  http://docs.moodle.org/dev/Coding
  The code checker plugin can be quite helpful in fine tuning your code and can
  be found at:  https://moodle.org/plugins/view.php?plugin=local_codechecker You
  may wish to consider using that tool to further improve your plugin. When I
  ran it, it produces significant amount of errors and warnings. Please do not
  underestimate following the Moodle coding style. The more consistent your code
  is with Moodle style, the more it is readable, maintenable and, let's admit,
  more enjoyable to review
  
  Note that by re-distributing visorFreemind.swf without any additional
  information might violate its GPL license that has some strict rules about
  re-distributing the files. I would suggest to keep the whole GNU GPL licensed
  flash viewer in its separate subfolder with an added readme file that would
  clearly state the origin of the file, its version, licence, authors etc.
  Your readme file should also mention the need for the flash requirement.
  
  I'm afraid you'll need to fix the strings file so that they do not contain
  trailing whitespaces. Moodle translation tools (like AMOS at lang.moodle.org)
  strip them automatically so translators would not be able to have them in
  their language packs. Also, plugins are supposed to ship just the English
  pack. All other translations (including your own Polish pack) are to be
  processed via AMOS. Also, some strings (like 'Download map file') are
  hard-coded in English still.
  
  Please review http://docs.moodle.org/dev/Plugin_validation#Recommended_URLs
  and then edit the plugin entry to provide the source, tracker, and
  documentation URLs that will enable others in the community to more actively
  participate in using and maintaining this plugin.
  
  I noticed that you did not specify an issue tracker for your plugin. Providing
  a place for users of your plugin to report issues encourages participation and
  provides a way for users to report bugs, make feature requests, or suggest
  other types of improvements. There are a couple of options. You are welcome to
  request that a component be created in the Moodle Tracker. This will allow for
  you to become more familiar with how issues are managed in Moodle core but may
  take a little more time to setup. Alternatively, for folks who are using
  Github.com, you can use the issues feature of Github to handle such requests.
  Kindly let me know which of these two options seems best for you.
  
  Please provide a documentation URL. You are welcome to create your
  documentaiton in Moodle Docs,  See
  http://docs.moodle.org/dev/Plugin_documentation for more information.
  
  For now, I am going to mark this plugin as needing more work until we get
  these issues resolved. Thanks for your patience with the review and approval
  process. I'm looking forward the revised version of your block!
  
• 

  David Mudrák

  Wed, 18 June 2014, 10:53 PM

  Thanks for the new version. I am going to approve the block now. Please note that I spotted that with $CFG->enableavailability disabled, the code throws PHP notices as the $conditionalhidden is not set. This is trivial to fix.
  
  I was thinking about the temp folder yet and I actually believe that the proper location for it it somewhere under $CFG->dataroot.'/block_mindmap/' and not in the filepool. That should be pretty simple to fix without big changes. I am sorry to hear you had troubles with the File API documentation. It did not change much since 2.0 so it is still valid. But I can understand that some tutorials/howtos would be good to have.
  
  You are cleared to land now. Welcome to Plugins directory!
• 

  José Ricardo Silva

  Wed, 23 July 2014, 7:57 PM

  Hi, thanks for sharing this amazing block, it was quite helpful.