Re: [Sugar-devel] [PATCH 4/4] Restore load homepage functionality

2011-11-30 Thread Rafael Ortiz 
2011/11/30 Anish Mangal an...@activitycentral.org

 On Wed 30 Nov 2011 10:11:43 AM IST, Manuel Quiñones wrote:
  Signed-off-by: Manuel Quiñones ma...@laptop.org
  ---
   browser.py |   14 ++
   1 files changed, 6 insertions(+), 8 deletions(-)
 
  diff --git a/browser.py b/browser.py
  index 320cb62..789988d 100644
  --- a/browser.py
  +++ b/browser.py
  @@ -39,7 +39,11 @@ import sessionstore
   from widgets import BrowserNotebook
 
   _ZOOM_AMOUNT = 0.1
  -_LIBRARY_PATH = '/usr/share/library-common/index.html'
  +if os.path.isfile('/usr/share/library-common/index.html'):
  +_HOMEPAGE_PATH = '/usr/share/library-common/index.html'
  +else:
  +_HOMEPAGE_PATH = os.path.join(activity.get_bundle_path(),
  +data/index.html)
 
 
   class SaveListener(object):
  @@ -272,13 +276,7 @@ class TabbedView(BrowserNotebook):
 
   def load_homepage(self):
   browser = self.current_browser
  -
  -if os.path.isfile(_LIBRARY_PATH):
  -browser.load_uri('file://' + _LIBRARY_PATH)
  -else:
  -default_page = os.path.join(activity.get_bundle_path(),
  -data/index.html)
  -browser.load_uri(default_page)
  +browser.load_uri('file://' + _HOMEPAGE_PATH)
 
   def _get_current_browser(self):
   return self.get_nth_page(self.get_current_page()).get_child()

 nitpicky
 IMO, moving the code from here to above is probably fine, but removing
 _LIBRARY_PATH would be adding that extra bit of complexity to the code.
 Having the variable makes it clear that:

 if LIBRARY_PATH:
   load it
 else
   load HOMEPAGE_PATH

 Also, maybe having an else-if instead of else for the HOMEPAGE_PATH
 check and having all this in a try..except would be safer
 /nitpicky


+1.


 Reviewed-by: Anish Mangal an...@activitycentral.com

 ___
 Sugar-devel mailing list
 Sugar-devel@lists.sugarlabs.org
 http://lists.sugarlabs.org/listinfo/sugar-devel

___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel

Re: [Sugar-devel] [PATCH 4/4] Restore load homepage functionality

2011-11-29 Thread Anish Mangal 
On Wed 30 Nov 2011 10:11:43 AM IST, Manuel Quiñones wrote:
 Signed-off-by: Manuel Quiñones ma...@laptop.org
 ---
  browser.py |   14 ++
  1 files changed, 6 insertions(+), 8 deletions(-)

 diff --git a/browser.py b/browser.py
 index 320cb62..789988d 100644
 --- a/browser.py
 +++ b/browser.py
 @@ -39,7 +39,11 @@ import sessionstore
  from widgets import BrowserNotebook
  
  _ZOOM_AMOUNT = 0.1
 -_LIBRARY_PATH = '/usr/share/library-common/index.html'
 +if os.path.isfile('/usr/share/library-common/index.html'):
 +_HOMEPAGE_PATH = '/usr/share/library-common/index.html'
 +else:
 +_HOMEPAGE_PATH = os.path.join(activity.get_bundle_path(),
 +data/index.html)
  
  
  class SaveListener(object):
 @@ -272,13 +276,7 @@ class TabbedView(BrowserNotebook):
  
  def load_homepage(self):
  browser = self.current_browser
 -
 -if os.path.isfile(_LIBRARY_PATH):
 -browser.load_uri('file://' + _LIBRARY_PATH)
 -else:
 -default_page = os.path.join(activity.get_bundle_path(),
 -data/index.html)
 -browser.load_uri(default_page)
 +browser.load_uri('file://' + _HOMEPAGE_PATH)
  
  def _get_current_browser(self):
  return self.get_nth_page(self.get_current_page()).get_child()

nitpicky
IMO, moving the code from here to above is probably fine, but removing 
_LIBRARY_PATH would be adding that extra bit of complexity to the code. 
Having the variable makes it clear that:

if LIBRARY_PATH:
   load it
else 
   load HOMEPAGE_PATH
   
Also, maybe having an else-if instead of else for the HOMEPAGE_PATH 
check and having all this in a try..except would be safer
/nitpicky

Reviewed-by: Anish Mangal an...@activitycentral.com

___
Sugar-devel mailing list
Sugar-devel@lists.sugarlabs.org
http://lists.sugarlabs.org/listinfo/sugar-devel