Selected download should use list selection color, not text highlight color

RESOLVED FIXED

Status

Camino Graveyard
Downloading
--
minor
RESOLVED FIXED
11 years ago
7 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: cpeterson)

Tracking

({polish})

Details

Attachments

(6 attachments, 5 obsolete attachments)

Our Downloads window is a list or table or something approximating that, yet we're using the text highlight color instead (and there's no selectable text).

When we clean up the mess that is that window, we need to fix this, too.
Severity: normal → minor
This would be reasonably simple to fix by changing 
[[NSColor selectedTextBackgroundColor] set];
to
[[NSColor alternateSelectedControlColor] set];
in ProgressView.mm, except for the fact that we also need to flip the text colors (from black and grey to white) when their row is selected; otherwise, the grey is hard to read in addition to being wrong.
(Assignee)

Updated

7 years ago
Assignee: nobody → mozilla.org
Hardware: PowerPC → All
(Assignee)

Comment 2

7 years ago
Created attachment 485999 [details] [diff] [review]
download-colors-v1.patch

This patch:
1. Highlight selected download rows' background color with |alternateSelectedControlColor| (e.g. dark blue).
2. Invert selected download rows' text color with |alternateSelectedControlTextColor| (e.g. white).
3. Deselect selected download rows when the user clicks in within the empty area of the Downloads scroll view.
Attachment #485999 - Flags: review?
(Assignee)

Updated

7 years ago
Attachment #485999 - Flags: review? → review?(ishermandom+bugs)
(Assignee)

Comment 3

7 years ago
After having implemented the "correct" row selection color, I must admit that the previous row selection color (light blue text highlight color) looks nicer, less heavy.  <:\
Status: NEW → ASSIGNED
(Assignee)

Comment 4

7 years ago
I also have a WIP patch that uses |[NSColor controlAlternatingRowBackgroundColors]| (e.g. alternating white and light blue) for the Downloads window's rows. That code change is a little messy and I wanted to check whether that functionality is even desirable for Camino?
(Assignee)

Comment 5

7 years ago
Created attachment 486004 [details]
Screenshot of Camino 2.0

Selection uses selectedTextBackgroundColor.
(Assignee)

Comment 6

7 years ago
Created attachment 486005 [details]
Screenshot of Camino 2.1 with alternateSelectedControlColor

using download-colors-v1.patch
(Assignee)

Comment 7

7 years ago
Created attachment 486006 [details]
Screenshot of Camino 2.1 with controlAlternatingRowBackgroundColors

Example using alternating row background colors.

Comment 8

7 years ago
Alternating row colors seems a bit busy to me, so I would lean toward not using it, but I don't have strong feeling about it.

I'm not liking all the text now being black though.
(Assignee)

Comment 9

7 years ago
Created attachment 486292 [details] [diff] [review]
download-colors-v2.patch

Patch v2 fixes the gray text color of the download status and time labels. (oops!)

The gray text color was manually specified in IB, so there is no named |NSColor| constant. I made |ProgressViewController init:| query and cache its labels' original, unselected text colors. Is there an easier way to do this?
Attachment #485999 - Attachment is obsolete: true
Attachment #486292 - Flags: review?(ishermandom+bugs)
Attachment #485999 - Flags: review?(ishermandom+bugs)
(In reply to comment #3)
> After having implemented the "correct" row selection color, I must admit that
> the previous row selection color (light blue text highlight color) looks nicer,
> less heavy.  <:\

Yeah, that was my initial feeling when I implemented just that part; we've been doing the wrong thing for so long it's a bit of an adjustment.

(In reply to comment #8)
> Alternating row colors seems a bit busy to me, so I would lean toward not using
> it, but I don't have strong feeling about it.

I kind of like it, but I'd like to see what it looks like with the progress meter (and also how it is in handling row updates, e.g. repaints for deletions of ProgressViews).

If the code is at all hairy, I guess I'd lean towards not doing it (even though Safari does), at least until we get rid of the custom view nightmare in that nib!
(Assignee)

Comment 11

7 years ago
Created attachment 486295 [details]
Screenshot with selected progress bar

Here is a screenshot of a selected progress bar with the "correct" selected control color (dark blue). Now that I fixed the black and gray text colors (that Stuart pointed out), the dark blue background does not look as "heavy".
(In reply to comment #8)
> Alternating row colors seems a bit busy to me, so I would lean toward not using
> it, but I don't have strong feeling about it.

Removing the separator line between each row would make it less busy is think.
attachment 486006 [details] has both.
(Assignee)

Comment 13

7 years ago
I tried alternating row colors without the separator lines. It looks OK, but it opens the problem that the alternating row colors stop at the last download item, which looked strange.

I have not yet investigated how to extend the alternating row colors. That would open a strange world where the download items' row colors are drawn by their ProgressViewControllers, but the empty rows filling the remainder of the Downloads window are drawn by someone else (the ProgressDlgController?).

Comment 14

7 years ago
Comment on attachment 486292 [details] [diff] [review]
download-colors-v2.patch

Apologies for being MIA for so long!

This approach looks reasonable in general, but I think we can make it a little cleaner:

>+  NSColor*        mFilenameLabelColor;
>+  NSColor*        mStatusLabelColor;
>+  NSColor*        mTimeLabelColor;

nit: These names would be clearer as |mFilenameLabelUnselectedColor|, etc.

>+-(NSColor*)retainLabelColor:(int)labelTag;

nit: I would call this method something like |-labelColorForTag:|.

>+-(NSColor*)retainLabelColor:(int)labelTag
>+{
>+  return [[[[self view] viewWithTag:labelTag] textColor] retain];
>+}

nit: Rather than |retain| within this method, just return the color here, and let the caller retain it.  I believe that is a more standard pattern in Obj-C code.

> -(void)setSelected:(BOOL)inSelected
> {
>   if (mIsSelected != inSelected)
>   {
>     mIsSelected = inSelected;
>     [[self view] setNeedsDisplay:YES];
>+    [self refreshDownloadInfo];

This seems overmuch to me -- it would be cleaner to have a method that only updates the label colors as needed.  This would also make |refreshDownloadInfo| less fragile, as you wouldn't have to remember to set the color in each code path through the method.
Attachment #486292 - Flags: review?(ishermandom+bugs) → review-
(Assignee)

Comment 15

7 years ago
Created attachment 491454 [details] [diff] [review]
download-colors-v3.patch

Thanks for the feedback, Ilya. Patch v3 incorporates your suggested changes.
Attachment #486292 - Attachment is obsolete: true
Attachment #491454 - Flags: review?(ishermandom+bugs)

Comment 16

7 years ago
Comment on attachment 491454 [details] [diff] [review]
download-colors-v3.patch

> -(void)setSelected:(BOOL)inSelected
> {
>   if (mIsSelected != inSelected)
>   {
>     mIsSelected = inSelected;
>     [[self view] setNeedsDisplay:YES];
>+    [self refreshLabelColors];
>   }
> }

nit: While functionally equivalent, I think this would be slightly clearer if the call to -refreshLabelColors: came before the call to -setNeedsDisplay:, since the refreshed label colors are needed for proper display.

r=ilya with that change


Camino builds are still hanging on my computer (grr...), so tagging Smokey for functionality review.
Attachment #491454 - Flags: review?(ishermandom+bugs)
Attachment #491454 - Flags: review?(alqahira)
Attachment #491454 - Flags: review+
(Assignee)

Comment 17

7 years ago
Created attachment 491463 [details] [diff] [review]
download-colors-v4.patch

Good idea. Patch v4 calls -refreshLabelColors before -setNeedsDisplay.

Now to Smokey for functionality review.
Attachment #491454 - Attachment is obsolete: true
Attachment #491463 - Flags: review?(alqahira)
Attachment #491454 - Flags: review?(alqahira)
(In reply to comment #17)
> Created attachment 491463 [details] [diff] [review]
> download-colors-v4.patch

With this patch I see one issue:

When the selected download changes state (from in progress to completed), the text is displayed in black + grey instead of white. That is hard to read, esp with the graphite theme. As soon as I force a focus - e.g. press down-arrow with the bottom most download (= most recent) -  the colors revert to white.

The same issue happens when starting up the browser (and the last download had focus and the download window was left open).

(That may have been an issue prior to this patch actually, except one doesn't notice it - the colors don't change... in release and nightly builds)
Created attachment 491470 [details]
screenshot for comment 18

What I describe in comment 18
Don't ask me what's written under 'Camino.dmg' :-( The old eyes don't want to play along.
(Assignee)

Comment 20

7 years ago
Created attachment 491777 [details] [diff] [review]
download-colors-v5.patch

Fixed. My previous patch tried to minimize unnecessary UI updates, but I forgot two code paths: when the download completes and when the browser launches (with old downloads in the Downloads window).
Attachment #491463 - Attachment is obsolete: true
Attachment #491777 - Flags: review?(alqahira)
Attachment #491463 - Flags: review?(alqahira)
Comment on attachment 491777 [details] [diff] [review]
download-colors-v5.patch

Looks like you accidentally diffed your entire tree here ;)  Please respin this with only the bits for this bug for sane testing ;)

I'm unlikely to be able to put this (or the parts of this that are for this bug) through its paces until I'm back next month.  Rather than bottle this patch up until then, I'm happy to delegate functionality review to philippe (if it hasn't cleared sr by the time I'm back, I'll take a pass then and make sure I don't see anything else, and otherwise will file follow-ups if I notice anything after it's landed).
Attachment #491777 - Flags: review?(alqahira)
(In reply to comment #20)
> Created attachment 491777 [details] [diff] [review]
> download-colors-v5.patch
> 
> Fixed. My previous patch tried to minimize unnecessary UI updates, but I forgot
> two code paths: when the download completes and when the browser launches (with
> old downloads in the Downloads window).

Does it also fixes the case when the user pauses/cancels a download in progress? Patch v4 has the same issue as completed downloads.
(Assignee)

Comment 23

7 years ago
Created attachment 492090 [details] [diff] [review]
download-colors-v6.patch

oops! Patch v6 contains just the changes for this bug.

And I confirmed that patch v6 (and v5) fixed both the "download completed" and "download cancelled" cases.
Attachment #491777 - Attachment is obsolete: true
Attachment #492090 - Flags: review?(phiw)
1. The color issues as noted above (comment 18) now work correctly. :-)

2. Functionality wise, I didn't notice any issues but see (3) below. Unfortunately I seem to have lost my big download.plist file (300+ downloads) to acid test responsiveness. But with about 50 downloads (completed/canceled/removed/ and 3 in progress) I didn't notice any slowdowns, odd repainting issues or similar.

3. one issue is much visible now than it was before: when the downloads window is *not* the front most window, the background color for the selected download is not grayed out.
(it is at least much more visible for me given my preferences for greyish colors - graphite/graphite. Someone who chooses 'gold' or 'red' as preferred color might disagree)

I don't want to block progress on this patch for that. I'll file a follow-up bug.
Comment on attachment 492090 [details] [diff] [review]
download-colors-v6.patch

r = me
Attachment #492090 - Flags: review?(phiw) → review+
Comment on attachment 492090 [details] [diff] [review]
download-colors-v6.patch

I had a look at this, too, and it looks good to me as well, with the exception of philippe's point 3, which got filed as bug 613833.

I'm not sure who's a better choice right now in superreviewer roulette :( but overall pink probably has less Camino stuff on his plate (and we reminded him of his queue today ;) )
Attachment #492090 - Flags: superreview?(mikepinkerton)
Attachment #492090 - Flags: review+
Comment on attachment 492090 [details] [diff] [review]
download-colors-v6.patch

sr=pink
Attachment #492090 - Flags: superreview?(mikepinkerton) → superreview+
http://hg.mozilla.org/camino/rev/d94bf3411454

Thanks for fixing this, Chris, and sorry about the long holiday delay in getting it in.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.