Bug 174143 - [patch] Reconnect fails on password protected channels
Summary: [patch] Reconnect fails on password protected channels
Status: RESOLVED FIXED
Alias: None
Product: konversation
Classification: Applications
Component: general (show other bugs)
Version: 1.1
Platform: Debian testing Linux
: NOR normal
Target Milestone: ---
Assignee: Konversation Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-03 04:09 UTC by Nach
Modified: 2009-06-25 17:19 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nach 2008-11-03 04:09:17 UTC
Version:           1.1 (using KDE 3.5.10)
Compiler:          gcc version 4.3.2 (Debian 4.3.2-1) x86-64
OS:                Linux
Installed from:    Debian testing/unstable Packages

When one reconnects to a server (file -> reconnect), channels that require passwords to enter fail to be rejoined, even though those channels are properly auto joined when first connecting to that server.

I'm not sure why that is, but it seems that passwords aren't being saved in the m_channelList structure in server.cpp.

I came up with a temporary patch to fix the problem, which replaces updateAutoJoin() in the source of 1.1 (not SVN, didn't look at it).

------------------------------------------
static std::map<QString, QString> ChannelPasswords;

void Server::updateAutoJoin(Konversation::ChannelSettings channel)
{
    if (!channel.name().isEmpty())
    {
        setAutoJoin(true);
        setAutoJoinCommands(QStringList("JOIN " + channel.name() + " " + channel.password()));
        return;
    }

    Konversation::ChannelList tmpList;

    if (m_channelList.isEmpty() && getServerGroup())
        tmpList = getServerGroup()->channelList();
    else
    {
        QPtrListIterator<Channel> it(m_channelList);
        Channel* channel;

        while ((channel = it.current()) != 0)
        {
            ++it;
            tmpList << channel->channelSettings();
        }
    }

    if (!tmpList.isEmpty())
    {
        setAutoJoin(true);

        QStringList channels;
        QStringList passwords;
        QStringList joinCommands;
        uint length = 0;

        Konversation::ChannelList::iterator it;

        for (it = tmpList.begin(); it != tmpList.end(); ++it)
        {
            QString channel = (*it).name();
            QString password = ((*it).password().isEmpty() ? "." : (*it).password());

            //Save/Load passwords
            if (password != ".")
            {
              ChannelPasswords[channel] = password;
            }
            else if (ChannelPasswords.find(channel) != ChannelPasswords.end())
            {
              password = ChannelPasswords[channel];
            }

            int tempLen = channel.length();
            uint currentLength = getIdentity()->getCodec()->fromUnicode(channel, tempLen).length();
            tempLen = password.length();
            currentLength += getIdentity()->getCodec()->fromUnicode(password, tempLen).length();

            //Have to add length for commas, but no -1 for joined comma, because last entry isn't joined yet
            if (length + currentLength + 6 + channels.count() + passwords.count() >= 512) // 6: "JOIN " plus separating space between chans and pws.
            {
                joinCommands << "JOIN " + channels.join(",") + " " + passwords.join(",");

                channels.clear();
                passwords.clear();

                length = 0;
            }

            length += currentLength;

            channels << channel;
            passwords << password;
        }

        joinCommands << "JOIN " + channels.join(",") + " " + passwords.join(",");

        setAutoJoinCommands(joinCommands);
    }
    else
        setAutoJoin(false);
}
------------------------------------------
This works by creating a map of channels to passwords (include <map>), and uses the map. A proper fix should probably elsewhere. However, while writing this patch, I noticed two other bugs in the code which I also fixed.

1) When splitting the line so it shouldn't exceed 512, the current channel is on the old JOIN line and the new JOIN line.
2) Commas aren't taken into account in the length of the line.

Those are fixed, and code repetition is removed.
Comment 1 Nach 2008-11-03 16:58:06 UTC
Just wanted to say that if one wants to incorporate this patch in their code, they should place "static std::map<QString, QString> ChannelPasswords;" in server.h in the Server class as "std::map<QString, QString> ChannelPasswords;", so that it works properly with multiple servers.
Comment 2 Eike Hein 2008-11-04 12:02:45 UTC
Hi Nach,

it would be very helpful if you could try if an unmodified SVN checkout still exhibits the problem for you. Hopefully it won't, because of this fix that has been made recently (citing from the changelog):

"Added a network settings lookup fallback to retrieving the key of a channel.
Previously, this relied solely on the channel's mode map. Closes the brief
gap between a channel join and the server's reply to MODE where possible,
so that e.g. reconnecting directly after auto-joining a channel with a key
doesn't result in a failed rejoin due to not having the key by way of the
MODE reply yet."

You can get info on how to get Konversation from SVN in our wiki at: http://konversation.kde.org/wiki/SVN

I'll see about incorporating the other improvements you've made. General, unrelated tip, though: While using the STL isn't exactly "forbidden" in KDE code, KDE devs tend to keep to Qt container classes, i.e. QMap rather than std::map.
Comment 3 Nach 2008-11-04 18:06:04 UTC
Hi Eike, thanks for the prompt response.

I compiled and ran SVN #3302. If I click on reconnect, it now enters all my password protected channels properly. However, if my connection resets (easy test, ifconfig eth0 down, and wait a few minutes, then put it back up), then it reconnects to all my password protected channels but one. Yet this one along with the others connects fine on initial load.

I'm going to try and stick in some debug code and see if I can somewhat pinpoint where things are failing.

Regarding my use of STL, I know in Qt programs that it's generally preferable to use the Qt container classes. However I switched to developing with Qt 4 almost right after it came out, and I forgot all the Qt 3 classes, and didn't really feel like looking up exact usage. STL on the other hand, I still remember it quite well, and use it with all Qt unrelated code. I reinstalled Qt 3 dev tools just to work on the bug here. Also on that note, good luck with porting Konversation to Qt 4, member functions have changed quite a bit and immensely for the better I might add.
Comment 4 Nach 2008-11-04 20:12:21 UTC
Okay, I did some debugging.

On initial connect, Konversation sends:
"JOIN #zsnes,#nsrt,#snes,#zsnes-docs,#secret0,#nesvideos,#secret1,#secret2,#vba-m .,.,.,.,pass0,.,pass1,pass2"
I obviously edited the channel names and passwords to protect secret channels.

On reconnect it sends:
"JOIN #zsnes,#nsrt,#snes,#zsnes-docs,#secret0,#nesvideos,#secret1,#secret2,#vba-m .,.,.,.,pass0,.,10,pass2"

For #secret1 it's sending "10", for no apparent reason. Except perhaps that the channel is limited to 10 users...
I imagine that's somehow getting mixed up in the channel settings somewhere. I'll look into that and see if I can pinpoint it more.

Oh, and why is the code dropping off the last channel if a password is not needed? If all the latter unpassworded channels don't need to be listed, and Konversation is looking to not send what isn't needed, if (passwords.last() == ".") passwords.pop_back(); should be: while (passwords.last() == ".") passwords.pop_back();
Comment 5 Nach 2008-11-04 20:27:13 UTC
Okay, I verified it. I set the channel limit to 9, and now on reconnect it's trying:
"JOIN
#zsnes,#nsrt,#snes,#zsnes-docs,#secret0,#nesvideos,#secret1,#secret2,#vba-m
.,.,.,.,pass0,.,9,pass2"

So for some reason, on automatic reconnect (after lag for 3 minutes) instead of manual reconnect, if a channel has a user limit set, it's overriding the password with the channel limit.
Comment 6 Nach 2008-11-04 21:20:26 UTC
Okay more debugging. In channel.cpp in updateModeWidgets(), I added the following:

  char m[] = { '-', '+' };
  N_out << m[plus] << mode << ' ' << (parameter.isEmpty() ? "" : parameter.ascii()) << std::endl;

Output was as follows:
+o Nach
+o Nach
+o Nach
+o Nach
+o Nach
+o Nach
+t
+n
+t
+n
+n
+t
+n
+c
+s
+m
+t
+n
+c
+k Password0
+m
+t
+n
+n
+c
+J
+s
+m
+t
+n
+c
+l 9
+k 9
+s
+t
+n
+k Password2
+t
+n
+c

I'll try to see if I can pinpoint more. But even here it's getting the wrong parameter.
Comment 7 Nach 2008-11-04 22:08:14 UTC
And here's the fix:
Index: konversation/src/inputfilter.cpp
===================================================================
--- konversation/src/inputfilter.cpp    (revision 880056)
+++ konversation/src/inputfilter.cpp    (working copy)
@@ -794,10 +794,10 @@
                 QString modesAre;
                 QString message = i18n("Channel modes: ") + modeString;

+                int parameterCount=3;
                 for(unsigned int index=0;index<modeString.length();index++)
                 {
                     QString parameter;
-                    int parameterCount=3;
                     char mode=modeString[index];
                     if(mode!='+')
                     {
-------------------------------------------------------------
parameterCount is supposed to be incremented for each parameter read off the parameter list, but it was being reset each time in the loop. Moving its creation to outside the loop fixes all problems.

Please commit this. (and perhaps look into merging the good stuff from my previous patch regarding the breaking down the JOIN line to <512 chars)
Comment 8 Nach 2008-11-04 22:17:40 UTC
Oh, and the new name for the bug would perhaps be: "Password protected channels that contain a limit are improperly handled on automatic reconnect". Or perhaps: "Channels with multiple modes containing parameters have the parameters conflict with each other".
Comment 9 Eike Hein 2008-11-05 11:52:49 UTC
Hi Nach,

thanks for your stubborness in getting to the bottom of this. I'll see to that your fixes to the join wrapping and the mode handling end up in the forthcoming 1.1.1 maintenance release.
Comment 10 Jonas Vejlin 2009-06-07 14:56:48 UTC
Should this bug be marked as fixed?
Comment 11 Eike Hein 2009-06-07 15:05:37 UTC
No.
Comment 12 Travis McHenry 2009-06-18 08:03:04 UTC
SVN commit 983354 by tjmchenry:

Apply the fixes and patches discussed in bug 174143
I've tested to ensure normal situations are unaffected by the changes
But the use-case where these fixes apply is very hard to reproduce (I tried for an hour)
So if the reporter could please test and report back I'd appreciate it.
CCBUG: 174143

 M  +1 -2      inputfilter.cpp  
 M  +10 -17    server.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=983354
Comment 13 Nach 2009-06-18 10:43:21 UTC
Hi Travis, thanks for the commit, that was certainly a long wait for a simple fix -_-

I'm not so sure what's so hard to reproduce about it, you only need to be in two password protected channels in IRC, and disconnect yourself from the network.

I will go test the latest SVN as soon as I can figure out how to compile KDE4 apps.

I noticed in committing everything, you seemed to jump over this:
"Oh, and why is the code dropping off the last channel if a password is not
needed? If all the latter unpassworded channels don't need to be listed, and
Konversation is looking to not send what isn't needed, if (passwords.last() ==
".") passwords.pop_back(); should be: while (passwords.last() == ".")
passwords.pop_back();"

Was there a decision made about this?
Comment 14 Nach 2009-06-18 10:45:18 UTC
Oh, let me quantify what I said, those password protect channels also have to have a limit to amount of users as I said earlier.

If you're on FreeNode, you can make your own two password protected and user limited channels to test it.
Comment 15 Nach 2009-06-18 11:25:21 UTC
Okay, figured out how to build KDE 4 apps and ran r983396.
I can't say I like the current state of Konversation, but the bug is indeed fixed.

Thanks Travis.
Comment 16 Eike Hein 2009-06-18 11:35:41 UTC
What do you mean with not liking the current state? Aside from known todos like bringing marker lines back and making the config dialog KDE 4 HIG-compliant, which are being worked on, SVN basically has feature parity with 1.1 and is in fact quite a bit better in many ways as the changelogs of the 1.2 alphas should show.
Comment 17 Nach 2009-06-18 11:55:51 UTC
>SVN basically has feature parity with 1.1
I don't see how it has feature parity when the file menu only has close on it.

Maybe it's also just the defaults in the new version which I haven't tried to modify, but I don't see any icons appearing next to channel operators.
Comment 18 Nach 2009-06-18 12:04:21 UTC
Okay looked around a bit more. I can't move channel tabs around either with the KDE 4 version.

Yeah, I think saying this has feature parity with older versions is disingenuous.
Comment 19 Travis McHenry 2009-06-18 15:45:29 UTC
@#13 Ah, I was creating 12 channels with passwords and couldn't reproduce, slipped my mind that there needed to be 2 parameters set to provoke it, thanks for testing though.

As far as the removing .'s thing goes, the while() approach is the wrong one. When I was making this last night, I assumed because it was in the foreach, every time it was getting a new password added it was checking and removing if it was a "." However, upon further inspection I've noticed it's inside the if, so it's only when it's too big for one line. I believe it should be moved to before the if statement, I'll give this more thought when I've woken up a bit more.

...*10 minutes later* Wait, so the join command is like so:
JOIN #asdf,#asdf2,#asdf3 pass1,pass2,pass3

Assume you have no password for #asdf2, you can't remove it, or the password for #asdf3 will be applied to #asdf2 like so:
JOIN #asdf,#asdf2,#asdf3 pass1,pass3

But, if #asdf3 doesn't need a pass you can safely remove it like so:
JOIN #asdf,#asdf2,#asdf3 pass1,pass2

That's my understanding, please let me know if you disagree.

@#15 Thank you for the patch!

@#17 It would seem you're running konversation from inside it's build directory, for whatever reason (a reason not isolated to konversation) you need to install it before you get the various custom context menus. 

@#18 Tabs are moved with middle click, I believe the button choice is a KDE thing. If you think it should be left click please file a wishlist bug report. :)

Thanks again for helping out, sorry it took so long!
Comment 20 Travis McHenry 2009-06-18 21:33:03 UTC
Ah, looking back your suggestion was to do only the last ones.

So you'd have:
#asdf,#asdf2,#asdf3 pass1
instead of: 
#asdf,#asdf2,#asdf3 pass1,.,. 

Making the while() approach correct inside the if statement, sorry! I'll commit that after I get some lunch.
Comment 21 Travis McHenry 2009-06-18 21:40:14 UTC
SVN commit 983685 by tjmchenry:

Remove *all* trailing passwords that are just fillers instead of only one
CCBUG:174143

 M  +1 -1      server.cpp  


WebSVN link: http://websvn.kde.org/?view=rev&revision=983685
Comment 22 Eike Hein 2009-06-19 18:04:13 UTC
> Yeah, I think saying this has feature parity with older versions is
disingenuous.

KDE applications - and by the way, this includes Konversation 1.1 - need to be installed into /usr or a prefix covered by $KDEDIR/$KDEDIRS/$KDEHOME so they can locate their data when run. This includes their menu structure, which is stored in an XML file, and, in Konversation's case, nicklist icon themes. The file menu in the KDE 4 version is identical to that of Konversation 1.1, and the same nicklist icon themes continue to be available, although the default has changed to the "Oxygen" one (first included with v1.1) for better KDE 4 integration.

More info:
http://techbase.kde.org/KDE_System_Administration/KDE_Filesystem_Hierarchy
http://techbase.kde.org/KDE_System_Administration/Environment_Variables

Moving tabs still works exactly as in Konversation 1.1 as well: With either the regular tab bar or the listview version of the tab bar you can drag tabs around using middle-mouse, or you can use the actions found in the tab/listview item context menu, or the (same) actions found in the "Window" menu, or the keyboard shortcuts associated with those actions (which can be changed via "Configure Shortcuts..." in the "Settings" menu).

To provide some further background, middle-mouse drag'n'drop was the default behavior of the tab widget found in the KDE user interface libraries in KDE 3.x, and tab DND behavior thus used to be fairly consistent between KDE applications. In KDE 4, the situation is unfortunately less consistent, with some apps now using left-mouse for tab DND. We're currently sticking to the Konversation 1.1 behavior as left-mouse drag is used for "surfing" the items in the listview version of the tab bar and we'd like to keep the interaction model consistent between the regular tab bar and the listview version of the tab bar.

The bottom line is, you appear to have been disingenuous when you claimed to have figured out how to build and take a look at the KDE 4 version, and when you claimed to possess the ability to make an accurate assessment of whether the SVN code achieves feature parity with the stable release (which, by the way, it does not; I listed at least one significant exception in comment #16, namely marker lines not being back in action yet).

Please don't go around calling people liars when you don't know what you're doing. Thanks.
Comment 23 Nach 2009-06-19 18:31:09 UTC
Hi Travis, commit looks good.
I never knew tabs were drag-able around with the middle click, it's the last thing that would come to my mind. Instead I've been right clicking on them using the move left/right feature. Which I don't see appearing in version 4. Unless of course that also is an issue of running it from the wrong directory, which I'll have to look into.


>KDE applications - and by the way, this includes Konversation 1.1 - need to be
installed into /usr or a prefix covered by $KDEDIR/$KDEDIRS/$KDEHOME so they
can locate their data when run. This includes their menu structure, which is
stored in an XML file, and, in Konversation's case, nicklist icon themes. The
file menu in the KDE 4 version is identical to that of Konversation 1.1, and
the same nicklist icon themes continue to be available, although the default
has changed to the "Oxygen" one (first included with v1.1) for better KDE 4
integration.

Okay, I will look into that when I get a chance. None of that was obvious to me.
I do quite a bit of programming with Qt 4, and I don't have menus disappear in my personal programs when run from the "wrong" location.

>Moving tabs still works exactly as in Konversation 1.1 as well: With either the
regular tab bar or the listview version of the tab bar you can drag tabs around
using middle-mouse, or you can use the actions found in the tab/listview item
context menu, or the (same) actions found in the "Window" menu, or the keyboard
shortcuts associated with those actions (which can be changed via "Configure
Shortcuts..." in the "Settings" menu).

I saw no right click context menu in my test, but I will try again.

>The bottom line is, you appear to have been disingenuous when you claimed to
have figured out how to build and take a look at the KDE 4 version

No I wasn't, as I did indeed build it to test it, and took a look at it to make sure it worked. I may not have had the best experience, my fault it seems, but I did do what seemed to be the most obvious at the time. There was nothing disingenuous about it.


>you claimed to possess the ability to make an accurate assessment of whether
the SVN code achieves feature parity with the stable release

I have made no such claim. All I can tell is what looks missing to me. I really have no idea about Konversation in a larger picture, I know I haven't seen even 5% of what Konversation can really do.


>Please don't go around calling people liars when you don't know what you're
doing. Thanks.
I never called you a liar, I called the statement disingenuous, please don't confuse the two. And thanks for working on Konversation, good job on porting it.
Comment 24 Eike Hein 2009-06-19 19:00:42 UTC
> I never knew tabs were drag-able around with the middle click, it's the last
thing that would come to my mind.

I agree the discoverability sucks, but in KDE 3 the situation was that it was at least consistent that way between pretty much all apps. Not so much in KDE 4 anymore, but if we switch to left-click as some apps have done, we have to give up the popular "surfing" feature mentioned above (unless we make it an option). So we're a bit conflicted as to how to proceed.


> I do quite a bit of programming with Qt 4, and I don't have menus disappear in
my personal programs when run from the "wrong" location.

KDE 4 apps happen to use Qt 4, but the KDE libraries add additional frameworks used by applications. One of them is the XMLGUI framework used to define the structures of menus and toolbars. A powerful feature of XMLGUI is merging things together from multiple sources, which is used in conjunction with KDE's GUI component technology KParts. The "Okular" application (KDE 4's document viewer), for example, is a fairly thin shell application with minimal UI of its own, which then loads the bulk of what you see in the application as a KPart component (which is also part of the Okular codebase, but not of the executable). That KPart however also gets reused by the Konqueror web browser to display for example PDF documents embedded in the browser window. In both cases XMLGUI is used to merge UI actions provided by the KPart into the hosting application.

Konversation makes no such advanced use of XMLGUI, but only the basic use of using it to define menu, context menu and toolbar structures. XMLGUI is also used to store changes to the toolbar made by the user.

http://techbase.kde.org/Development/Architecture/KDE4/XMLGUI_Technology
http://techbase.kde.org/Development/Tutorials/Using_KParts


> I saw no right click context menu in my test, but I will try again.

The contents of that context menu are also defined in that aforementioned XML file that needs to be installed.


> There was nothing disingenuous about it.

Well, I hope you see now that neither was mine. SVN is not a stable release yet - it's called 1.2alpha for a reason, namely that it's not considered feature-complete yet - but aside from the aforementioned missing marker lines (which will return before 1.2 final with the merge of a rewritten chat view), it should be pretty much a drop-in replacement for 1.1 users right now, and due to bug fixes and robustness improvements in various areas is probably overall the best it's ever been, in fact.


> I never called you a liar, I called the statement disingenuous, please don't
confuse the two.

Alright, since I'm not a native English speaker I may have misunderstood the exact meaning.
Comment 25 Eike Hein 2009-06-19 19:01:27 UTC
(s/left-click/left-mouse/, to be clear.)
Comment 26 Nach 2009-06-24 18:50:00 UTC
Hi.

I ran make install.
I tried copying Konversation to many different directories within /usr, and running it from there.
I set a slew of environment variables.
Nothing seemed to work, I'm missing contents of the file menu in each one (except for close), and right click doesn't work.

Of course there's probably something I'm missing. But nothing I tried, and nothing I read in any documentation seemed to make Konversation work right.
Comment 27 Eike Hein 2009-06-25 05:29:43 UTC
cd $HOME
mkdir konversation
cd konversation
mkdir build install
svn co svn://anonsvn.kde.org/home/kde/trunk/extragear/network/konversation src
cd build
cmake -DCMAKE_INSTALL_PREFIX=$HOME/konversation/install ../src
make
make install
export PATH=$HOME/konversation/install/bin:$PATH
export KDEDIRS=$HOME/konversation/install:$KDEDIRS
export XDG_DATA_DIRS=$HOME/konversation/install/share:$XDG_DATA_DIRS
konversation
Comment 28 Nach 2009-06-25 15:27:54 UTC
Hi.
-DCMAKE_INSTALL_PREFIX seemed to do the trick. I set it to a sub directory and ran Konversation from there, and everything worked great. The environment variables didn't seem to do anything one way or the other.

Looking at SVN now, I can say I'm pleased with the port. I didn't put it under a microscope, but everything I used from previous versions which I looked for is now there. :)
Comment 29 Eike Hein 2009-06-25 15:34:27 UTC
'-DCMAKE_INSTALL_PREFIX' sets the prefix to which 'make install' installs the application (it's equivalent to the '--prefix' argument to the 'configure' scripts of autotools build systems). The environment variables should then contain the prefix as outlined above, or rather the prefix should be something that KDE is looking in by way of $KDEDIR/$KDEDIRS/$KDEHOME (or /usr, which I guess is looked at by default).
Comment 30 Nach 2009-06-25 17:14:44 UTC
I don't have any KDE environment variables set, but it works after setting
that prefix thing. It seems without it, make install doesn't copy the
"share" files where they need to go. I even installed it to a random
directory, and running it from there works fine, as long as
-DCMAKE_INSTALL_PREFIX pointed there.
Comment 31 Eike Hein 2009-06-25 17:19:05 UTC
Well, good that it works then :).