[2.1] Added processing of color codes into /rules #126

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

Justasic commented May 17, 2012

now without the branch merge messages :D

there you go

Contributor

SaberUK commented May 18, 2012

@Justasic Duplicating the code between modules is messy and should be avoided. You would probably be better off moving the parsing to configreader.cpp and automatically processing the files when the server configuration is loaded.

Contributor

Justasic commented May 20, 2012

alright >.< ill look into it when i get time. I have been busy all weekend

Contributor

SaberUK commented May 23, 2012

@Justasic You are still processing them on first execution rather than load. If you move the processing method call to ConfigReaderThread::Finish() you can get rid of ServerInstance->ProcessedMotdEscapes and ServerInstance->ProcessedRulesEscapes.

Contributor

Justasic commented May 23, 2012

bah.. I guess I didn't understand what you wanted when you tagged me on IRC, I'll fix it tonight.

Owner

attilamolnar commented May 23, 2012

Why don't you make this a seperate module? You could process the motd/rules on rehash, and also have a nice interface for other modules so they can supply arbitrary text for processing.

Contributor

Justasic commented May 23, 2012

@attilamolnar the parsing (the way that @SaberUK wants it) will put the function into config reader and make it part of the API.. now whoever thought it was a good idea to not allow core commands such as /motd to not be at least overloadable by other, 3rd party modules is beyond me but it does make it hard of offer other features that would affect core commands (such as this color codes thing). If that makes sense.

Owner

attilamolnar commented May 23, 2012

@Justasic No need to override the command to change the motd, one could change the loaded motd on the fly after a rehash event, see cmd_motd.cpp for details on how to access it.
As a side note, every command including core commands can be overloaded in modules.

Contributor

SaberUK commented May 23, 2012

the way that @SaberUK wants it

Actually, that was a suggestion not a demand. 👅

Contributor

Justasic commented May 24, 2012

@SaberUK I know, but you are correct that code should not be copy-paste like that, I'd like to keep inspircd as clean as possible.

@attilamolnar are you saying make a separate module just to hook the I_OnRehash event to parse the motd/rules?

Owner

rburchell commented May 25, 2012

please don't do this as a module. while I love modularity, there's such a thing as going a bit too far when you need to load a module to parse control characters in a motd - and there's really no good way of communicating that functionality.

just chuck the utility function somewhere central, and use it for both motd and rules, and let's be done with it

Contributor

Justasic commented May 25, 2012

yes, please lol

Contributor

Justasic commented May 27, 2012

Alright, I've applied what @SaberUK suggested (though at the time of posting this 2.1 needs the update) so this can be merged now.

Contributor

Justasic commented May 27, 2012

K fixed 2.1

Owner

rburchell commented May 28, 2012

@attilamolnar, any thoughts?

Owner

attilamolnar commented May 28, 2012

@Justasic in the double slash skipping logic make sure you don't do ret[pos-1] when pos == 0

Contributor

Justasic commented May 28, 2012

:o that would be bad wouldn't it? I think this fixes it

Owner

attilamolnar commented May 29, 2012

Still broken, pos being 0 is normal - it occurs when the first thing to be replaced is right at the beginning of the line. (also it can't be negative because std::string::size_type is unsigned) so testing for pos <= 0 doesn't make sense here.
A working approach would be to locate the string to be replaced, and if pos isn't 0, check if it's a double dash (If yes then skip). If pos is 0 then you already know it can't be a double dash since you don't have anything beginning with \ in your list of strings to be replaced.
By the way "\0017" should be "\017" so you replace \017 not \0017.

Owner

rburchell commented Jun 15, 2012

thanks for the contribution, but the 2.1 branch is closing, due to a general lack of faith in its stability and release readiness. we're closing it, and focusing on 2.0 for the time being; though we expect that a number of the improvements from 2.1 will end up being pulled in, so don't worry about this too much.

as a result of 2.1 being closed, i'm closing this pull request.

rburchell closed this Jun 15, 2012

Contributor

Justasic commented Jun 15, 2012

I read in #inspircd a few days ago, I'll finish it for 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment