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

[Eike Hein]

> On 2009-09-15 08:26:38, Eike Hein wrote:
> > 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.
> >
> 
> Luigi Toscano wrote:
>     The new version of the patch moves the all connection code into ConnectionManager and removes m_initialChannel.
>     
>     PS. Please note that I can commit myself as I have an svn account.

Hi Luigi,

thanks for making the changes. The patch looks good now. Unfortunately I have to ask you to hold off on comitting it for now, though, as we've just entered feature freeze on Konversation 1.2 (aside from the planned re-addition of marker line support). That's not a reflection upon the quality of your work, just that we need to muster up some release discipline right now, which means stopping to merge stuff even when it looks good.

I'll inform you once trunk is back open for new features (we intend to release 1.2 in early October), and then it can go in. I've wanted this functionality for a long time actually, in fact it was one of the first things I wanted to add when I became a Konversation contributor myself -- but it turned out I had to do things like add proper network support and write ConnectionManager first. So kudos for finally getting us the rest of the way!


- Eike


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


On 2009-09-16 22:33:35, Luigi Toscano wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1610/
> -----------------------------------------------------------
> 
> (Updated 2009-09-16 22:33:35)
> 
> 
> 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/viewer/viewcontainer.cpp 1024521 
>   /trunk/extragear/network/konversation/src/viewer/viewcontainer.h 1024521 
>   /trunk/extragear/network/konversation/src/mainwindow.h 1024521 
>   /trunk/extragear/network/konversation/src/mainwindow.cpp 1024521 
>   /trunk/extragear/network/konversation/src/viewer/chatwindow.h 1024521 
>   /trunk/extragear/network/konversation/src/viewer/chatwindow.cpp 1024521 
>   /trunk/extragear/network/konversation/src/connectionsettings.h 1024521 
>   /trunk/extragear/network/konversation/src/connectionsettings.cpp 1024521 
>   /trunk/extragear/network/konversation/src/irc/server.h 1024521 
>   /trunk/extragear/network/konversation/src/irc/server.cpp 1024521 
>   /trunk/extragear/network/konversation/src/connectionmanager.h 1024521 
>   /trunk/extragear/network/konversation/src/connectionmanager.cpp 1024521 
>   /trunk/extragear/network/konversation/src/bookmarkhandler.h 1024521 
>   /trunk/extragear/network/konversation/src/bookmarkhandler.cpp 1024521 
> 
> Diff: http://reviewboard.kde.org/r/1610/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Luigi
> 
>