communi / communi-desktop

An IRC client for desktop environments
https://communi.github.io/
BSD 3-Clause "New" or "Revised" License
57 stars 22 forks source link

Add logger plugin #124

Closed anselmolsm closed 7 years ago

anselmolsm commented 7 years ago

Submitting this PR to gather feedback on this. I wrote this plugin to store channels history in logfiles, as I need to grep things on them from time to time at work. After a few code modifications and weeks of tests, I thought it could be interesting to have this as part of communi.

As I said it works fine - I've been testing this for a while on Linux and Windows - however it needs polishing, for example there are hard coded things (e.g. dir logs are stored) and proper error handling is missing.

jpnurmi commented 7 years ago

Since there is currently no mechanism that allows plugins to define settings, I think it would be fine to just simply create a "Logging" section (Enabled, Directory, ...?) next to the "Theme" section on the settings page on the app side for now.

jpnurmi commented 7 years ago

How about something like this?

screenshot from 2017-04-20 08-57-13

anselmolsm commented 7 years ago

@jpnurmi Yeah, I wasn't sure about how to add a config option for a plugin. I'll follow your suggestion. New patch soon.

anselmolsm commented 7 years ago

I pushed this commit again, now with the code changes suggested. The UI for settings and related changes are in my branch logger-wip, but first I want to find a proper way to disable the plugin, which might require a patch to PluginLoader as well.

Venemo commented 7 years ago

@anselmolsm What would you suggest in PluginLoader?

anselmolsm commented 7 years ago

@Venemo Perhaps a way to execute QPluginLoader::unload() for a given plugin, to be used in cases like the checkbox "Logging" on the settings page. This would avoid calls to plugin's methods full of internal checks to abort them, if the plugin is disabled.

Venemo commented 7 years ago

@anselmolsm Ah, I get your point. Perhaps there could be a list of plugins in Settings and the user could choose which ones to enable? Or do you mean to add a special case just for your logger plugin?

anselmolsm commented 7 years ago

@Venemo I haven't thought about a list to enable/disable plugin, but I guess the load/unload infra in PluginLoader could be used for that as well.

Venemo commented 7 years ago

@anselmolsm I believe I could give the PluginLoader a go next weekend at the earliest. I will also see what I can do about the settings GUI. I like the proposal from @jpnurmi here.

One more question, just out of curiosity: as far as I see, your plugin simply logs messages into a file. Some other apps that do logging also load the logs and show the messages on the GUI at app startup. Is this something you plan to add, or is that out of your scope?

anselmolsm commented 7 years ago

@Venemo cool, looking forward to see your changes :)

Regarding loading logs - Yes, I think loading logs belongs to this plugin as well. Once this initial patch gets merged, I'll resume the work on that.

Venemo commented 7 years ago

@anselmolsm Okay, so I merged your plugin locally and implemented the necessary stuff, but there is an issue. I'll push my changes once I resolve it.

When the logger is disabled at application startup, then re-enabled during runtime, it doesn't log anything. The reason: when the plugin is disabled its bufferAdded and bufferRemoved methods don't get called, so it isn't connected to any of the buffers... I should have thought of this earlier, but it didn't occour to me until I saw it happen.

So now I need to add a way for the BufferPlugin to get the list of all the currently existing buffers. This is very tricky to do without introducing a bunch of inter-dependencies between stuff that shouldn't depend on each other, but I'll think of something.

Venemo commented 7 years ago

Okay, the problem has now been solved, in a somewhat complicated way.

And I modified LoggerPlugin as well:

Hope this helps!

Venemo commented 7 years ago

@anselmolsm Please take a look at my commits and see if they are okay with you.

anselmolsm commented 7 years ago

@Venemo , cool! I couldn't see your patches during the weekend, but I'll take a look later today. By your comments, it looks promising :)