GNOME Bugzilla – Bug 111122
[download] Fix Pause/Resume
Last modified: 2004-12-22 21:47:04 UTC
(daveb, sorry about copying your [] stuff ;) So, Mozilla supports resume for channels different than FTP since ages ago, and our embed code assumes it only works for FTP. Besides, the code was broken even for FTP (tsk tsk marco), so this patch removes crap and just assumes Pause/Resume support for every type of channel. I asked darin, the maintanier of Necko, about this and he said it was ok to assume that in out context, as only some Mail and other obscure channels do not support this option. Anyway, download still needs lots of love. Marco, do you agree about mergin pause/resume in one button? You told me once it could cause problems with multiple downloads, but can't figure what kind of problems... There's also some weirdness in the remaining time computation, and the resize stuff (there's a bug filled about it).
Created attachment 15844 [details] [review] Fix for pause/resume
IMO in general it is better to have individual buttons for each control. This way it is easier to develop muscle memory and to remember where certain buttons are.
Dave, I agree in general. In this case though I think it's pretty important to have space for one more button (Close/Help). I dont think Pause/Resume are used that often.
While we discuss this, may I commit the fix? :)
Sure ;) I thought you was already doign the ui changes. Thank you !
Ok, the UI show so far: - Merged pause/resume in one button. - Put the view to SELECTION_SINGLE, can't do multiple-actions now. - Removed File: from details view. Possible TODO: - Completely remove details view (daveb agrees, will try to do a mockup) - Remove the keep open checkbox Regardless of your opinion on the TODO, can I commit what I've already done?
Created attachment 16088 [details] [review] UI work, v 1.0
Large changes, hard to review :/ It looks fine but please test it well before committing. It may be worth to rename the pause button callback to reflect the fact that it's for both pause and resume.
I'm attaching my proposed workaround for the problems in the download dialog, already commented in the mail I sent you Marco. I think we should probably rewrite most of it to do things properly, but this works (as far as I can see), and it's not very intrusive. What do you think?
Created attachment 16186 [details] [review] UI work + dirty hack 2.0
Sorry for the big patch (and this spam). I can try to cut it in pieces if it's too much for one go. Btw, it seems that InitForDownload is not used anywhere in Epiphany (I've commented it out and everything works ok, it's never called), ok to remove it too?
I think there are two approaches to the downloader api: 1 Current. We emit a signal when the user request to pause a download, and the ProgressListener code actually Pause it 2 We wrap the mozilla persisting api (see ephy-embed-persist.c) and we add a pause function to it. Now while 2 seem definately better there is a conflict with how mozilla downloads works. There are 2 cases: A We ask mozilla to download a file (this is the case of the context menu download). B Mozilla start a download itself, this is the click on a tgz case In 2 we just override the ui mozilla shows. The filepicker and the ProgressListener. Obviously we cant register our gobject as mozilla progress listener ... so I dont see a way to implement 2 with current way downloads works.
So in practice unless you see a better to way to do this, I'd go with your solution. I dont think it's an hack (we are just noitifying with a signal that user has requested a download to be stopped), obviously it's not optimal but ... One thing I dont understand in your patch is that you are removeing DownloaderView * from signals. All signal pass a pointer to the object emitted them as first arg ... (You can remove that code if unused)
Mi biggest concern is that we hook every download to the signals, and when we emit the actual pause/resume/remove signal, every one of them is activated although we are only acting on one (so we need to do the, IMHO, dirty check in the callbacks). Maybe I'm being picky? I think the method sucks. I'll try to investigate it further, but for 0.6.0 I suggest commiting this. And ok, I'll re-add the parameter (not really sure why I did that) and remove the code. So, ok to commit with that?
>Mi biggest concern is that we hook every download to the signals, >and when we emit the actual pause/resume/remove signal, every one >of them is activated although we are only acting on one (so we need >to do the, IMHO, dirty check in the callbacks). Maybe I'm being >picky? I think the method sucks. Well the downloader interface is emitting a signal to notify that the user requested one download to be stopped. This is not bad ihmo. I must say I dont like to use callbacks to request actions to be taken, they are used that way sometimes though ... (see the egg create_menu_item signal I've seen two minutes ago for example). I'd certainly be happy to avoid it, but I explained before why we cant do it with current mozilla download api ... :/ >I'll try to investigate it further, >but for 0.6.0 I suggest commiting this. >And ok, I'll re-add the parameter (not really sure why I did that) >and >remove the code. So, ok to commit with tha >t? Yeah please commit. Is the crash on pause on finished downloads fixed too ? Thanks !
Yes, it does not crashes more for me. I think that it crashed because when we clicked paused we were actually pausing every download, even finished ones, which I assume is braindead and causes mozilla to crash. Commiting and closing, thanks for the help :)