Adium

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#5129 closed enhancement (fixed)

Line underneath "All" in log viewer

Reported by: tick Owned by: pleasereassign
Milestone: Adium X 1.1 Component: Adium UI
Version: Severity: normal
Keywords: Cc:
Patch Status: Accepted

Description (last modified by Chris Forsythe)

This is for usability.

In the provided mockup there is a line underneath the "All" entry in the sidebar. Something like that.

Attachments (6)

sidebarmockup.png (89.9 KB) - added by Chris Forsythe 13 years ago.
ticket5129-fix.diff (2.7 KB) - added by Christopher Harms 13 years ago.
Proposed patch for this.
ticket5129-fix-rev1.diff (13.1 KB) - added by Christopher Harms 13 years ago.
Revision 1 of Patch for #5129.
ticket5129-fix-rev1-logviewer.tar.gz (14.5 KB) - added by Christopher Harms 13 years ago.
Patched LogViewer.nib for #5129
ticket5129-fix-rev2.diff (13.1 KB) - added by Christopher Harms 13 years ago.
2nd revision for #5129. You still need the nib-file from rev2.
ticket5129-fix-rev3.diff (13.3 KB) - added by Christopher Harms 13 years ago.
As announced in the mailiglist: switching back to the category, some changes to AIDividerPosition.

Download all attachments as: .zip

Change History (21)

Changed 13 years ago by Chris Forsythe

Attachment: sidebarmockup.png added

comment:1 Changed 13 years ago by Chris Forsythe

Milestone: Adium X 1.0
Owner: changed from nobody to pleasereassign

comment:2 Changed 13 years ago by Chris Forsythe

Component: NoneAdium UI
Description: modified (diff)
Milestone: Adium X 1.0Sometime after 1.0

comment:3 Changed 13 years ago by Christopher Harms

Patch coming via diff-file. See there... I've implemented a protocol to AIAlternatingRowOutlineView.h. Actually, I don't think this is really necessary, but I don't see a reason why it shouldn't be there. Even, if there is no -outlineView:hasItemDividerBelow: defined it should work. Don't hit me, if it doesn't, but I'm quite sure.

Changed 13 years ago by Christopher Harms

Attachment: ticket5129-fix.diff added

Proposed patch for this.

comment:4 in reply to:  3 Changed 13 years ago by Christopher Harms

Replying to CalganX:

Patch coming via diff-file. See there...

Something I've forgotten: the color is ugly, I admit. But I'm no designer and I used a color which seems to be useful for this. I wanted to use the blue given in the mockup, but I didn't find it. So, if you have a better idea for the color, just say it. It should not be very difficult to change that.

comment:5 Changed 13 years ago by Eric Richie

field_haspatch: 01
Patch Status: Needs Dev Review
Severity: blockernormal

comment:6 Changed 13 years ago by Evan Schoenberg

Patch Status: Needs Dev ReviewNeeds Changes by Author

This is a good start. A couple comments:

  1. Rather than defining a protocol, you should add an interface category on NSObject. Something like:
    @interface NSObject (AIOutlineView_ItemDivider)
    - (BOOL)outlineView:(NSOutlineView*)outlineView hasItemDividerBelow:(id)item; ;
    @end
    
  2. I don't like having every drawing operation on every outline view incur the added expense used only in this one place. How about making a subclass of AIAlternatingRowOutlineView which adds the query-and-draw code you have here, then using that subclass in the log viewer specifically?

comment:7 Changed 13 years ago by Christopher Harms

Thanks, Evan. I'm going to change that. Patch incoming later that day.

Changed 13 years ago by Christopher Harms

Attachment: ticket5129-fix-rev1.diff added

Revision 1 of Patch for #5129.

Changed 13 years ago by Christopher Harms

Patched LogViewer.nib for #5129

comment:8 Changed 13 years ago by Christopher Harms

There you go. I added some more methods to make it more universal (you can also add a divider above and create an item as a divider). Further comments appreciated.

comment:9 Changed 13 years ago by Evan Schoenberg

Duplicated code should be avoided; in the current implementation, you should define a method or function which handles the thrice-duplicated drawing code which has only minor differences. However, I'd change this in a different way:

Rather than make 3 calls to the delegate, checking each if the delegate implements it, how about this: add a 4-value enum to the header, for no-divider/is-divider/divider-above/divider-below. Query the delegate with a single method (outlineView:dividerTypeForItem:). React accordingly.

comment:10 Changed 13 years ago by Christopher Harms

Sounds better. But what to do if you want a divider above *and* below? At least two times you have to set up your graphics context. That was the reason I didn't removed the duplicated code.

Changed 13 years ago by Christopher Harms

Attachment: ticket5129-fix-rev2.diff added

2nd revision for #5129. You still need the nib-file from rev2.

comment:11 Changed 13 years ago by Christopher Harms

Changed what you've mentioned, but reimplemented the protocol. I've sent you an Email about this. I think this is the common way to do what I wanted. But, I'd suggest we clear this via mail.

comment:12 in reply to:  10 Changed 13 years ago by Evan Schoenberg

Replying to CalganX:

Sounds better. But what to do if you want a divider above *and* below? At least two times you have to set up your graphics context. That was the reason I didn't removed the duplicated code.

So that's another option in your enum... and why do you have to set up multiple graphics contexts? I'm not clear on why we're saving/restoring graphics contexts at all. For two lines, make a single NSBezierPath, use moveToPoint and lineToPoint twice each, then stroke it.

I'll reply to your email this afternoon.

comment:13 Changed 13 years ago by Christopher Harms

Forget the thing about the two lines. I don't think this will ever be necessary. At last, I draw and set up the context only one time. Was kind of a blockhead tomorrow about this. I think it should be a better code now, as before - at least I hope so.

Changed 13 years ago by Christopher Harms

Attachment: ticket5129-fix-rev3.diff added

As announced in the mailiglist: switching back to the category, some changes to AIDividerPosition.

comment:14 Changed 13 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(In [19715]) Patch from Christopher Harms which adds a new AIAlternatingRowOutlineView subclass, AIDividedAlternatingRowOutlineView, which can display a divider line above or below an item, and uses that class to put a line below the "All" item at the top of the transcript viewer source list. Good work!

Fixes #5129.

comment:15 Changed 13 years ago by Evan Schoenberg

Milestone: Good idea for "later"Adium X 1.1
Patch Status: Needs Changes by AuthorAccepted
Version: 1.1svn
Note: See TracTickets for help on using tickets.