Browse: display a message while downloading a PDF

[manuq]

Currently it displays a blank page while the PDF is downloading. Instead, display a message with the same style as others in Sugar, like in the Journal and the "no matches" in the Home activities list view.

As a plus, the icon can be animated to display the download progress.

[manuq]

New IconProgress widget for toolkit.

[manuq]

Browse patch.

[manuq]

Screenshot.

[manuq]

CC'ing Gary for design review, humitos for Browse review, erikos for toolkit review.

[manuq]

While testing, please consider this other bug I've just found: #4385 .

[humitos]

Replying to manuq:

CC'ing Gary for design review, humitos for Browse review, erikos for toolkit review.

Wow! This feature looks awesome for me. I like the style, the idea, and I think the user experience is much better now... I'm happy :)

I have some minor comments about your patches:

• sugar-toolkit-gtk3: set the default value for direction in the argument instead of using None and then check if that argument is None:

  diff --git a/src/sugar3/graphics/iconprogress.py b/src/sugar3/graphics/iconprogress.py
  index 9f1c5a4..40e2712 100644
  --- a/src/sugar3/graphics/iconprogress.py
  +++ b/src/sugar3/graphics/iconprogress.py
  @@ -34,12 +34,10 @@ class IconProgress(Gtk.DrawingArea):
   
       """
       def __init__(self, icon_name, pixel_size, stroke_color, fill_color,
  -                 direction=None):
  +                 direction='vertical'):
           Gtk.DrawingArea.__init__(self)
   
           self._icon_name = icon_name
  -        if direction == None:
  -            direction = 'vertical'
           self._direction = direction
           self._progress = 0
  

• browse: I don't like (personal opinion) the use of self.get_children()[0] because if later we add a new child to PDFTabPage maybe the PDFMessageBox is not in the [0] position anymore. I prefer using self.message_box = PDFMessageBox.

  diff --git a/pdfviewer.py b/pdfviewer.py
  index a3ededa..bd52bba 100644
  --- a/pdfviewer.py
  +++ b/pdfviewer.py
  @@ -408,11 +408,11 @@ class PDFTabPage(Gtk.HBox):
           """Download the PDF from a remote location to a temporal file."""
   
           # Display a message
  -        message_box = PDFMessageBox(
  +        self.message_box = PDFMessageBox(
               message=_("Downloading document..."),
               button_callback=self.cancel_download)
  -        self.pack_start(message_box, True, True, 0)
  -        message_box.show()
  +        self.pack_start(self.message_box, True, True, 0)
  +        self.message_box.show()
   
           # Figure out download URI
           temp_path = os.path.join(activity.get_activity_root(), 'instance')
  @@ -438,8 +438,7 @@ class PDFTabPage(Gtk.HBox):
           self._browser.props.progress = progress
   
           # Update progress in the message box too
  -        message_box = self.get_children()[0]
  -        message_box.icon_progress.update(progress)
  +        self.message_box.icon_progress.update(progress)
   
       def __download_status_cb(self, download, data):
           status = download.get_status()
  @@ -449,8 +448,7 @@ class PDFTabPage(Gtk.HBox):
               self._browser.props.load_status = WebKit.LoadStatus.FINISHED
   
               # Remove the message box
  -            message_box = self.get_children()[0]
  -            self.remove(message_box)
  +            self.remove(self.message_box)
   
               self._show_pdf()
           elif status == WebKit.DownloadStatus.CANCELLED:
  

Besides these little comments, the patches looks good for me. #4385 happened to me many times while I was testing this. Testing this patch with a fresh instance of Browse works perfect.

[manuq]

Thanks humitos.

Replying to humitos:

I have some minor comments about your patches:

• sugar-toolkit-gtk3: set the default value for direction in the argument instead of using None and then check if that argument is None:

There are important reasons to not use mutable objects as default parameters in Python:

​http://effbot.org/zone/default-values.htm

• browse: I don't like (personal opinion) the use of self.get_children()[0] because if later we add a new child to PDFTabPage maybe the PDFMessageBox is not in the [0] position anymore. I prefer using self.message_box = PDFMessageBox.

Good, I will change it to a private property, _message_box (name starting with a '_').

[humitos]

Replying to manuq:

There are important reasons to not use mutable objects as default parameters in Python:

Yes, but Strings are immutable objects in Python ;)

Good, I will change it to a private property, _message_box (name starting with a '_').

Better!

[manuq]

Replying to manuq:

Thanks humitos.

Replying to humitos:

I have some minor comments about your patches:

• sugar-toolkit-gtk3: set the default value for direction in the argument instead of using None and then check if that argument is None:

There are important reasons to not use mutable objects as default parameters in Python:

​http://effbot.org/zone/default-values.htm

Oh but strings are immutable :P So I'll add your change. Thanks!

[erikos]

Fantastic work Manuel, I really like it. Even RTL is handled. And it can be used in other places to show progress in an animation.

As a class name I would prefer ProgressIcon instead of IconProgress, I think that sounds better. Thanks for doing the test case as well, that is good habbit, we just have to integrate with sugar-build those now.

And yes, I second the suggestion to put a default direction.

_draw_cb ---> draw_cb

[manuq]

Tnanks Simon. I've made the fixes and pushed toolkit 693eaab9 and Browse ebac9a46 .

[dnarvaez]

Milestone 1.0 deleted