Adium

Opened 12 years ago

Closed 12 years ago

Last modified 11 years ago

#9502 closed defect (fixed)

Status Menu Item Offcenter

Reported by: hortont424 Owned by: nobody
Milestone: Adium 1.3 Component: Adium UI
Version: Severity: minor
Keywords: Cc:
Patch Status: Accepted

Description

In current builds of Adium, the status menu item is not horizontally centered, leading to a confusing differences in spacing in the user's menu bar (Apple's all seem to be reasonably consistent). See the attached PDF for an example, using both the Adiumy and iChat 4 status menu themes. The attached patch fixes this bug.

Attachments (2)

statusmenu-problem.pdf (18.4 KB) - added by Tim Horton 12 years ago.
An illustration of the bug
statusitemwidth.diff (1.2 KB) - added by Tim Horton 12 years ago.
Fixes status item width, now much much shorter!

Download all attachments as: .zip

Change History (24)

Changed 12 years ago by Tim Horton

Attachment: statusmenu-problem.pdf added

An illustration of the bug

comment:1 Changed 12 years ago by Jordan

Milestone: Adium X 1.3

comment:2 Changed 12 years ago by Jordan

Unless we need that space for overlays... like the number of unread messages?

comment:3 Changed 12 years ago by Tim Horton

You're right. I'll fix that and post another diff in a day or two, because this has been bugging me for a while.

comment:4 Changed 12 years ago by Evan Schoenberg

Patch Status: Initially IncludedNeeds Changes by Author

comment:5 Changed 12 years ago by Evan Schoenberg

Good progress. Some comments:

  1. Why are we returning a string and then doing comparisons to determine what that string was (getStatusImageName)? This should be an enum, and the result should be analyzed using a switch statement.
  2. Cocoa accessors don't typically start with 'get'; calling the method implicitly is a request to get the value. Rather than getStatusImageName I would use statusImageName or perhaps currentStatusImageName (though of course given (1) above, the name should change more than that).
  3. String comparisons should never be done with a pointer comparison (==); if it works properly, it's pure coincidence. -[NSString isEqualToSring:], -[NSString compare:], or -[NSString caseInsensitiveCompare:] should be used as appropriate.

comment:6 Changed 12 years ago by Evan Schoenberg

I meant, of course, -[NSString isEqualToString:]

comment:7 Changed 12 years ago by Tim Horton

Got it! How's this? I'm really not sure what's the best way to go about doing this, duplicating the least amount of code, especially for such a small fix...

comment:8 Changed 12 years ago by Zachary West

You can use NSStatusItem's method -(NSImage *)image to image that's already assigned, eliminating the rewrite of the other sections.

comment:9 Changed 12 years ago by Zachary West

Rather, to get the image that's already assigned.

Changed 12 years ago by Tim Horton

Attachment: statusitemwidth.diff added

Fixes status item width, now much much shorter!

comment:10 Changed 12 years ago by Tim Horton

That massively simplifies things - try this patch! Seems to work for me.

comment:11 Changed 12 years ago by Zachary West

Resolution: fixed
Status: newclosed

(In [23000]) Patch from hortont424 which toggls between a set and varied status item width when necessary. Fixes #9502.

comment:12 Changed 12 years ago by Robert

Patch Status: Needs Changes by AuthorAccepted

comment:13 Changed 12 years ago by Robert

priority: normallowest
Severity: normaltrivial

comment:14 Changed 12 years ago by Zachary West

(In [23001]) Forgot a case for [23000], when the image changes width and the icon needs to be updated. Refs #9502.

comment:15 Changed 12 years ago by Zachary West

(In [23002]) Which means these cases from [23000] are redundant. Refs #9502.

comment:16 Changed 12 years ago by Peter Hosey

The nature of the change, and of the code before, suggests that this is a bug in Mac OS X. Has anyone filed this?

comment:17 Changed 12 years ago by Tim Horton

I agree. I also fixed the same bug in MarcoPolo, so I'm pretty sure it's a Mac OS X bug. I'll go file it right now and see.

comment:18 Changed 12 years ago by Peter Hosey

hortont424: What's the bug number? I'd like to reference it in a comment in the relevant code.

comment:19 Changed 12 years ago by Tim Horton

rdar://problem/5829508

comment:20 Changed 12 years ago by Peter Hosey

(In [23003]) Added comment describing why we're doing this computation and linking to the Radar report for the Mac OS X bug that makes it necessary. Refs #9502.

comment:21 Changed 12 years ago by Peter Hosey

(In [23004]) Changed the length of the status item with no title from (image width + margin) to NSSquareStatusItemLength. This removes the need for a constant which had been dead, and hortont424's patch (aka r23000) reanimated. Refs #9502.

comment:22 Changed 12 years ago by Peter Hosey

(In [23005]) Reverted r23004, after Zac pointed out that not all status item icon themes are necessarily square. Refs #9502.

Note: See TracTickets for help on using tickets.