Closed Bug 223583 Opened 21 years ago Closed 21 years ago

Download manager rewrite

Categories

(Camino Graveyard :: Downloading, enhancement)

PowerPC
macOS
enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Camino0.8

People

(Reporter: jaas, Assigned: mikepinkerton)

Details

Attachments

(2 files, 8 obsolete files)

User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a) Gecko/20031023 Firebird/0.7+ Build Identifier: N/A Camino's download manager is being rewritten. Here you'll find screenshots, patches, and discussion of what it should be like. Reproducible: Always Steps to Reproduce:
Attached image download manager window screenshot (obsolete) (deleted) —
This is a screenshot from Interface Builder. There are some obvious inconsistencies, like the buttons being enabled when there are no downloads present or selected. This is just to give a general idea of the layout. There will be little images to the right of the text in the buttons. The Reload button will change to Cancel when an active download is selected.
Attached image Download instance screenshot (obsolete) (deleted) —
This is a screenshot from Interface Builder showing what an individual download instance will look like. This is just to give a general idea, and the icon has been pasted it since it doesn't show up in IB.
Josh would it be possible to give our users some text feedback if we open a file or pass it to a helper application. We cold use the space freed by the progress indicator and put a text string there: "Passed to Finder". I also think we should reconsider what prefernces we put with this new manager. - In panther we aren't able to set the download destination fro Camino, I think we should add this. So we can remove the "open internet preferences button" which doesn't work in Panther. Some other potential options which I think cold be usefull: _ auto open finished downloads in the finder. (we already have this) • auto clear history. • remember (x) amount of last downloads. - clear succesfull downloads form history. - clear history upon Quit.
All of that stuff sounds good, at least worth trying. I'll look at adding that stuff after everything more basic is functional.
Attached image actual screenshot of working dl manager (obsolete) (deleted) —
This is an actual screenshot of the new dl manager in action. The buttons aren't hooked up correctly and they have terrible icons now, so ignore those...
Attachment #134057 - Attachment is obsolete: true
Attachment #134058 - Attachment is obsolete: true
use white as the bg of the downloads, not the pinstripe. too busy. also we need smaller images for those toolbar buttons. Should they be the square pushbuttons? Is that the correct buttons style? anyone with some artistic background care to make it purdy? :)
I suggest we only use icons (with text on the right side) for the toolbar and not use buttons. Just tell me what size the icossn need to be and I'll make them.
Thanks for the offer Jasper (I added you to CC list on this bug, hopefully you don't mind). I think 20x20 icons would be good for now. If you could make them bigger (32x32?) but still scale nicely to 20x20 that would be best, in case things change. We need icons for reload, cancel, remove, and reveal. I'm not actually working on the DL manager at the moment because I'm waiting for panther-building changes to get checked in, so now is a good time to work out UI issues. I've already made the background white and tweaked some minor things in the download instances themselves. Are there any major/largish UI changes people want to suggest based on the screenshot attached to this bug? Little refinement suggestions should wait for a more finished product to base them on.
No problem Josh. I still have one thing and that is the url status bar, it's waste of space if you haven't got a download instance selected. Perhaps you could instead display the url using a tooltip and put a "copy source link" in the contextual menu?
Comments/feedback on Jasper's suggestion in Comment #9? I think it sounds good. Safari doesn't do any better, so I'm assuming this less forward way allowing people to access the source URL passed their UI guru tests. Pink?
yeah, jasper's comment sounds good to me. no reason the user needs to see that every time.
Re: comment 9, perhaps it's more 'discoverable' to have a "Get Info" button that would toggle the display of the URL and other attributes for all instances in the list, like the current disclosure triangle display, but with more global control.
That's another option, but since we only need to show the source URL (except ofcourse if there is other info you want to be displayed) I think we should go for the simplest solution. A tool tip doesn't need any click or other kind of actions, just a hoover. And a contextual menu is a 2 click action. Your idea is more complicated. An info button generally opens a info pane and not an other kind of view, so it would look odd. But it's an option. And I don't think it's worth the effort, the number of people that will need the source url will be very small especially since we will get the reload option, so let's give it a low profile UI.
I'm gonna cast my vote in with Jasper on this one. The more I think about it, a tooltip and a contextual menu is plenty. Adding the whole info system would add a lot to the complexity for a very small, if any, gain in usability.
is this a dup of bug 187193?
They're not dupes even though there is considerable overlap in the conversations surrounding them. This is a total GUI overhaul, that bug is tweaks to the existing one. What will probably happen is this new DL manager will eventually make that bug obsolete.
So what's up Josh how is everything going?
Things are coming along nicely, but I haven't had more than a couple hours to work on it in the past few weeks. I should have a near-finished product by the end of November if things go as planned. I did get rid of the source URL part of the window. The latest look and feel seems to work well - I use it frequently now for my own downloading. I'm quite happy with the UI decisions that have been made so far through our discussions.
There have been a lot of requests for features in this new DL manager and it got me thinking about when I should stop adding features/functionality and work on getting this checked in. I'm an hour or two of hacking from full feature parity with the current DL manager, and I don't really know what to do next in terms of prioritizing new features. Here is what I'd like to see happen - please comment on it so I can proceed with a plan. Once I reach feature parity with the existing download manager I'd like to make sure everything is as functional and stable as I think it is, post a patch, and work things out to get it checked in. I think its important to get rid of the DL manager that is there now simply because the new one is already much nicer, but also because I think it would probably be best to get comments from nightly users started as early as possible. There wouldn't be any feature regressions so the switch wouldn't be premature and other bugs could be filed for enhancements that people want. Then I could prioritize features based on bug votes and responses to the new DL manager from nightly users. So - either work on getting it checked in at feature parity, or add more stuff first. Which one, and if the latter, what features?
I think you should go for the checked in at feature parity. That way we all can give the new dl gui a spin and see what's good or not good. And at the same time people can look what kind of features they would like added. As for possible added feautures I would opt for very simple things as i mentioned before. _ auto open finished downloads in the finder. _ auto open finished downloads with helper application (we already have this) • auto clear history. • remember (x) amount of last downloads. - clear succesfull downloads form history. - clear history upon Quit (very optional) Perhaps now is also a good time to incorporate the ability for Camino to choose where files are downloaded to (a folder) since Apple removed that from the system preferences in Panther. I also hope you make sure that the window focus is not shifted from the main window to the dl manager when a dl starts. And that closing camino while in a dl wil give you the option to quit (or not) but that camino will remeber the downloads that where interupted.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Camino0.8
Attached file Download Manager Patch v1.0 (obsolete) (deleted) —
Merry Christmas Y'all - Here is v1.0 of the download manager patch. There is more to be done, but I think its good to get this landed before adding fun stuff like prefs and other customization stuff. It has all the features of the current download manager with a vastly improved user interface. Things to test are the keyboard navigation (up, down, and delete keys), selection behavior (shift and apple key/click modifiers), action validation (contextual menus and toolbar items), and status correctness.
Attachment #134228 - Attachment is obsolete: true
Attached file Download Manager Patch v1.1 (obsolete) (deleted) —
This version of the patch includes a fix for the 64 download limit bug (187509) and a lower minimum width for the download manager window.
Attachment #137948 - Attachment is obsolete: true
Attached file Download Manager Patch v1.2 (obsolete) (deleted) —
This version includes better wording for menus and tooltips, some further code cleanup, more space-efficient wording for download status, and better feedback for completed downloads (now shows total size and time).
Attachment #138085 - Attachment is obsolete: true
Josh I had a look at what I found a strange border. The problem is that you are using a border for the NSSCrollView which you shouldn't. In IB> info panel> attributes> border stule, you should choose no style (first icon). The scroll view will look a lot better as you will see. It would be nice if you would make the minimum NSScrollview height and window height 100px that way 2 dl instances will fit nicely in the window instead of 2 and 1/4 instance as we have now. I think you should enable the users to customize the toolbar. There is no reason why we shouldn't if we make sure the default is set as you do now. Oh and perhaps you could change the toolbar "launch" title to "open in Finder" or "open" just like in the contextual menu's. The last thing, perhaps we should use the more dark blue select color instead of the SelectTextBackground color for marking selected items?
Attached file Download Manager Patch v1.3 (obsolete) (deleted) —
This version lowers the minimum window width and height, removes the scroll view border, and allows users to customize the toolbar.
Attachment #138088 - Attachment is obsolete: true
patch comments: + NSSize mDefaultWindowSize; // is this necessary any more? you know best. if it's not used, please remove it. +// do this because there is no "no" key in the set of key modifiers +#define __NO_KEY 0 +#define __SHIFT_KEY 1 +#define __COMMAND_KEY 2 don't use #defines for constants, and don't use "__" to prefix them, they're reserved by the compiler. use instead: enum { kNoKey, kShiftKey, kCommandKey }; -+ (ProgressDlgController *)sharedDownloadController; ++(ProgressDlgController *)sharedDownloadController; while you're touching this line, get rid of the trailing semicolon (cosmetic) - if (gSharedProgressController == nil) + if (gSharedProgressController == nil) { gSharedProgressController = [[ProgressDlgController alloc] init]; + } no need to make this change just to add braces. +-(void)awakeFromNib { brace on following line for functions. happens in many functions. + for (i = 0; i < [selected count]; i++) { + [[selected objectAtIndex:i] cancel:self]; no need to keep checking |[selected count]| each time, just do it once before the loop starts. this is done in several other routines as well. furthermore, if |i| is only valid w/in the loop, declare it inside the for loop for (unsigned long i = 0; i < count....) { + // if dl instance after last selection exists, select it better comment on what's going on here, please +// cancel all selected instances +-(IBAction)reveal:(id)sender { fix comment +-(void)DLInstanceSelected:(NSNotification*)notification { + NSMutableArray* selectedArray = [self getSelectedProgressViewControllers]; + ProgressView *sender = ((ProgressView*)[notification object]); instead of just blindly casting, can you use the obj-c runtime to bulletproof this? return if |sender| isn't the right kind of object. more to come....still looking....
+ else if (key == (int)'\177') { // delete key - remove all selected items unless an active one is selected the powerbook uses a different key for this. we've got the constant hardcoded elsewhere in camino, you should try a search for it. +-(void)didEndDownload:(id <CHDownloadProgressDisplay>)progressDisplay + [self rebuildViews]; // to swap in the completed view +} isn't this expensive (rebuilding all views) when there are a lot of views and only 1 changes state? is there any way to only rebuild the one that finished? factor out the code to determine if commands are enabled for remove/open/cancel and share it between the validation routines. just makes them cleaner. +@interface ProgressView : NSView +{ + BOOL selected; + int lastModifier; + ProgressViewController* pvController; +} add a @private in here since you provide accessor methods. +-(NSMenu*)menuForEvent:(NSEvent*)theEvent; +-(NSView*)hitTest:(NSPoint)aPoint; if these are from NSView, you don't need to redeclare it here. just clutters the public interface. +-(NSMenu*)contextualMenu +{ .... why not just put the context menu into the nib?
63 dl instances bug is fixed. I had 71 instances completed in one session.
Attached file Download Manager Patch v1.4 (obsolete) (deleted) —
Updated to address pinkerton's comments
Attachment #138116 - Attachment is obsolete: true
Attached file Download Manager Patch v1.5 (deleted) —
This version includes support for page-up and page-down keys. Also fixes license indentation to minimize the diffs.
Attachment #138125 - Attachment is obsolete: true
1.5 landed with some cleanup, bug and leak fixes, and comments. thanks a million josh!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Attached file updated icons, from mikes comment (deleted) —
They are not perfect yet, but it works :)
Verified fixed in the 2004010603
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: