Closed
Bug 223583
Opened 21 years ago
Closed 21 years ago
Download manager rewrite
Categories
(Camino Graveyard :: Downloading, enhancement)
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:
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.
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.
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•21 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? :)
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?
Reporter | ||
Comment 10•21 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•21 years ago
|
||
yeah, jasper's comment sounds good to me. no reason the user needs to see that
every time.
Comment 12•21 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•21 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•21 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•21 years ago
|
||
is this a dup of bug 187193?
Reporter | ||
Comment 16•21 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•21 years ago
|
||
So what's up Josh how is everything going?
Reporter | ||
Comment 18•21 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•21 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•21 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•21 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Target Milestone: --- → Camino0.8
Reporter | ||
Comment 21•21 years ago
|
||
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
Reporter | ||
Comment 22•21 years ago
|
||
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•21 years ago
|
||
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
Comment 24•21 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•21 years ago
|
||
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
Assignee | ||
Comment 26•21 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•21 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•21 years ago
|
||
63 dl instances bug is fixed. I had 71 instances completed in one session.
Reporter | ||
Comment 29•21 years ago
|
||
Updated to address pinkerton's comments
Attachment #138116 -
Attachment is obsolete: true
Reporter | ||
Comment 30•21 years ago
|
||
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
Assignee | ||
Comment 31•21 years ago
|
||
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
Comment 32•21 years ago
|
||
They are not perfect yet, but it works :)
You need to log in
before you can comment on or make changes to this bug.
Description
•