Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4384 closed enhancement (fixed)

Browse: display a message while downloading a PDF

Reported by: manuq Owned by: manuq
Priority: Unspecified by Maintainer Milestone:
Component: Browse Version: Unspecified
Severity: Unspecified Keywords: r+, olpc-test-pending
Cc: garycmartin, humitos, erikos Distribution/OS: Unspecified
Bug Status: Unconfirmed

Description

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.

Attachments (3)

0001-New-IconProgress-widget-SL.patch (7.3 KB) - added by manuq 8 years ago.
New IconProgress widget for toolkit.
0001-Add-a-message-box-while-the-PDF-is-being-downloaded-.patch (6.7 KB) - added by manuq 8 years ago.
Browse patch.
downloading.png (29.8 KB) - added by manuq 8 years ago.
Screenshot.

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by manuq

New IconProgress widget for toolkit.

Changed 8 years ago by manuq

Screenshot.

comment:1 follow-up: Changed 8 years ago by manuq

  • Cc garycmartin humitos erikos added
  • Keywords r? added

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

comment:2 Changed 8 years ago by manuq

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

comment:3 in reply to: ↑ 1 ; follow-up: Changed 8 years ago by 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.

comment:4 in reply to: ↑ 3 ; follow-ups: Changed 8 years ago by 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 '_').

comment:5 in reply to: ↑ 4 Changed 8 years ago by 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!

comment:6 in reply to: ↑ 4 Changed 8 years ago by 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!

comment:7 Changed 8 years ago by 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

comment:8 Changed 8 years ago by manuq

  • Keywords r+ olpc-test-pending added; r? removed
  • Resolution set to fixed
  • Status changed from new to closed

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

comment:9 Changed 8 years ago by dnarvaez

  • Milestone 1.0 deleted

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.