[mythtv-commits] Ticket #3834: Patches MythWeb Weather module to use new MythWeather-revamp sources

[MythTV]

#3834: Patches MythWeb Weather module to use new MythWeather-revamp sources
-----------------------------------------------+----------------------------
 Reporter:  Joe Ripley <vitaminjoe at gmail.com>  |        Owner:  xris   
     Type:  enhancement                        |       Status:  new    
 Priority:  minor                              |    Milestone:  unknown
Component:  mythweb                            |      Version:  unknown
 Severity:  low                                |   Resolution:         
  Mlocked:  0                                  |  
-----------------------------------------------+----------------------------

Comment(by Joe Ripley <vitaminjoe at gmail.com>):

 Replying to [comment:3 xris]:
 > There are a handful of small broken things in the patch (mostly just
 rewriting the svn auto-generated headers at the top of the files).

 Indeed.  I apologize for that, I wrote most of this patch before I
 understood the nuances of SVN and the protocols followed for coding stuff
 in Myth.  I can clean that up. :)

 > Anyway, aside from little stuff like the fact that you have "code" in
 the tmpl directory (tmpl/default/set_screen.php) -- template files
 shouldn't call other template files other than tiny snippets, use the main
 module controller for that -- the settings screen doesn't actually seem to
 work -- no inactive screens are listed to be added to the group.

 Yeah, I totally agree with you.  It was just a quick and lazy way to get
 things to work. :)  I'll  definitely move the code out to the main module
 controller.  As for the settings screen not working, that's weird (no
 inactive screens listed?).  The inactive screens list is built by checking
 the database for configured sources and then only listing the screen types
 that you have sources for.

 If you have nothing in your weathersourcesettings table, that's probably
 why you have no inactive screens.  I'll modify the code to check for this
 and spit out an appropriate error, or something.

 > Other nitpicky things:
 >
 >  * Use <label> tags (active screens)
 >  * use ' instead of " when there isn't any interpretation going on (I
 actually use them
 >    differently to tell if I should expect interpreted characters).
 >  * don't leave ?> at the end of files -- it leads to problematic
 whitespace at the end of
 >    the file that makes tracking down "output started at..." type errors.
 >  * Don't use \t in strings when the rest of the code is using spaces, it
 leads to ugly html
 >    source.

 All good ideas.  I will modify the patch to conform to these standards.

 > Also, what files are no longer needed as part of this?

 From just a cursory glance, these files are not required:
 mythplugins/mythweb/modules/weather/accid.dat
 mythplugins/mythweb/modules/weather/weathertypes.dat
 mythplugins/mythweb/modules/weather/set_prefs.php
 mythplugins/mythweb/modules/weather/Forecast.php
 mythplugins/mythweb/modules/weather/WeatherSite.php
 mythplugins/mythweb/modules/weather/set_prefs.php


 Thanks for all the feedback Chris.  I'll get working on the updates to the
 patch and hopefully post a revised version shortly.

 --
 Joe Ripley
 vitaminjoe at gmail.com

-- 
Ticket URL: <http://svn.mythtv.org/trac/ticket/3834#comment:4>
MythTV <http://svn.mythtv.org/trac>
MythTV