391738 - Feed icon changes don't update location bar padding correctly

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Stuart Morgan]

I almost always see incorrect padding around the feed icon (no space at all between the left edge and the text) when it appears on top of a URL in the location bar, but anything that causes a redraw (clicking in the bar, resizing the window, etc.) fixes it. It feels like we are changing some internal sizing but not calling setNeedsDisplay:.

Comment 1

[Christopher Henderson]

Attachment: Fix v1.0

You're right, it does need a setNeedsDisplay: call or two. Here's a patch.

Comment 2

[Stuart Morgan]

Comment on attachment 337186 [details] [diff] [review]
Fix v1.0

> -(void)positionFeedIcon
> [...]
>+  [self setNeedsDisplay:YES];

This one should be in displayFeedIcon:

In both cases, put the setNeedsDisplay: inside each of the if and else blocks, since when neither happens we don't need to display.

(Just go ahead and request sr from me for the next round)

Comment 3

[Christopher Henderson]

Attachment: Fix v1.1

Fix incorporating reviewer comments.

Comment 4

[Stuart Morgan]

Comment on attachment 337245 [details] [diff] [review]
Fix v1.1

>+    [self setNeedsDisplay:YES];
>     
>     // If the feed icon needs to be displayed, and it was drawn before the secure icon, move the 
>     // feed icon to the left hand side of the secure icon.
>     if ([[self cell] isFeedIconDisplayed])
>       [self positionFeedIcon];

setNeedsDisplay should be called after all the changes that require a display.

Also, we should be calling setNeedsDisplay in the hiding cases as well; either changes requires re-displaying.

Comment 5

[Christopher Henderson]

Attachment: Fix v1.2

Should have setNeedsDisplay in all the right places.

Comment 6

[Stuart Morgan]

Comment on attachment 337260 [details] [diff] [review]
Fix v1.2

Looks good, sr=smorgan

Comment 7

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

Landed on cvs trunk.