Make collapsed groups searchable in the buddy list

Review Request #2167 — Created Jan. 2, 2023 and discarded

Information

pidgin/pidgin
release-2.x.y

Reviewers

Searching collapsed groups works by expanding them as they are
traversed by the search function. To mitigate leaving behind
expanded groups that were previously collapsed by the user,
the groups that were expanded by the search are flagged as
such so that they can be collapsed back automatically.

The search is started by typing while the Buddy List
window is active, or by pressing CTRL+F in the Buddy List.
Use the UP and DOWN arrow keys to move to the previous and
next matches.

Tested on Debian Sid and Cinnamon DE

Summary ID
Make collapsed groups searchable in the buddy list
Searching collapsed groups works by expanding them as they are traversed by the search function. To mitigate leaving behind expanded groups that were previously collapsed by the user, the groups that were expanded by the search are flagged as such so that they can be collapsed back automatically.
fe3bf88f570dd8bcfcd38c51321284ede14e65a1
Description From Last Updated

Unfortunately G_SOURCE_REMOVE wasn't added until glib 2.32 and the Windows build only has 2.16. So this needs to be replaced …

grimgrim

This would be a bit more readable if you create a variable for the function, something like GtkTreeViewSearchPositionFunc func = …

grimgrim

Correct me if I'm wrong, but this appears to iterate the entire blist structure to collapse any nodes that were …

grimgrim

This only iterates through chats and contacts and skips over buddies.

grimgrim

This should at least have NULL); on it's own line, but it might just be better to drop the existing …

grimgrim
grim
  1. 
      
  2. Sorry for taking so long on this :-/

  3. pidgin/gtkblist.c (Diff revision 1)
     
     
    Show all issues

    Unfortunately G_SOURCE_REMOVE wasn't added until glib 2.32 and the Windows build only has 2.16. So this needs to be replaced with FALSE.

  4. pidgin/gtkblist.c (Diff revision 1)
     
     
    Show all issues

    This would be a bit more readable if you create a variable for the function, something like

    GtkTreeViewSearchPositionFunc func = user_data;
    
    ...
    
    func(tree_view, search_dialog, NULL);
    

    Also note that we should probably be passing NULL as the user data, as even if the function was expecting user data, it probably isn't expecting a function pointer to itself.

    Also these lines need to be formatted to not wrap.

  5. pidgin/gtkblist.c (Diff revision 1)
     
     
    Show all issues

    Correct me if I'm wrong, but this appears to iterate the entire blist structure to collapse any nodes that were expanded for the search. Which is fine, but having it in a GtkTreeViewSearchEqualFunc is a problem.

    This function get called for every row in the tree, which means we're iterating all of the groups in the tree every time a row is checked for equality. Which is going to take a very long time for large blists.

    I think a better place to put this is in the wrapper function for the GtkTreeViewSearchPositionFunc. But I realize that might not be enough as we don't know when the user has changed the search term, but presumably the timeout will take care of it?

  6. pidgin/gtkblist.c (Diff revision 1)
     
     
    Show all issues

    This only iterates through chats and contacts and skips over buddies.

  7. pidgin/gtkblist.c (Diff revision 1)
     
     
    Show all issues

    This should at least have NULL); on it's own line, but it might just be better to drop the existing function into a variable and use that instead.

  8. 
      
grim
Review request changed
Status:
Discarded
Change Summary:

This is a dupe of /r/1494/