Download manager rewrite

VERIFIED FIXED in Camino0.8

Status

Camino Graveyard
Downloading
--
enhancement
VERIFIED FIXED
14 years ago
14 years ago

People

(Reporter: Josh Aas, Assigned: Mike Pinkerton (not reading bugmail))

Tracking

unspecified
Camino0.8
PowerPC
Mac OS X

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

14 years ago
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:
(Reporter)

Comment 1

14 years ago
Created attachment 134057 [details]
download manager window screenshot

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.
(Reporter)

Comment 2

14 years ago
Created attachment 134058 [details]
Download instance screenshot

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.

Comment 3

14 years ago
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.
(Reporter)

Comment 4

14 years ago
All of that stuff sounds good, at least worth trying. I'll look at adding that
stuff after everything more basic is functional.
(Reporter)

Comment 5

14 years ago
Created attachment 134228 [details]
actual screenshot of working dl manager

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
(Assignee)

Comment 6

14 years ago
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? :)

Comment 7

14 years ago
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.
(Reporter)

Comment 8

14 years ago
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.

Comment 9

14 years ago
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?
(Reporter)

Comment 10

14 years ago
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?
(Assignee)

Comment 11

14 years ago
yeah, jasper's comment sounds good to me. no reason the user needs to see that
every time.

Comment 12

14 years ago
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.

Comment 13

14 years ago
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.
(Reporter)

Comment 14

14 years ago
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.

Comment 15

14 years ago
is this a dup of bug 187193?
(Reporter)

Comment 16

14 years ago
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.

Comment 17

14 years ago
So what's up Josh how is everything going?
(Reporter)

Comment 18

14 years ago
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.
(Reporter)

Comment 19

14 years ago
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?

Comment 20

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Camino0.8
(Reporter)

Comment 21

14 years ago
Created attachment 137948 [details]
Download Manager Patch v1.0

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.
(Reporter)

Updated

14 years ago
Attachment #134228 - Attachment is obsolete: true
(Reporter)

Comment 22

14 years ago
Created attachment 138085 [details]
Download Manager Patch v1.1

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
(Reporter)

Comment 23

14 years ago
Created attachment 138088 [details]
Download Manager Patch v1.2

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).
(Reporter)

Updated

14 years ago
Attachment #138085 - Attachment is obsolete: true

Comment 24

14 years ago
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?
(Reporter)

Comment 25

14 years ago
Created attachment 138116 [details]
Download Manager Patch v1.3

This version lowers the minimum window width and height, removes the scroll
view border, and allows users to customize the toolbar.
(Reporter)

Updated

14 years ago
Attachment #138088 - Attachment is obsolete: true
(Assignee)

Comment 26

14 years ago
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....
(Assignee)

Comment 27

14 years ago
+    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?

Comment 28

14 years ago
63 dl instances bug is fixed. I had 71 instances completed in one session.
(Reporter)

Comment 29

14 years ago
Created attachment 138125 [details]
Download Manager Patch v1.4

Updated to address pinkerton's comments
(Reporter)

Updated

14 years ago
Attachment #138116 - Attachment is obsolete: true
(Reporter)

Comment 30

14 years ago
Created attachment 138147 [details]
Download Manager Patch v1.5

This version includes support for page-up and page-down keys. Also fixes
license indentation to minimize the diffs.
(Reporter)

Updated

14 years ago
Attachment #138125 - Attachment is obsolete: true
(Assignee)

Comment 31

14 years ago
1.5 landed with some cleanup, bug and leak fixes, and comments. thanks a million
josh!
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 32

14 years ago
Created attachment 138181 [details]
updated icons, from mikes comment

They are not perfect yet, but it works :)

Comment 33

14 years ago
Verified fixed in the 2004010603
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.