No description provided.
Summary: Handle Download Complete in the InfoBar → Make the download button in the appbar toggle the downloads infobar
This patch needs to be placed on top of the patches in bug 883951.
Updated with the latest pull so that it applies cleanly on top of patches from bug 883951. Note: while the infobar is toggled with this patch, this bug does not address UI/behaviour changes that affect how/where the bar is displayed (i.e. it is hidden when the context menu appears). These will be addressed as part of bug 886942.
Comment on attachment 772693 [details] [diff] [review] V2: Clicking the download button on the appbar toggles the download infobar. Review of attachment 772693 [details] [diff] [review]: ----------------------------------------------------------------- Looks good so far, looking forward to see how you test this stuff :) ::: browser/metro/base/content/appbar.js @@ +108,5 @@ > + Downloads._showDownloadCompleteNotification(this._xpcomObj); > + break; > + } > + this._xpcomObj = null; > + this._currNotification = null; I have to take your word for it that this logic is correct. You've tested multiple simultaneous downloads and notifications? And failure cases? What happens if you lose internet connection - is the state left correct? ::: browser/metro/base/content/downloads.js @@ +208,5 @@ > file.reveal(); > } > } > ]; > + let observerString = "download-complete"; maybe observerTopic = "download-complete" or just topic = @@ +271,5 @@ > } > } > ]; > > + let observerString = "download-progress"; ditto ::: browser/metro/components/HelperAppDialog.js @@ +97,5 @@ > downloadSize = this._getDownloadSize(aLauncher.contentLength); > > let msg = browserBundle.formatStringFromName("alertDownloadSave", [aLauncher.suggestedFileName, downloadSize, aLauncher.source.host], 3); > > + let observerString = "save-download"; ditto
Attachment #772693 - Flags: feedback?(sfoster) → feedback+
In reply to comment 3: * Made the small nit changes. * Right now when there are network issues, a download is paused and resumed when the issues are fixed but the UI isn't very clear about what's going on. That will be taken care of in bug 893763. * Multiple simultaneous downloads and notifications are a part of bug 883951 which still requires a couple of small changes but you're right that for this patch, if there are multiple notifications only the topmost one will be toggled. I'll update this patch to handle that once bug 883951 is taken care of. tl;dr: Will update/ship bug 883951 before getting back to this one
Attachment #772693 - Attachment is obsolete: true
Edits incorporated and work completed. Will land as a bundle in a future iteration.
Apparently there is a much simpler way of doing this! Wish I found it earlier.
Attachment #786388 - Flags: feedback?(sfoster) → review?(sfoster)
Attachment #786388 - Attachment description: toggle-notificationbox → v4: Clicking the download button on the appbar toggles the download infobar
You'll have to step me through this because as your reviewer I feel I should understand this simpler way too! And its not immediately clear to me looking across the patches.
Ah sorry for being unclear. Before I was manually keeping track of the notifications using observers. I would remove them to 'hide' them and reappend them to 'show' them. However, I noticed there is a notificationbox attribute, "notificationsHidden", which can simply be set to true or false to show or hide all notifications at once.
Comment on attachment 786388 [details] [diff] [review] v4: Clicking the download button on the appbar toggles the download infobar Review of attachment 786388 [details] [diff] [review]: ----------------------------------------------------------------- I look forward to seeing bug 895053 land, it niggles at me r+ing a series of patches without automated tests. But yes, this looks good and works well.
Attachment #786388 - Flags: review?(sfoster) → review+
Whiteboard: [preview] → [preview][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [preview][fixed-in-fx-team] → [preview]
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.