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

2011-11-30 Thread Rafael Ortiz
2011/11/30 Anish Mangal 

> On Wed 30 Nov 2011 10:11:43 AM IST, Manuel Quiñones wrote:
> > Signed-off-by: Manuel Quiñones 
> > ---
> >  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()
>
> 
> 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
> 
>
>
+1.


> Reviewed-by: Anish Mangal 
>
> ___
> 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 
> ---
>  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()


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


Reviewed-by: Anish Mangal 

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