613833 - Background of selected download is not grayed out when Downloads window is not front most

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[philippe (part-time)]

STR:
1. start a download
2. send Downloads window to the background (focus on main window)
AR: the background-color of the selected download remains colored
ER: background is grayed out (as are toolbars/scrollbars/...)

The issue will become much more visible after bug 380606; the colors are much stronger, the downloads window becomes a distraction.

Comment 1

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

Yeah, we should try to fix this shortly after bug 380606 lands.  I assume that since these are our own custom views, we'll need to have some object in that window hierarchy listen for key/main change notifications and fire off another round of UI updates to swap in the background window selection color.

Comment 2

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

Taking; Stuart pointed me at some stuff, and I have this half-working right now :)

Comment 3

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

Attachment: WIP

Here's the current status of this: I have the view color working properly, and I have the label colors working properly on window layering changes.

However, whenever the two views are swapped (when a download finishes [in the foreground], when the user presses pause, and when the user presses resume [for a brief but noticeable period]), I end up with the foreground control color and the unselected/background text label colors.

My working theory is that the completed/paused view is initialized in the background (before/as the window is opening) and still thinks it is when the download finishes, but that's not a very complete explanation.

Things I've tried, but which didn't work:

1) Making refreshLabelColors public and having the PV call it when it gets a notification (rather than having both PV and PVC listening for the notifications)

2) Ilya suggested perhaps I needed to set the label colors for both mProgressView and mCompletedView rather than just for curView/[self view]

3) A combination of the two

So I'm not sure exactly what's going on, or how to debug it, or how to fix it :(

Comment 4

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

Attachment: Fix

Stuart pointed out that the ProgressDlgController logic on, say, pause is:

1) Call PVC's pause:
2) Rebuild the view hierarchy

PVC's pause: eventually calls refreshLabelColors, but it's calling it on the other view, which (as Ilya noted the other night on irc) isn't in a window yet.

So my curView window] isKeyWindow] check always amounted to [nil isKeyWindow], which was NO every time.

Ultimately Stuart thinks that the PV should control the label colors in drawRect: just as it does the view color, but that's a bigger change than I wanted to undertake right now/here.

This fix (which is just an "if we don't have foo's window, use bar's" check on top of the previous patch) lets us get the downloads colors working 100% properly in a manner I can handle in time for a1 ;)

Comment 5

[Ilya Sherman]

Comment on attachment 502629 [details] [diff] [review]
Fix

It seems like only one of ProgressView and ProgressViewController should be listening for these new notifications; otherwise, I'd expect there to be a race condition between ProgressViewController setting the appropriate colors and ProgressView being redrawn.

>+  if (mIsSelected && [enclosingWindow isKeyWindow]) {

Erm, shouldn't this be [[self enclosingWindow] isKeyWindow]?

Comment 6

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

Attachment: Fix, now diffed after compiling

(In reply to comment #5)
> It seems like only one of ProgressView and ProgressViewController should be
> listening for these new notifications; otherwise, I'd expect there to be a race
> condition between ProgressViewController setting the appropriate colors and
> ProgressView being redrawn.

Stuart explained that they'll both call setNeedsDisplay: during the same event loop, so there's no race.  (It obviously is ugly, but so is having the labels in one file and the view color in another; fixing the latter should clean up the former).

> >+  if (mIsSelected && [enclosingWindow isKeyWindow]) {
> 
> Erm, shouldn't this be [[self enclosingWindow] isKeyWindow]?

Yes.  Apparently I forgot to re-diff after getting the patch to compile and work :oops:

Comment 7

[Ilya Sherman]

Comment on attachment 502678 [details] [diff] [review]
Fix, now diffed after compiling

(In reply to comment #6)
> (In reply to comment #5)
> > It seems like only one of ProgressView and ProgressViewController should be
> > listening for these new notifications; otherwise, I'd expect there to be a race
> > condition between ProgressViewController setting the appropriate colors and
> > ProgressView being redrawn.
> 
> Stuart explained that they'll both call setNeedsDisplay: during the same event
> loop, so there's no race.  (It obviously is ugly, but so is having the labels
> in one file and the view color in another; fixing the latter should clean up
> the former).

Ok, sounds good then =)

r=ilya

Comment 8

[philippe (part-time)]

Comment on attachment 502678 [details] [diff] [review]
Fix, now diffed after compiling

Testing this with a list of 25 downloads or so, of which one is active, one paused, everything seems to work fine.

Much more comfortable to the eyes when the Downloads window is in the background:-).

Comment 9

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

Comment on attachment 502678 [details] [diff] [review]
Fix, now diffed after compiling

Stuart, you want to look at this ("again"), or should pink?

Comment 10

[Stuart Morgan]

Comment on attachment 502678 [details] [diff] [review]
Fix, now diffed after compiling

>+- (void)viewDidMoveToWindow
>+{
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowKeyStatusChanged:)
>+                                               name:NSWindowDidBecomeKeyNotification
>+                                             object:[self window]];
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowKeyStatusChanged:)
>+                                               name:NSWindowDidResignKeyNotification
>+                                             object:[self window]];
>+}

This should be:

if ([self window]) {
  <what you have now>
}

Otherwise once it's removed from the window it will start listening for *every* window changing key status.

>+  if ([mProgressController isSelected] && [[self window] isKeyWindow]) {
>     [[NSColor alternateSelectedControlColor] set];
>   }
>+  else if ([mProgressController isSelected] && ![[self window] isKeyWindow]) {
>+    [[NSColor secondarySelectedControlColor] set];
>+  }
>   else {
>     [[NSColor controlBackgroundColor] set];
>   }

This would be clearer as:

  if ([mProgressController isSelected]) {
    if ([[self window] isKeyWindow])
      [[NSColor alternateSelectedControlColor] set];
    else
      [[NSColor secondarySelectedControlColor] set];
  }
  else {
    [[NSColor controlBackgroundColor] set];
  }

>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowKeyStatusChanged:)
>+                                               name:NSWindowDidBecomeKeyNotification
>+                                             object:[[self view] window]];
>+  [[NSNotificationCenter defaultCenter] addObserver:self
>+                                           selector:@selector(windowKeyStatusChanged:)
>+                                               name:NSWindowDidResignKeyNotification
>+                                             object:[[self view] window]];

Unfortunately this is almost certainly wrong too, in that window is going to be nil at this point, so it's listening for *every* window changing. Sorry this didn't occur to me yesterday :(

I don't see a good place to add a working observer here, so I think we really do need to move the text color into the PV. It shouldn't really be that bad though after thinking about it more:
- Move the "// Cache the labels' unselected text color specified in IB" part into PV awakeFromNib (moving the member variable declarations in the header with it).
- Move refreshLabelColors into PV, replacing mIsSelected with a call to PVC's isSelected method.
- Add a public |- (void)selectionChanged| method to PV. Call it from PVC's setSelected: (where refreshLabelColors: is called now).
- Remove all existing calls to refreshLabelColors, and instead call it in these PV methods:
 1) didMoveToWindow, in the |if (window)| case you'll be adding above.
 2) the new selectionChanged
 3) the key window change callback you added.

Comment 11

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

Since the rabbit hole is getting deeper and a bunch of stuff is moving to PV, I pushed http://hg.mozilla.org/camino/rev/61152e46e091 to fix the -(foo) vs - (foo) situation there first.

Comment 12

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

(In reply to comment #11)
> Since the rabbit hole is getting deeper and a bunch of stuff is moving to PV

[6:50pm] sauron: smorgan: the kLabelTag enum constants are now used in both files
[6:50pm] smorgan: Hrm
[6:50pm] smorgan: What for in PVC?
[6:51pm] sauron: all over in refreshDownloadInfo
[6:51pm] sauron: to set all those values
[6:52pm] smorgan: This code is so horrible
[6:53pm] sauron: yes
[6:53pm] sauron: even i can tell that!
[6:53pm] smorgan: We have MVC! Just look at our filenames!
[6:53pm] sauron: that whole window is so horrible
[6:53pm] smorgan: So all the string setting should move into PV
[6:54pm] smorgan: PV should have three setters
[6:55pm] smorgan: Really we should be using bindings, but that's out of scope
[...]
[7:02pm] smorgan: (and really we should stop using tags, and add outlets to the view, but whatever; that doesn't have to happen now)

Comment 13

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

Attachment: More comprehensive fix

This purports to implement everything from Stuart's review comments plus everything we discussed in the channel (special thanks to a well-timed arrival in the channel by ilya to unwrangle a type mismatch for me that I missed and caused compile failures).

I've tested it again and everything seems to behave properly after all this surgery, but my head kinda hurts, so asking philippe to double-check my testing.

Comment 14

[philippe (part-time)]

Comment on attachment 503021 [details] [diff] [review]
More comprehensive fix

Purely from a functionality POV, this works correctly.

Comment 15

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

Comment on attachment 503021 [details] [diff] [review]
More comprehensive fix

Boy, my head really was off; I managed to mess up both of those requests :P That was supposed to be sr? not r?

Comment 16

[Stuart Morgan]

Comment on attachment 503021 [details] [diff] [review]
More comprehensive fix

sr=smorgan with a variety of nits.

>+- (void)selectionChanged
>+{
>+  [self refreshLabelColors];
>+}

Add a call to [self setNeedsDisplay:YES] here (since selection changing means the background changes).

>+  if (filenameLabelView) {
>+    if ([[filenameLabelView stringValue] isEqualToString:filename])
>+      return;
>+    
>+    [filenameLabelView setStringValue:filename];
>+    [filenameLabelView setNeedsDisplay:YES];
>+  }

Everything here is just a message to the object, so if it's nil nothing will happen. You can simplify by just removing the whole |if (filenameLabelView) check entirely. Same with all the other update methods.

>+    NSString* statusString;

NSString* statusString = nil;

(Doesn't matter with the current logic, but it's good practice.)

>+    NSString* statusString;
>+    statusString = [NSString stringWithFormat:NSLocalizedString(@"DownloadPausedStatusString", nil),

NSString* statusString = [...

>+      [[self class] formatBytes:mCurrentProgress],
>+      (mDownloadSize > 0 ? [[self class] formatBytes:mDownloadSize] : @"?")];

Indent these lines two more spaces each (I know we don't have a style rule for this, but I'm going to enforce the Google style ;) ).

>+    NSString* statusString;
>+    float byteSec = mCurrentProgress / elapsedTime;
>+
>+    statusString = [NSString stringWithFormat:NSLocalizedString(@"DownloadStatusString", nil),

float byteSec = ...
NSString* statusString = ...

(Don't declare variables separately when there's no need to.)

>+      [[self class] formatBytes:mCurrentProgress],
>+      (mDownloadSize > 0 ? [[self class] formatBytes:mDownloadSize] : @"?"),
>+      [[self class] formatBytes:byteSec]];

More indenting.

>+    if (mDownloadSize > 0) {
>+      int secToGo = (int)ceil((elapsedTime * mDownloadSize / mCurrentProgress) - elapsedTime);
>+    [curView updateTimeRemaining:[[self class] formatFuzzyTime:secToGo]];
>     }

Fix the third line's indenting.

>+    [[self view] selectionChanged];
>     [[self view] setNeedsDisplay:YES];

Remove the setNeedsDisplay:; that's the view's job now.

Comment 17

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

(In reply to comment #16)
> >+  if (filenameLabelView) {
> >+    if ([[filenameLabelView stringValue] isEqualToString:filename])
> >+      return;
> >+    
> >+    [filenameLabelView setStringValue:filename];
> >+    [filenameLabelView setNeedsDisplay:YES];
> >+  }
> 
> Everything here is just a message to the object, so if it's nil nothing will
> happen. You can simplify by just removing the whole |if (filenameLabelView)
> check entirely. Same with all the other update methods.

The old code in PVC needed them because of all the dancing around it was doing, or it was just bad code?

> >+      [[self class] formatBytes:mCurrentProgress],
> >+      (mDownloadSize > 0 ? [[self class] formatBytes:mDownloadSize] : @"?")];
> 
> Indent these lines two more spaces each (I know we don't have a style rule for
> this, but I'm going to enforce the Google style ;) ).

I did wonder how on earth those were supposed to be indented; the one example in the existing code (the statsuString in the |else| at the bottom of the |if (mDownloadDone)|) used just the two spaces, but I'm happy to switch to four and will also fix the place I copied :)

Comment 18

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

http://hg.mozilla.org/camino/rev/444d5832a2b1

Thanks for all the help!

(In reply to comment #12)
> [7:02pm] smorgan: (and really we should stop using tags, and add outlets to the
> view, but whatever; that doesn't have to happen now)

Bug 624978 filed on that.