Bug 292882 - JJ: Add "Add Contact" button to the presence applet
Summary: JJ: Add "Add Contact" button to the presence applet
Status: RESOLVED FIXED
Alias: None
Product: telepathy
Classification: Unmaintained
Component: presence-applet (show other bugs)
Version: git-latest
Platform: Unlisted Binaries Linux
: NOR wishlist
Target Milestone: 0.4.0
Assignee: Mikhail Krishtop
URL:
Keywords: junior-jobs
Depends on:
Blocks:
 
Reported: 2012-01-30 17:00 UTC by Daniele E. Domenichelli
Modified: 2012-07-06 12:47 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
first patch (8.09 KB, patch)
2012-03-23 00:34 UTC, Mikhail Krishtop
Details
second patch (15.38 KB, patch)
2012-03-23 00:35 UTC, Mikhail Krishtop
Details
ktp-contact-list patch 1 (1.97 KB, patch)
2012-03-23 14:19 UTC, Mikhail Krishtop
Details
ktp-contact-list patch 2 (8.03 KB, patch)
2012-03-23 14:20 UTC, Mikhail Krishtop
Details
ktp-presence-applet patch (9.01 KB, patch)
2012-03-23 14:21 UTC, Mikhail Krishtop
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniele E. Domenichelli 2012-01-30 17:00:51 UTC
Looking at kopete tray icon right click menu [1], makes me think that the presence applet could be a lot more useful

Some features that in my opinion should be added:

1) Add Contact
2) Join Group Chat


We could also (even if I'm not really sure about this) add a sub menu for each account to set the status and more.

[1]http://wstaw.org/m/2012/01/30/snapshot45.png
Comment 1 David Edmundson 2012-02-03 13:31:56 UTC
We should add features because they're useful and you find yourself wanting to have this feature NEVER simply because Kopete has them.

I definitely don't think we need everything in that Kopete screenshot. 

With regards to the two items explicitly mentioned, I like the idea of doing them from directly. The plasma contact list /may/ be the more logical place once that gets going. I'm not sure. 

It might be worth doing them as individual bug reports. This current bug is too vague to ever be closed as "Fixed", because there's always more features that could be added.
Comment 2 Daniele E. Domenichelli 2012-02-03 13:49:57 UTC
I think it is worth to have them in the presence applet, since (at least for the 2 items mentioned) doesn't completely fit the contact list, since they are not actions on the contacts (it is probably worth to have them in both places)

If you want to join a chat room it might not have sense to start the contact list, same for adding a contact (for example, I receive an e-mail with a contact that I just want to add, but I'm not interested in chatting with him right now).

Anyway I'm changing this bug to Add "Add Contact" button to the presence applet,
and I'll clone this for the "Join chat room"
Comment 3 Daniele E. Domenichelli 2012-03-19 17:25:28 UTC
Marking as a junior job since this can be easily done, we just need to move  the add contact dialog in ktp-common-internals/Widgets and then use it in contact list and in the presence applet...
Comment 4 Mikhail Krishtop 2012-03-23 00:34:21 UTC
Created attachment 69824 [details]
first patch
Comment 5 Mikhail Krishtop 2012-03-23 00:35:09 UTC
Created attachment 69825 [details]
second patch
Comment 6 Mikhail Krishtop 2012-03-23 00:44:39 UTC
Begin work on this bug.

Patch one move the add contact dialog from ktp-contact-list to ktp-common-internals\Widgets.
Patch two rename the add contact dialog as add contact widget. (not sure that it should be)

Can you say me the right name for this class?

Mikhail
Comment 7 David Edmundson 2012-03-23 01:04:54 UTC
I don't understand why you've renamed it from Dialog to Widget. 

Especially when the class inherits from KDialog.
Comment 8 Mikhail Krishtop 2012-03-23 11:32:20 UTC
Oops. I'm confused that contact grid widget is placed in ktp-common-internals/Widgets. He is named so because he inherits from QWidget, not because he placed in widgets library. I understood my mistake. 

Just ignore second patch.

Mikhail
Comment 9 Mikhail Krishtop 2012-03-23 14:19:10 UTC
Created attachment 69830 [details]
ktp-contact-list patch 1

Updating ktp-contact-list main widget to use ktp-common-internals/Widgets add contact dialog instead of his own add contact dialog.
Comment 10 Mikhail Krishtop 2012-03-23 14:20:07 UTC
Created attachment 69831 [details]
ktp-contact-list patch 2

Removing ktp-contact-list/dialogs add contact dialog since ktp-contact-list isn't used it.
Comment 11 Mikhail Krishtop 2012-03-23 14:21:35 UTC
Created attachment 69832 [details]
ktp-presence-applet patch

Adding ktp-common-internals/Widgets add contact dialod button.

P.S.: It's okey how I'm posting patches or I should use git post-review command in future instead?
Comment 12 Daniele E. Domenichelli 2012-03-23 17:25:25 UTC
(In reply to comment #11)
> P.S.: It's okey how I'm posting patches or I should use git post-review
> command in future instead?

Reviewing patch on bugzilla is not easy and boring, we have to download the patch, apply it, make a lot of copy and paste to point you at the problems with the patch. And programmers are usually lazy.

If you want your patch reviewed you should use reviewboard (read this blog post by david http://www.sharpley.org.uk/node/30 )

For complicated patches another option to allow us to review the single commits is to have you git repository clone, push your changes there and add to the reviewboard your git repository url (if you have a kde identity account see http://community.kde.org/Sysadmin/GitKdeOrgManual#Personal_clones_of_project_repositories )

If you are working on this bug you should also assign this to yourself, and mark it as assigned, so that the others know that you are doing this and won't do it :)
Actually we usually forget about this, but we shouldn't ;)
Comment 13 David Edmundson 2012-03-27 17:09:46 UTC
in the presence applet you've called the variable "m_addContactAction"

we use "m_" to say "member variable". This isn't one. 

In your patch the presence applet now has an accounts model, which means the applet keeps track of every contacts name, avatar, capabilities etc.. all the time and it doesn't ever show them.

Create it only when you show the add-contact dialog, and that way it won't be in memory and updating constantly.

You've changed the AccountFactory, ContactFactory etc? Why? I don't mind if there's a good reason, but doing it without one adds a lot of dbus overhead and I'm pretty sure there's no need for it.

(Long term discussion for everyone else, we should make a new model just for accounts (i.e the one the accounts kcm uses) and use that in these dialogs. It'll need doing for the port to nepomuk models anyway)
Comment 14 Daniele E. Domenichelli 2012-03-28 07:06:26 UTC
Since this comment is useless for the patch I'll use this to assign the bug to Mikhail :)

(In reply to comment #13)
> (Long term discussion for everyone else, we should make a new model just for
> accounts (i.e the one the accounts kcm uses) and use that in these dialogs.
> It'll need doing for the port to nepomuk models anyway)

Just a reminder unrelated to the patch: it would be even better to have just one model that can work in account/contact/metacontact mode and handles all the factory/features stuff depending on what you need
Comment 15 David Edmundson 2012-03-28 21:24:00 UTC
Git commit f40ae1128e1e12b229aa7c1f8d85b687abac8345 by David Edmundson, on behalf of Mikhail Krishtop.
Committed on 28/03/2012 at 23:23.
Pushed by davidedmundson into branch 'master'.

Add "Add Contact" action

REVIEW:104430

M  +2    -0    CMakeLists.txt
M  +50   -0    src/presenceapplet.cpp
M  +3    -0    src/presenceapplet.h

http://commits.kde.org/telepathy-presence-applet/f40ae1128e1e12b229aa7c1f8d85b687abac8345