Review Request: Handle favorites in JoinChatroomDialog

[David Edmundson]

> On March 6, 2012, 5:18 p.m., David Edmundson wrote:
> > Code is mostly fine, in fact all the code bits are fine, and I would have happily clicked "ship it!".
> > 
> > Only thing I'm not sure about it that you don't store/load/check the account selected. So if I save a favourite from my IRC account on #kde-telepathy, then load this dialog again, only this time my jabber account is selected. I'll still see #kde-telepathy in my favourites, if I hit continue it won't work.
> > 
> > I'm not sure if we want to make the favourites only show ones from the current account (might look weird), or select the correct account (a lot more risky IMHO, as the account might be deleted be offline, etc.) or something else entirely.
> 
> Dominik Cermak wrote:
>     Hm, I don't want to link the favorites to a specific account as for example one could have a favorite jabber room and different jabber accounts, whereby it doesn't matter with which account one joins it. Not sure if this is a valid use case or only a very special one from me but maybe only show those matching the protocolName or serviceName (probably serviceName is the better choice) of the currently selected account. So you wouldn't see your IRC favorites with only a jabber account online, but have common favorites for multiple jabber accounts.
>     
>     An extension could be to allow to declare a favorite to be account specific, so if you have a private and a work account you probably don't want to see your work favorites when you select your private account and vice versa.

Ok, so options are:
 1) Show only favourites on current account 
 2) Show only favourites on current service
 3) Show only favourites on current account with some sort of option "[ ] show for all accounts".

I think option 1 or 2 need doing before we can release this, it makes sense to me to do one of them first, then option 3 can be added as an extension afterwards if there's any demand for it.

I don't really see [3] as needed, as worst possible case ever is someone has to make two favourites, however I'm not against it either if you can  merge it in the UI nicely.

Also if you like, you can ship this as it currently is, if you open a bug report on what we've said.


> On March 6, 2012, 5:18 p.m., David Edmundson wrote:
> > rooms-model.cpp, line 178
> > <http://git.reviewboard.kde.org/r/104173/diff/1/?file=52062#file52062line178>
> >
> >     .isValid() should check index < rowCount() so this isn't needed.
> >     
> >
> 
> Dominik Cermak wrote:
>     Are you sure about that?
>     "A valid index belongs to a model, and has non-negative row and column numbers." (http://qt-project.org/doc/qt-4.8/qmodelindex.html#isValid)
>     
>     As far as I understand this statement it isn't checked, but I you are sure it is I'm happy to remove it.
> 
> Martin Klapetek wrote:
>     You are correct, it does not check < rowCount();
>     
>     The function actually looks like this:
>     
>       inline bool isValid() const { return (r >= 0) && (c >= 0) && (m != 0); }
>     
>     so it does not even check if the data exists.
>     
>     (for reference -- http://qt.gitorious.org/qt/qt/blobs/4.7/src/corelib/kernel/qabstractitemmodel.h#line77 )

Turns out I can be wrong. Who knew? :)


- David


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


On March 6, 2012, 2:35 p.m., Dominik Cermak wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104173/
> -----------------------------------------------------------
> 
> (Updated March 6, 2012, 2:35 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Now you can maintain a list of your favorite chatrooms.
> These are saved in the general ktelepathyrc to use them for autoconnection (not available yet, will work on it).
> 
> To not clutter the dialog too much I introduced tabs.
> 
> I'm wondering whether it would make sense to merge those two models (RoomsModel and FavoriteRoomsModel) into one (with FavoriteRoomsModel as the base for the new one).
> 
> 
> This addresses bug 291711.
>     http://bugs.kde.org/show_bug.cgi?id=291711
> 
> 
> Diffs
> -----
> 
>   dialogs/join-chat-room-dialog.h b5f7c6773f6a73f5653995a559bfab39b085e089 
>   dialogs/join-chat-room-dialog.cpp 06d0a3ae0d9cbfcb2247dd2593f9a6616613db20 
>   dialogs/join-chat-room-dialog.ui bec08058e2ffd56255b0f6c7977980226e973390 
>   rooms-model.h 675ea4cbb02cef5d823b69b6e48b292d3b4b07cf 
>   rooms-model.cpp 8a5cbb0b8fb0eaed8b30d3b516a574d231d7876e 
> 
> Diff: http://git.reviewboard.kde.org/r/104173/diff/
> 
> 
> Testing
> -------
> 
> Adding and removing favorites.
> After closing and reopening contact-list the favorites are still there.
> 
> 
> Screenshots
> -----------
> 
> favorites tab
>   http://git.reviewboard.kde.org/r/104173/s/448/
> query tab
>   http://git.reviewboard.kde.org/r/104173/s/449/
> 
> 
> Thanks,
> 
> Dominik Cermak
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120307/f82d59d8/attachment.html>