[Konversation-devel] Review Request: Enable advanced bookmarks functionality

[Eike Hein]

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


This looks pretty good already, and thank you for submitting it.

However rather than making decodeIrcUrl() public and constructing ConnectionSettings objects in the bookmarks code, I'd prefer it if there was a new connection instanciation method added to ConnectionManager that takes a QList of irc:// URL strings and takes it from there. The design goal for ConnectionManager was to centralize all connection instanciation and management operations, which were previously strewn all about the app - the current incarnation of the patch does too much of the heavy lifting in the bookmarks code for my taste.

Also, I think that with the addition of m_oneShotChannelList, m_initialChannel becomes unnecessary und should be removed. Rather, what m_initialChannel does can be done with a single channel in m_oneShotChannelList just as well. That gets rid of a codepath and makes things more generic and less error-prone.


- Eike


On 2009-09-14 21:00:58, Luigi Toscano wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1610/
> -----------------------------------------------------------
> 
> (Updated 2009-09-14 21:00:58)
> 
> 
> Review request for konversation.
> 
> 
> Summary
> -------
> 
> - Enable advanced bookmarks functionality, by overriding KBookmarkOwner's methods in KonviBookmarkHandler. Support for "Bookmark Tabs as Folder" and "Open Folder in Tabs" actions.
> - More verbose description for new bookmarks: "channel (network)".
> - Move title and URI generators from ViewContainer to more generic methods of ChatWindow.
> - Remove currentURL and currentTitle from mainWindow; they are a left-over from the days when there was no separate view container.
> 
> - Make ConnectionManager::decodeIrcUrl public.
> - Add new code to ConnectionSettings to support a set of channels (m_oneShotChannelList) that should be used only one for the current ConnectionSettings object (used by KonviBookmarkHandler::openFolderInTabs).
> - Move the code that generates a JOIN command from a ChannelList to a separate function.
> 
> 
> Diffs
> -----
> 
>   /trunk/extragear/network/konversation/src/bookmarkhandler.h 1023510 
>   /trunk/extragear/network/konversation/src/bookmarkhandler.cpp 1023510 
>   /trunk/extragear/network/konversation/src/connectionmanager.h 1023510 
>   /trunk/extragear/network/konversation/src/connectionmanager.cpp 1023510 
>   /trunk/extragear/network/konversation/src/connectionsettings.h 1023510 
>   /trunk/extragear/network/konversation/src/connectionsettings.cpp 1023510 
>   /trunk/extragear/network/konversation/src/irc/server.h 1023510 
>   /trunk/extragear/network/konversation/src/irc/server.cpp 1023510 
>   /trunk/extragear/network/konversation/src/mainwindow.h 1023510 
>   /trunk/extragear/network/konversation/src/mainwindow.cpp 1023510 
>   /trunk/extragear/network/konversation/src/viewer/chatwindow.h 1023510 
>   /trunk/extragear/network/konversation/src/viewer/chatwindow.cpp 1023510 
>   /trunk/extragear/network/konversation/src/viewer/viewcontainer.h 1023510 
>   /trunk/extragear/network/konversation/src/viewer/viewcontainer.cpp 1023510 
> 
> Diff: http://reviewboard.kde.org/r/1610/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Luigi
> 
>