Full-fledged query buffers#598
Conversation
src/Query.cpp
Outdated
|
If you get a private message from someone, the CQuery will stay forever until ZNC shutdown, even if its buffer is empty now? |
There was a problem hiding this comment.
If controlpanel gets ability to change new parameters, maybe webadmin needs too?
|
So now default channel buffer size is also size for queries? Ok, but various descriptions of the parameter should say so. Btw, interesting attack vector: make multiple nicks all PM you (up to |
|
Now queries are deleted when appropriate instead of just clearing them, and I've also introduced another config option MaxQueries that defaults to 50. I reverted the controlpanel change and suggest tackling modules case by case in separate change(s). I'm not familiar enough with the modules to say what kind of consequences this change has on them, but I'd imagine a few of them could be affected. Maybe it's worth waiting for some clients to catch up with this feature before polluting controlpanel & webadmin with new options? For now, those who have a capable client are probably also capable of editing the .conf by hand. ;) |
How so? |
include/znc/Query.h
Outdated
There was a problem hiding this comment.
3rd parameter is always null. If it's unused, better remove it.
|
Hmm, private notices are still causing headache. IRC clients typically don't open a new query for private notices, but show them in the current view. Thus, there's no query to close and such notices as those from NickServ during the login keep hanging there... Any ideas how to tackle this? Should we perhaps keep the old kind of shared buffer for private notices and use dedicated buffers for private messages only? |
|
👍 I have updated the patch as per my earlier proposal. Query buffers are now PRIVMSGs only. Things seem to be running smooth here. |
src/IRCNetwork.cpp
Outdated
|
I'm not sure if it's good idea to separate notices from privmsgs... but ok. controlpanel and webadmin still need ability to change these new options (though preferably in separate commit). No, requiring user to change znc.conf is not an option. |
I realize now that my earlier comment was confusing. What I basically meant is that IMHO there's no need to complicate this pull request further with changes to various modules. For the most eager ones this is already usable as is - one just needs to edit znc.conf by hand until controlpanel/webadmin are updated. I'm willing to help with updating the modules later, but I hope we can leave that to a separate pull request. |
|
It's fine to complicate pull request with related changes to various modules. It's not fine to complicate commit with them. |
|
It's the usual dilemma. The more stuff you push the longer it takes to review and the whole thing gets delayed... :P |
|
That's true... Well, the main commit looks fine, though I guess I need to actually test it :) |
|
I think I got all relevant modules updated:
|
|
I've tested this. With Auto Clear Query Buffer set, it doesn't clear anything... Also, you didn't address my older comment "So now default channel buffer size is also size for queries? Ok, but various descriptions of the parameter should say so." |
Fixed, diff: http://pastie.org/9439545
Yes, the same buffer size applies for both. Which parameter descriptions did I miss? |
|
E.g. in webadmin on user page 2014-08-02 19:32 GMT+01:00 J-P Nurmi notifications@github.com:
|
|
webadmin & clearbufferonmsg updated: http://pastie.org/9441967 |
|
Thanks. One more detail, please rename MaxQueries to MaxBufferedQueries, or something like that. Current name is too confusing. |
Store query buffers per query the same way it's done for channels. This allows clients to implement persistent query buffers. Queries remain open across clients and sessions until a client explicitly sends a command to clear a (closed) query buffer. A new config option AutoClearQueryBuffer that default to false ensures behavioral backwards compatibility, and another config MaxQueries protects from OOM eg. due to PM attacks.
|
I just came to add a note and realized that I hadn't actually pushed the update I thought I had done. Sorry for the delay. I'm going on vacation for the rest of the month starting next week. I just wanted to say that I'm taking full responsibility of the feature and will be there to help fix any unforeseen issues it may raise. However, my availability will be somewhat limited the next few weeks. |
Full-fledged query buffers
|
For those looking for more details/instructions: http://wiki.znc.in/Query_buffers |
Store query buffers per query the same way it's done for channels.
This allows clients to implement persistent query buffers. Queries
remain open across clients and sessions until a client explicitly
sends a command to clear a (closed) query buffer.
A new config option AutoClearQueryBuffer that default to false
ensures behavioral backwards compatibility.