Adium

Opened 15 years ago

Closed 15 years ago

Last modified 11 years ago

#344 closed defect (fixed)

Need visual notification of blocked users

Reported by: Ludge Owned by: Kiel
Milestone: Adium X 1.0 Component: Adium Core
Version: Severity: normal
Keywords: block display icon Cc:
Patch Status:

Description

Now that the blocking works OK, it would be good if we could have some way of seeing which buddies are blocked within the contact list. Currently if you want to see if a user is blocked, you have to right click on their name and see if the menu says "unblock" or not. I think a new status icon would probably be best, something like the normal icon but with a crossed circle on it (like this character: Ø). Also, having their display name as a different colour or strikethrough text could work.

Attachments (7)

KGContactBlockedPlugin.diff (8.7 KB) - added by Kiel Gillard 15 years ago.
The proposed plugin, version 1.0
cl-strikeout-test.html (1.4 KB) - added by Peter Hosey 15 years ago.
demonstration of strike-out in various colour combinations. I don't think it looks so bad.
potentialSolution.png (6.1 KB) - added by Kiel Gillard 15 years ago.
Mockup of a potential solution to this ticket (envision this as a row on the contact list).
blockedContactsAlpha.diff (12.9 KB) - added by Kiel Gillard 15 years ago.
Experimental patch... please let me know what you think :-)
KGContactBlockedPlugin2.diff (48.3 KB) - added by Kiel Gillard 15 years ago.
Potential patch to close ticket. Please extensively review!
KielBlocking.patch (38.9 KB) - added by Kiel Gillard 15 years ago.
cbarrett's tamed svk version of my patch…
KGBlockedContacts3.diff (46.3 KB) - added by Kiel Gillard 15 years ago.
Untamed version of cbarrett's patch.

Download all attachments as: .zip

Change History (58)

comment:1 Changed 15 years ago by David Smith

Milestone: Adium X 0.82Adium X 0.90
Severity: majornormal

I like the idea, but I can't see it happening for 0.82. Pushing back to 0.9 for now until we decide what to do with it. For future reference, please let us set the severity and milestone.

comment:2 Changed 15 years ago by Augie Fackler

Milestone: Adium X 0.90 (Old)Adium X 0.90

comment:3 Changed 15 years ago by Kiel Gillard

I've toyed around with having tooltips display whether a contact is blocked or not. This is probably not enough to address the needs of this ticket, but it's a start - until a better solution reveals itself.

I've attached a .diff file which contains a plugin that I wrote to add a "Blocked: Yes" to the body of a tooltip of blocked contacts. It should be noted that this .diff file adds the KGContactBlockedPlugin to the CoreComponents.plist file.

Please let me know what you think :-)

Changed 15 years ago by Kiel Gillard

Attachment: KGContactBlockedPlugin.diff added

The proposed plugin, version 1.0

comment:4 Changed 15 years ago by Peter Hosey

haven't tried the component, but what I suggest is displaying the contact in strike-out type, and taking out the ': Yes' if possible (leaving just "Blocked").

comment:5 Changed 15 years ago by Kiel Gillard

I personally think that striking out type would look bad... and make it difficult to read the contact's screen name and/or UID.

I'm all for making the tooltip say "Blocked" instead of "Blocked: Yes", but I was attempting to keep consistent with similar features of Adium - for example, if a contact is Away, the tooltip says "Away: Yes" (this may be only for the Messenger protocol, but this is how it appears). Also, the InterfaceController requires a valid entry to each label - I guess I could get away with @" " but that would leave the tooltip saying "Blocked: ". Maybe we should see what others say...

Thanks for your feedback, looking forward to some more :-)

comment:6 Changed 15 years ago by Peter Hosey

maybe make the InterfaceController omit the ': ' when the value is an empty string. the same could be then done with 'Away'.

Changed 15 years ago by Peter Hosey

Attachment: cl-strikeout-test.html added

demonstration of strike-out in various colour combinations. I don't think it looks so bad.

comment:7 Changed 15 years ago by Kiel Gillard

The InterfaceController could omit the colon when the label's entry is an empty string, but it might be better to have Adium say "Status: Away" instead. While I'm at it (and this is probably something for another ticket), what do you think of changing the tooltips altogether, so it looks more like:

<image> <screen name> (Blocked) (if applicable)
-seperator-
Service: <service>
Status: <status> (if not available)
User ID: <formattedUID> (if not different to screen name)

As for the strike out test, I personally think it looks ugly :-P I mean, the black strike thru with a dark background colour looks crap. I'd favour a little block icon for a blocked contact on the contact list (if you email me at kgilla10@…, I'll send you a mockup of how this might look - I sent one to Evan today to get his feedback).

Changed 15 years ago by Kiel Gillard

Attachment: potentialSolution.png added

Mockup of a potential solution to this ticket (envision this as a row on the contact list).

comment:8 Changed 15 years ago by Kiel Gillard

Eh, I attached the mockup anyway, just to get some feedback from others who stumble upon this ticket. It looks bad because this is a screen grab of Preview sitting behind my shadowless Adium contact list, so it isn't aligned properly and things... but hopefully you get the idea.

comment:9 Changed 15 years ago by David Smith

Neat idea..., though I rather like the reporter's idea of having it be a status icon (perhaps because the status gems are the only non-contact-image icons on my buddy list :) ). I tried to implement strikethrough a while back, in a kinda halfhearted way. I forget what ended up stopping me.

comment:10 Changed 15 years ago by Evan Schoenberg

I like Status: Away like better than Away: Yes.

I'm not sure about adding (Blocked) after the name... it can already get really long: This Is My Long Display Name (evan.s@…) (MSN) (Blocked)' would be crazy.

Blocked: Yes makes some degree of sense. I suggest the following changes to the patch (which I think is appropriate even with the status icon thing implemented, as not all lists use status icons and not all status icon packs have a blocked icon):

  • Make it check first for whether the object is an AIListContact
  • If yes, check that contact's account. If no, just break.

However, how to handle an AIMetaContact also needs to be resolved.

comment:11 Changed 15 years ago by Kiel Gillard

Catfish_man, I feel that having a status icon for a blocked buddy is problematic because then a user would not be able to tell what the blocked contact's real status is. To work around this, you have a blocked variant of each status but that means twice the work for the people that create status packs. If I was to implement a little block icon, I would design the solution to use the block icon of a status pack and enable the user to choose if it is shown or not, so you don't need to worry about it taking up space on your contact list :-)

Evan, I agree with your statement that the title of the tooltips could get really long, but I suggested that the tooltips be re-designed to the following format:

<image> <screen name> (Blocked) (if applicable)
-seperator-
Service: <service>
Status: <status> (if not available)
User ID: <formattedUID> (if not different to screen name)

So, given your example, the tooltip would look like:
<image> This is my really long display name (Blocked)
-seperator-
Service: MSN
Status: Away
User ID: evan.s@…

I will take your suggestions to heart and look into these issues :-)

comment:12 Changed 15 years ago by Evan Schoenberg

(In [13366]) Contacts who are away without an away message now show Status: Away rather than Away: Yes in their tooltips. Refs #344 as it results from discussion there.

comment:13 Changed 15 years ago by Kiel Gillard

Ok, this one took a good study break :-)

Attached is a patch which incorporates a few interesting and serious changes:
1) AIListContact now responds to the methods -(BOOL)isBlocked and -(void)setIsBlocked:(BOOL)yesOrNo, using the statusObjectForKey: methods.
2) CBGaimAccount methods that add and remove contacts from privacy lists now instruct the AIListContact instances to store whether or not they're blocked. According to durin42, I think I've used the wrong methods...
3) ESMetaContentsContactPlugin (I think that it's name from the top of my head) has been changed to append @" (Blocked)" if one or more (but not all) of the Meta's contacts are blocked.
4) The KGContactBlockedPlugin creates a secondary tooltip entry which says "Blocked: Yes" (for the time being) of ListContacts or Metas whose contacts are all blocked.

Please be aware that this plugin is added to the CoreComponents plist.

Patch attached to get feedback from the devs.

Changed 15 years ago by Kiel Gillard

Attachment: blockedContactsAlpha.diff added

Experimental patch... please let me know what you think :-)

comment:14 Changed 15 years ago by Kiel Gillard

I noticed a contact on my contact list had the secondary tooltip entry "Idle: Yes". Perhaps this should be changed to "Status: Idle" as well? Or perhaps for contacts who are both away and idle (is that even at all possible?) "Status: Away, Idle".

comment:15 Changed 15 years ago by David Smith

field_haspatch: 1

comment:16 Changed 15 years ago by Colin Barrett

field_haspatch: 10

Kiel: If you want to tackle the Status: Idle change, that'd be neat. It's probably a fairly simple change, basically just looking to see how the Away plugin does things, and porting that over to the Idle plugin.

If we had a "Good for patchwriters" flag, I'd hit set it.

comment:17 Changed 15 years ago by Colin Barrett

field_haspatch: 01

why'd it delete the flag?

comment:18 Changed 15 years ago by Kiel Gillard

I think I can tackle the "Status: Idle" change, but I need to know one thing: can a contact on any service be Away AND Idle? How about metas? Or should I code the solution to take into consideration the possibility that a contact/meta can be both Away and Idle just for the principle's sake?

I'll give this a bit more thought, my instincts tell me there's a better solution for this...

Aside: Currently I'm working on this ticket and (shortly) #1117. Is there any way I can assign myself to these tickets so nobody else does the work I do?

comment:19 Changed 15 years ago by Evan Schoenberg

field_haspatch: 10
Owner: changed from anybody to Kiel Gillard

comment:20 Changed 15 years ago by Kiel Gillard

Cheers :-)

Changed 15 years ago by Kiel Gillard

Potential patch to close ticket. Please extensively review!

comment:21 Changed 15 years ago by Kiel Gillard

Ok, it's been a while in the making (no thanks to uni life) but here it is - a massive patch.

I need the devs to pay particular attention to the small changes I've made to CBGaimAccount (the privacy methods). Augie suggested these were the wrong places to put them and I can't remember why I disagreed with him. I think the other methods weren't called at startup.

Included in this patch is 2 plug ins for organising tooltips so they're not insanely long (as they could get).

Everything else should be fairly self explanatory - pretty sure I met the coding guidelines but if I didn't then tell me to try again!

comment:22 Changed 15 years ago by Kiel Gillard

I forgot to mention, if this patch is committed, status packs need to start using a Blocked icon. Which means the default status packs need to be updated. Included in this patch is an updated Gems status icons pack with a blocked icon from Catfish_Man. People can use the Gems pack as an example to upgrade their packs. No difficult change, just another key and value to implement.

Also, localised variants of the ListLayout nibs need to be updated to sport a "Show Blocked Icons" check box and complementary popup menu. This checkbox and menu needs to be connected to the File's Owner like all the other GUI widgets.

comment:23 Changed 15 years ago by Colin Barrett

field_haspatch: 01

comment:24 Changed 15 years ago by Colin Barrett

Owner: changed from Kiel Gillard to Colin Barrett

comment:25 Changed 15 years ago by Colin Barrett

field_haspatch: 10
Status: newassigned

assiging this to me (since I'm reviewing the patch), and accepting it.

comment:26 Changed 15 years ago by Colin Barrett

field_haspatch: 01

damn has patch.

comment:27 Changed 15 years ago by Kiel Gillard

The patch needs a minor update (have recently discovered the wonders of svn st). Once svn is fixed, the best place to call setIsBlocked: on any subclass of AIListContact has been determined and I've finished ticket #1117 (which I hope will be soon), I'll attach an updated patch. Therefore, an updated patch will also include code to close ticket #1117.

cbarrett, can you please contact me with any other issues you may have with my patch for this ticket which could correct in an update. I welcome feedback from other people, too. My email address is on #1117.

comment:28 Changed 15 years ago by Colin Barrett

field_haspatch: 10

My only complaints about this are that I believe you are duplicating some code in the blocking plugin with this new method. Take a look at the functions for blocking and unblocking there, see if those are of any use.

comment:29 Changed 15 years ago by Colin Barrett

field_haspatch: 01

argh, stupid has patch. Evan, could you reverse apply that diff? The template for the ticket page is expecting different input, and it's not coming (I haven't had time to set up a trac testbed. 24 hours in the day :\)

comment:30 Changed 15 years ago by Kiel Gillard

Where in the patch attached to this ticket do I enumerate through accounts of a particular service, check to see if the current account conforms to a privacy protocol and if it does add or remove the contact of that account to/from the privacy list? *scratches head*

The only major problem with this patch is the fact that I (and nobody else, apparently) knows where the proper place is to send an AIListContact instance the setIsBlocked: message.

Minor problems to the patch include the fact that I hadn't reverted files that I had changed to compile a revision I had download from the svn repository, as well as a dodgy bit of code that I swear I had removed.

comment:31 Changed 15 years ago by Colin Barrett

field_haspatch: 10

My initial reaction would be that you'd just want to check the privacy lists, rather than bothering with a new method to AIListContact. But since you've got it there, look in the blocking plugin. Wherever the same actions as setIsBlocked are performed (i.e. adding to the privacy lists), then replace it with setIsBlocked. I would still probably advise you to just smoke the privacy lists straight at this point. A better privacy API is coming from Gaim soon.

Also, why are there extra files added, like the "service: " tooltip? Do these relate to the ticket in some way I've missed? Note that I'm not being hostile and saying IT SUX. On the contrary, I think your code for the actual drawing and adding the icon was well hacked. You just got a little overzealous with abstraction :)

comment:32 Changed 15 years ago by Kiel Gillard

The whole reason there's an isBlocked status object is because IMHO it is a much more efficient way of determining if a contact is blocked, rather than calling a method that loops through all the accounts of a service kind for an account to look for a UID. This could add up to a sizeable amount of processing time if you're wondering if every contact on your contact list (potentially 100+) is blocked.

However, your idea of AIListContacts blocking themselves (I think that's what you implied) is a good one. I'll look into that.

The "Service: " tooltip is relevant to this ticket because if you have a look at the discussion about half way through this page (in particular a comment from Evan), the title of tooltips could get crazy long, particularly on MSN. It's fine if you think it sucks, personally I thought the way it is in 0.86 sucks too lol. Having them in a secondary position made much more sense. Something had to be done to address the issue. Same applies with the UID.

I'll wait until Evan has updated libgaim to 2.0.0 until I think about the better privacy API in Gaim.

comment:33 Changed 15 years ago by Evan Schoenberg

svn now has libgaim 2.0.0, such as it is -- I don't think the privacy API there changed :\

comment:34 Changed 15 years ago by David Smith

I just wanted to say that I highly approve of having list contacts know whether they're blocked (even if it's internally just iterating over the privacy list, I like the API).

comment:35 Changed 15 years ago by Colin Barrett

I agree. I was mostly just complaining that this introduces an inconsistency. The blocking plugin does not use this method, and other areas do. The correct time to call setIsBlocked: is in the blocking plugin, when they select the blocking menu.

I didn't do this whole thing at first because I wanted to get rudimentary blocking support working and done. First make it work, etc...

The privacy API is something that's slated for further down the line, but it is coming. It may or may not be in 2.0

comment:36 Changed 15 years ago by Evan Schoenberg

"The correct time to call setIsBlocked: is in the blocking plugin, when they select the blocking menu."

I would think it'd be in account code when we find out that the contact is blocked.

comment:37 Changed 15 years ago by Colin Barrett

Oh, duh. :) There too :D

Changed 15 years ago by Kiel Gillard

Attachment: KielBlocking.patch added

cbarrett's tamed svk version of my patch...

comment:38 Changed 15 years ago by Kiel Gillard

Ok, I've attached cbarrett's 'tamed' version of my patch. Why did it need to be tamed? Because I'd originally made some changes that are both controversial and slightly beyond the scope of my patch (that is, re-arranging information on tooltips). Tooltips are not going to provide a visual notification of blocked contacts until we reach some sort of agreement about the layout of tooltips. I will send an email to the list shortly detailing the problem and encouraging discussion about the whole issue.

Please let me know what you think about the patch :-)

Changed 15 years ago by Kiel Gillard

Attachment: KGBlockedContacts3.diff added

Untamed version of cbarrett's patch.

comment:39 Changed 15 years ago by Evan Schoenberg

What's the status of this patch, Colin?

comment:40 Changed 15 years ago by Kiel Gillard

Augie and I are working on it. I've been away for a couple of days but I'll try and wrap this up soon (Augie doesn't seem to be online much, when I'm online at least).

comment:41 Changed 15 years ago by sebhelyesfarku@…

What about having a Show/Hide Blocked Contacts item in the View menu? Good idea, isn't it?

comment:42 Changed 15 years ago by Peter Hosey

hmmm. '(Show|Hide) Blocked Contacts'.

I like it.

comment:43 Changed 15 years ago by Colin Barrett

Owner: changed from Colin Barrett to Augie Fackler
Status: assignednew

this has been passed off to Augie.

comment:44 Changed 15 years ago by Augie Fackler

Resolution: invalid
Status: newclosed

Killing this as per IRC discussion.

comment:45 Changed 15 years ago by anonymous

"Killing this as per IRC discussion."

Occasional visitors like me have absolutely no idea of what the IRC discussion was and why this ticket was flagged invalid.

comment:46 Changed 15 years ago by Chris Forsythe

You will when we release 1.0. This stays closed.

comment:47 Changed 15 years ago by David Smith

Milestone: Adium X 1.0

comment:48 Changed 15 years ago by Evan Schoenberg

Resolution: invalid
Status: closedreopened

comment:49 Changed 15 years ago by Evan Schoenberg

Milestone: Adium X 1.0
Owner: changed from Augie Fackler to Kiel
Status: reopenednew

The end discussion of this patch makes no sense to me, so I'm ignoring it. The original purpose of this ticket was "need visual notification of blocked users," and that's a valid enhancement request. Furthermore, it's one which Kiel has no fulfilled. :)

comment:50 Changed 15 years ago by Evan Schoenberg

Resolution: fixed
Status: newclosed

(In [15210]) Patch from Kiel Gillard which implements a "blocked" status icon. There is a default blocked icon which will be used if the status icon set does not supply one, which it can do with a "Blocked" key. Fixes #344. Good work! :)

The patch also improves the the blocking API a bit, allowing contacts to block themselves.

comment:51 Changed 11 years ago by Zachary West

Component: Core AdiumAdium Core
Note: See TracTickets for help on using tickets.