Opened 10 years ago

Last modified 9 years ago

#10891 new patch

gevolution plugin adds buddies to random account

Reported by: cedel Owned by:
Milestone: Patches Needing Improvement Component: plugins
Version: 2.6.3 Keywords:
Cc:

Description

OK, it doesn't actually add it to random account, but the account connected with the first connection it finds, regardless of protocol.

How to reproduce:

  1. Click "Add Buddy"
  2. Select buddy from list
  3. click "Select Buddy"

Attachments (3)

matching_account_entry.diff (34.4 KB) - added by cedel 10 years ago.
matching_account_entry_1.diff (24.3 KB) - added by cedel 10 years ago.
matching_account_entry_2.diff (25.5 KB) - added by cedel 10 years ago.

Download all attachments as: .zip

Change History (18)

Changed 10 years ago by cedel

comment:1 Changed 10 years ago by cedel

Above is a patch against 2.6.3, that should solve the problem. It also adds an extra dialog, which lets the user choose an account, if they have more than one account with matching protocol (if they have only one, it behaves as usual). Prevents adding buddy (with the same account) to "Buddies" group twice if no group is selected in the "Add Buddy" dialog.

It also fixes ticket:10890 and adds the gevolution_gg_skype.diff patch from ticket:10709 .

Please double-check the patch, since I'm a newbie regarding C programming. It works for me.

comment:2 follow-up: Changed 10 years ago by darkrain42

  • Milestone set to Patches Needing Improvement
  • Type changed from defect to patch

It looks like there's a lot of needless re-indentation that makes it difficult (for me...I'm lazy...) to review the patch.

Take a look at line 451 of add_buddy_dialog.c in the patch.

If i missed a reason for the indentation, my apologies, but I didn't see one.

comment:3 in reply to: ↑ 2 Changed 10 years ago by cedel

Replying to darkrain42:

It looks like there's a lot of needless re-indentation that makes it difficult (for me...I'm lazy...) to review the patch.

Take a look at line 451 of add_buddy_dialog.c in the patch.

If i missed a reason for the indentation, my apologies, but I didn't see one.

Re-indentation? I'm sorry, I don't exactly understand what you mean (after all, I'm not native english speaker)... The code is indented by one Tab from the bracket, I tried to break lines longer then some 80-100 characters and in some cases (along the lines of the present plugin code) put separate parameters on individual lines (like in treeview etc.). Maybe that's what's causing the problems? To be true, the indentation and word wrap looks different in Eclipse, in Bluefish and on the page (in Trac).

So, to improve the patch, you want me to remove the indentation and manual line wrap?

comment:4 Changed 10 years ago by darkrain42

  • Status changed from new to pending

Starting around this section of the patch:

536	 	        gtk_tree_selection_set_mode(selection, GTK_SELECTION_SINGLE); 
 	818	        dialog->win = pidgin_create_window(_("Add Buddy"), PIDGIN_HIG_BORDER, "add_buddy", TRUE); 
 	819	                gtk_widget_set_size_request(dialog->win, -1, 400); 
537	820	 
538	 	        g_signal_connect(G_OBJECT(selection), "changed", 
539	 	                                         G_CALLBACK(selected_cb), dialog); 
 	821	                g_signal_connect(G_OBJECT(dialog->win), "delete_event", 
 	822	                                                 G_CALLBACK(delete_win_cb), dialog); 
540	823	 
541	 	        add_columns(dialog); 
 	824	                /* Setup the vbox */ 
 	825	                vbox = gtk_vbox_new(FALSE, 12); 
 	826	                gtk_container_add(GTK_CONTAINER(dialog->win), vbox); 
 	827	                gtk_widget_show(vbox); 

the indentation changes from one tab to two tabs, which is incorrect.

comment:5 Changed 10 years ago by cedel

  • Status changed from pending to new

Ouch, sorry, missed that one...:(

Attempt No.2, hopefully better.

Changed 10 years ago by cedel

comment:6 follow-up: Changed 10 years ago by darkrain42

That looks much better, thanks!

I have a few comments: if (( protocolname != NULL) && (strcmp(protocolname, (dialog->account)->protocol_id)) != 0)

  • This should use the purple_account_get_protocol_id accessor function (there are a number of places where this is the case. In general, if you're doing "account->something", try to use an accessor function instead).
  • Can protocolname be NULL? Under what circumstances does that happen? (Sorry, I'm not terribly familiar with the dialog in the first place.)

The method you use to iterate over linked lists is inefficient. Doing:

    int length = g_list_length(list);
    int i;
    for (i = 0; i < length; ++i) {
        data = g_list_nth_data(list, i);
        ...
    }

causes you to end up traversing the linked list a large number of times. For example, to retrieve the 2nd piece of data, g_list_nth_data has to traverse the first two links in the list, but to get the third, it has to traverse the first three links (this becomes inefficient very quickly).

Instead, try something like this:

    GList *node;

    node = list;
    while (node) {
        data = node->data;
        ...do something with data
        node = node->next;
    }

(hopefully that makes sense. If not, I can try to explain again)

On line 445, t = find_account_for_protocol(dialog, protocolname); , but the value 't' does not appear to be used anywhere? It looks like that function only ever returns 0, which is never used. It should be void instead (i.e. returns no value).

comment:7 in reply to: ↑ 6 Changed 10 years ago by cedel

Replying to darkrain42:

First, sorry for the delay.

  • This should use the purple_account_get_protocol_id accessor function (there are a number of places where this is the case. In general, if you're doing "account->something", try to use an accessor function instead).

Corrected (hopefully didn't miss any place).

  • Can protocolname be NULL? Under what circumstances does that happen? (Sorry, I'm not terribly familiar with the dialog in the first place.)

Yes, it can. The dialog displays all contacts in the given eds address book, even ones that have no IM info. The original version makes all decisions based on presence of IM username in the contact and seems to discard protocol hint right after icon for it is created - basically, I added a string column into the tree model (that stores protocolname) to make comparisons easy (or even possible) and set it to NULL, when there's no IM info from eds - no username = NULL protocolname.

In this case I just thought it safer to check for protocolname (instead of username), if the next comparison uses strcmp to compare the value of protocolname.

The method you use to iterate over linked lists is inefficient. (hopefully that makes sense. If not, I can try to explain again)

Thanks, it makes perfect sense - corrected (in a slightly different way, but hopefully along the lines of the recommendation).

On line 445, t = find_account_for_protocol(dialog, protocolname); , but the value 't' does not appear to be used anywhere? It looks like that function only ever returns 0, which is never used. It should be void instead (i.e. returns no value).

Corrected, it was a leftover from a previous experiment.

I also made a few other subtle changes.

Changed 10 years ago by cedel

comment:8 Changed 9 years ago by darkrain42

Ticket #10890 has been marked as a duplicate of this ticket.

comment:9 Changed 9 years ago by darkrain42

  • Milestone changed from Patches Needing Improvement to Patches Needing Review

comment:10 Changed 9 years ago by QuLogic

There are unnecessary changes in this patch, such as adding the GG and Skype protocols.

comment:11 follow-up: Changed 9 years ago by QuLogic

What is changewin->selection for? It does not appear to be used outside of the function in which it is set, so why does it need to be saved?

Using g_list_length to check if the list is 0, 1, or "many" is not necessary. Just check list and list->next. Also, you call g_list_length multiple times instead of using the cached value in d.

comment:12 follow-up: Changed 9 years ago by QuLogic

  • Keywords gevolution Evolution removed
  • Milestone changed from Patches Needing Review to Patches Needing Improvement

comment:13 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by cedel

Replying to QuLogic:

What is changewin->selection for? It does not appear to be used outside of the function in which it is set, so why does it need to be saved?

Please, can you be more specific, on what line number? Cause if I look at the last published version of the patch, I don't see it anywhere? But I am sleepy now...

Using g_list_length to check if the list is 0, 1, or "many" is not necessary. Just check list and list->next. Also, you call g_list_length multiple times instead of using the cached value in d.

True, but it seems more understandable this way (at least for me) and debug gets fresh info if something happened (so g_list_length gets called twice within the same function). But will check on this one again later.

comment:14 in reply to: ↑ 12 Changed 9 years ago by cedel

Replying to QuLogic:

I understand the change of milestone, but why the keywords?

comment:15 in reply to: ↑ 13 Changed 9 years ago by QuLogic

Replying to cedel:

Replying to QuLogic:

What is changewin->selection for? It does not appear to be used outside of the function in which it is set, so why does it need to be saved?

Please, can you be more specific, on what line number? Cause if I look at the last published version of the patch, I don't see it anywhere? But I am sleepy now...

In selected_account_cb and gevo_select_account_dialog.

Using g_list_length to check if the list is 0, 1, or "many" is not necessary. Just check list and list->next. Also, you call g_list_length multiple times instead of using the cached value in d.

True, but it seems more understandable this way (at least for me) and debug gets fresh info if something happened (so g_list_length gets called twice within the same function). But will check on this one again later.

It's less efficient that way. I don't know what you mean by "fresh info", since the two g_list_length calls are in almost adjacent statements and the list cannot change length between them.

Another instance is in gevo_select_account_dialog. You don't need to check the list length if all you want to know is if there's more than one. Also, if you reverse the condition, you could return from the function right there and you wouldn't need all the indenting (since basically the entire function is indented an extra tab).

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!