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

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 3

[Chris Peterson [:cpeterson]]

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.  <:\

Comment 4

[Chris Peterson [:cpeterson]]

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?

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: Screenshot of Camino 2.0

Selection uses selectedTextBackgroundColor.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: Screenshot of Camino 2.1 with alternateSelectedControlColor

using download-colors-v1.patch

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Screenshot of Camino 2.1 with controlAlternatingRowBackgroundColors

Example using alternating row background colors.

Comment 8

[Stuart Morgan]

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.

Comment 9

[Chris Peterson [:cpeterson]]

Attachment: 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?

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(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!

Comment 11

[Chris Peterson [:cpeterson]]

Attachment: 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".

Comment 12

[philippe (part-time)]

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

Comment 13

[Chris Peterson [:cpeterson]]

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

[Ilya Sherman]

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.

Comment 15

[Chris Peterson [:cpeterson]]

Attachment: download-colors-v3.patch

Thanks for the feedback, Ilya. Patch v3 incorporates your suggested changes.

Comment 16

[Ilya Sherman]

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.

Comment 17

[Chris Peterson [:cpeterson]]

Attachment: download-colors-v4.patch

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

Now to Smokey for functionality review.

Comment 18

[philippe (part-time)]

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

Comment 19

[philippe (part-time)]

Attachment: 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.

Comment 20

[Chris Peterson [:cpeterson]]

Attachment: 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).

Comment 21

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 22

[philippe (part-time)]

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

Comment 23

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 24

[philippe (part-time)]

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 25

[philippe (part-time)]

Comment on attachment 492090 [details] [diff] [review]
download-colors-v6.patch

r = me

Comment 26

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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 ;) )

Comment 27

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 492090 [details] [diff] [review]
download-colors-v6.patch

sr=pink

Comment 28

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/d94bf3411454

Thanks for fixing this, Chris, and sorry about the long holiday delay in getting it in.