[kopete-devel] Review Request: kopete chatmemberlistmodel enhancement

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/
-----------------------------------------------------------

Review request for Kopete.


Summary
-------

- Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
- chatmemberlistmodel manages it's own sorted list of contacts
- implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
- added signal/slot for nickname-change to chatsession (model needs to know for resorting)


Diffs
-----

  branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
  branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
  branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
  branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610

Diff: http://reviewboard.kde.org/r/1862/diff


Testing
-------

works for me with IRC, ICQ,..


Screenshots
-----------

IRC with sorted members
  http://reviewboard.kde.org/r/1862/s/228/


Thanks,

Cyberbeat

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/#review2677
-----------------------------------------------------------

Ship it!


the d-pointer is nice, but we don't prefix d-pointer member variables with 'm' or 'm_'. If you have an svn account, please commit. If you don't have one, let me know and I'll commit it, with the aforementioned d-pointer changes.


branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp
<http://reviewboard.kde.org/r/1862/#comment1985>

    fix this whitespace error


- Matt


On 2009-10-16 17:11:52, Cyberbeat wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1862/
> -----------------------------------------------------------
>
> (Updated 2009-10-16 17:11:52)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> - Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
> - chatmemberlistmodel manages it's own sorted list of contacts
> - implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
> - added signal/slot for nickname-change to chatsession (model needs to know for resorting)
>
>
> Diffs
> -----
>
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610
>
> Diff: http://reviewboard.kde.org/r/1862/diff
>
>
> Testing
> -------
>
> works for me with IRC, ICQ,..
>
>
> Screenshots
> -----------
>
> IRC with sorted members
>   http://reviewboard.kde.org/r/1862/s/228/
>
>
> Thanks,
>
> Cyberbeat
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

> On 2009-10-17 02:39:13, Matt Rogers wrote:
> > the d-pointer is nice, but we don't prefix d-pointer member variables with 'm' or 'm_'. If you have an svn account, please commit. If you don't have one, let me know and I'll commit it, with the aforementioned d-pointer changes.

I have encountered a problem with my solution (happend with a metacontact included MSN/ICQ), but don't know how to reproduce it (how to add a contact twice to a chatsession):

in kopetechatsession.cpp there is a signal "contactadded" emitted, even if a contact is not added, because he already is contained in the session (in method "addContact"). Is this signal-emitting essential or may I remove it? Because the memberslistmodel will add the contact twice in the other case. Another solution would be to check, if the contact is already in the memberslistmodel, but that would make it more intelligent as it should be?

I don't understand why the "addContact" method in chatsession is so complicated. What cases have to have such special treatment?


- Cyberbeat


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/#review2677
-----------------------------------------------------------


On 2009-10-16 17:11:52, Cyberbeat wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1862/
> -----------------------------------------------------------
>
> (Updated 2009-10-16 17:11:52)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> - Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
> - chatmemberlistmodel manages it's own sorted list of contacts
> - implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
> - added signal/slot for nickname-change to chatsession (model needs to know for resorting)
>
>
> Diffs
> -----
>
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610
>
> Diff: http://reviewboard.kde.org/r/1862/diff
>
>
> Testing
> -------
>
> works for me with IRC, ICQ,..
>
>
> Screenshots
> -----------
>
> IRC with sorted members
>   http://reviewboard.kde.org/r/1862/s/228/
>
>
> Thanks,
>
> Cyberbeat
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

> On None, Cyberbeat wrote:
> >

I am not confident with this solution anymore. It is too slow for large chatrooms ( > 100 people):

- swiching tabs in chatwindow is slow, even if I optimize the initial fill of the modell with an n*log(n) algorithm/structure
- changing online-status/add/remove many people once is too slow

What I would prefer now:
- do the sorting in the kopetechatsession

But there are a few problems which have to be solved:
a) the myself-contact should also be added to the chatsession's members list (many other classes have to adapt to that behaviour, would be nice to get some help then)
b) the add/remove/..-signals emitted by the chatsession should also have an index to be more efficient
c) perhaps multiple add/remove/.. slot-calls should aggregated with a QTimer (with a sane timeout) to be able to do the sorted inserting more efficient.

I think the internal contact-storage-structure has to be a QList, even it is slow for inserting:
- some model-methods need indexes: data() and beginInsertItem() and beginRemoveItem()
- if many items have to be inserted/removed it could be done with a temporary QMap structure.


- Cyberbeat


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/#review2668
-----------------------------------------------------------


On 2009-10-16 17:11:52, Cyberbeat wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1862/
> -----------------------------------------------------------
>
> (Updated 2009-10-16 17:11:52)
>
>
> Review request for Kopete.
>
>
> Summary
> -------
>
> - Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
> - chatmemberlistmodel manages it's own sorted list of contacts
> - implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
> - added signal/slot for nickname-change to chatsession (model needs to know for resorting)
>
>
> Diffs
> -----
>
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
>   branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610
>
> Diff: http://reviewboard.kde.org/r/1862/diff
>
>
> Testing
> -------
>
> works for me with IRC, ICQ,..
>
>
> Screenshots
> -----------
>
> IRC with sorted members
>   http://reviewboard.kde.org/r/1862/s/228/
>
>
> Thanks,
>
> Cyberbeat
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/
-----------------------------------------------------------

(Updated 2009-10-27 23:02:59.614799)


Review request for Kopete and Matt Rogers.


Changes
-------

optimized speed for big chatrooms:
- faster inserts in chatmembersmodel
- skip some unimportant things (which really cost much time ~ 150 ms per contact added) in chatwindow for chatrooms with more members


Summary
-------

- Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
- chatmemberlistmodel manages it's own sorted list of contacts
- implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
- added signal/slot for nickname-change to chatsession (model needs to know for resorting)


Diffs (updated)
-----

  /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chattexteditpart.cpp 1026738
  /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp 1026738
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610

Diff: http://reviewboard.kde.org/r/1862/diff


Testing
-------

works for me with IRC, ICQ,..


Screenshots
-----------

IRC with sorted members
  http://reviewboard.kde.org/r/1862/s/228/


Thanks,

Cyberbeat

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/#review2859
-----------------------------------------------------------


Nice patch. Fix the nitpicking I did and it can be committed.


/branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp
<http://reviewboard.kde.org/r/1862/#comment2231>

    braces go on separate lines in kopete



/branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp
<http://reviewboard.kde.org/r/1862/#comment2232>

    braces on separate line here too.



/branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp
<http://reviewboard.kde.org/r/1862/#comment2233>

    the debug should be removed.



/branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp
<http://reviewboard.kde.org/r/1862/#comment2234>

    braces on a separate line here.



/branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h
<http://reviewboard.kde.org/r/1862/#comment2235>

    add another p in suppress. :)



/branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp
<http://reviewboard.kde.org/r/1862/#comment2236>

    i see no reason to keep this debug statement.


- Matt


On 2009-10-27 23:02:59, Cyberbeat wrote:

>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1862/
> -----------------------------------------------------------
>
> (Updated 2009-10-27 23:02:59)
>
>
> Review request for Kopete and Matt Rogers.
>
>
> Summary
> -------
>
> - Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
> - chatmemberlistmodel manages it's own sorted list of contacts
> - implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
> - added signal/slot for nickname-change to chatsession (model needs to know for resorting)
>
>
> Diffs
> -----
>
>   /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chattexteditpart.cpp 1026738
>   /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp 1026738
>   /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
>   /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
>   /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
>   /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610
>
> Diff: http://reviewboard.kde.org/r/1862/diff
>
>
> Testing
> -------
>
> works for me with IRC, ICQ,..
>
>
> Screenshots
> -----------
>
> IRC with sorted members
>   http://reviewboard.kde.org/r/1862/s/228/
>
>
> Thanks,
>
> Cyberbeat
>
>

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1862/
-----------------------------------------------------------

(Updated 2009-10-30 14:37:27.946772)


Review request for Kopete and Matt Rogers.


Changes
-------

I have now done the little changes. One last (big?) problem: this diff is against 4.3 branch. I have no trunk build, and will not use kde trunk on my pc (pc is very slow).

a) may I commit it to 4.3 branch?
b) may someone else make it work in trunk? (the files should not have changed much I think)


Summary
-------

- Changed the chatmemberlistmodel so that it sorts the contacts by onlinestatus-weight/nickname (important for IRC).
- chatmemberlistmodel manages it's own sorted list of contacts
- implemented the "FIXME"s so that the add/remove slots do not call "reset()" anymore, which takes a lot of time for big irc channels
- added signal/slot for nickname-change to chatsession (model needs to know for resorting)


Diffs (updated)
-----

  /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chattexteditpart.cpp 1026738
  /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp 1026738
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.h 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.cpp 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp 1035610

Diff: http://reviewboard.kde.org/r/1862/diff


Testing
-------

works for me with IRC, ICQ,..


Screenshots
-----------

IRC with sorted members
  http://reviewboard.kde.org/r/1862/s/228/


Thanks,

Cyberbeat

_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel

On Friday 30 October 2009 09:37:28 cyberbeat@... wrote:

> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1862/
> -----------------------------------------------------------
>
> (Updated 2009-10-30 14:37:27.946772)
>
>
> Review request for Kopete and Matt Rogers.
>
>
> Changes
> -------
>
> I have now done the little changes. One last (big?) problem: this diff is
>  against 4.3 branch. I have no trunk build, and will not use kde trunk on
>  my pc (pc is very slow).
>
> a) may I commit it to 4.3 branch?
> b) may someone else make it work in trunk? (the files should not have
>  changed much I think)
>
>
> Summary
> -------
>
> - Changed the chatmemberlistmodel so that it sorts the contacts by
>  onlinestatus-weight/nickname (important for IRC). - chatmemberlistmodel
>  manages it's own sorted list of contacts
> - implemented the "FIXME"s so that the add/remove slots do not call
>  "reset()" anymore, which takes a lot of time for big irc channels - added
>  signal/slot for nickname-change to chatsession (model needs to know for
>  resorting)
>
>
> Diffs (updated)
> -----
>
>  
>  /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chattexteditpart.cpp
>  1026738 /branches/KDE/4.3/kdenetwork/kopete/kopete/chatwindow/chatview.cpp
>  1026738
>  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.
> h 1035610
>  /branches/KDE/4.3/kdenetwork/kopete/libkopete/chatsessionmemberslistmodel.
> cpp 1035610
>  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.h 1035610
>  /branches/KDE/4.3/kdenetwork/kopete/libkopete/kopetechatsession.cpp
>  1035610
>
> Diff: http://reviewboard.kde.org/r/1862/diff
>
>
> Testing
> -------
>
> works for me with IRC, ICQ,..
>
>
> Screenshots
> -----------
>
> IRC with sorted members
>   http://reviewboard.kde.org/r/1862/s/228/
>
>
> Thanks,
>
> Cyberbeat
>

Saw that this was committed. Thanks. :) Are you going to backport it to the
4.3 branch? Or should I?
--
Matt


_______________________________________________
kopete-devel mailing list
kopete-devel@...
https://mail.kde.org/mailman/listinfo/kopete-devel