Various adaptations

[OndrejPopp]

Hi Frances, I have used your library as a submodule for kdenlive to implement a code tracer. I was having some problems making it thread safe. At first I tried something myself, because it appeared quite simple at first adding a QWriteLocker. However it is not thàt simple apparently because that did crash occasionally while logging to a file. So then I moved in your library and that works ok, thanks!

I made some changes to it that are described here below. You are welcome to tweak or modify this further any way you like if you want this. Thanks again, Ondrej

• made QLoggerManager::getInstance() return a reference instead of a pointer

• added

  bool QLoggerManager::hasModules() const bool QLoggerManager::hasModule(const QString& module) const

  to check for, and add a destination on the fly, if you do not have (or want to have) a seperate configuration section

• changed QMutex mMutex { QMutex::Recursive } -> QRecursiveMutex mMutex

• added QDebug.noquote().nospace() to console formatting

• moved the addition of the newline character to writing to file only because qInfo() adds already its own.

• renamed QLogger::LogFileDisplay -> QLogger::LogFileTag

• added QLogger::LogFileHandling to specify a file handling mode,

  enum class LogFileHandling
  {
      Single,        // single file, with original name
      SingleTagged,  // single file, with original name plus a tag according to LogFileTag
      Split,         // the original behaviour : keep original name but split on predefined file size with tag according to LogFileTag
      Default        // select predefined default choice
  }

• also added 'Default' enumerators to LogMode and LogFileTag to more correctly detect the default choice

[francescmm]

Thanks for the contribution @OndrejPopp!

I'll take a look to the changes and see which one I can merge directly. Most of the changes look alright and in fact the only thing that makes me hesitate to just merge it, is the change to a reference instead of a pointer. Mainly because that's a huge breaking change.

At the same time for those who use this as dependency for their app (myself in GitQlient). But I'm pretty sure that is a nice one to have.

I have one question about the QRecursiveMutex. I guess that you compile this with Qt6, right? If that's the case, I think I might have to keep some ifdefs in the code for now until Linux distros are up to speed migrating to Qt6.

I'll take a look soon and try to merge it after modifying GitQlient.

[francescmm]

@OndrejPopp I've changed the base so I'll work with it in a development branch and when ready (in other repos) I'll merge all together :)

[OndrejPopp]

Hi Frances, Sorry about the breaking changes. I thought concerning modern C++ and all that, that a reference would be neater! In any case, because I also introduced LogFileHandling in the interface for addDestination there are more breaking  changes when you use the default parameters.  But I  would still expect that updating all this shouldn't take more than a few minutes? Depends ofcourse how many times these functions are referenced but I expect not that much. An other thing are the __Log macro's, I didn't use those so I may have forgotten updating those, but after that is done the change is then transparant to the user.

Next, because I added noquote().nospace() to qInfo() for printing to the console, you may see some changes in the output. First I thought to make this configurable by means of a lambda function, (more breaking changes) but then I didn't. So if this is an issue this needs to be done as well.

Concerning the QRecursiveMutex, actually I am still using Qt 5.15, but somehow although it should still work according to the documentation the compiler complained that QMutex has no Recursive member, in addition the documentation suggests to use QRecursiveMutex in this case so I did that, https://doc.qt.io/qt-6/qmutex.html#RecursionMode-enum I just copy-pasted this link from qt5 documentation but it gives me a link to qt6 nevertheless, so I guess it is a migration issue that already starts with Qt 5.15

For the rest, Have a nice weekend, Ondrej

⁣Get BlueMail for Android ​

On Jul 16, 2022, 09:48, at 09:48, "Francesc M." @.***> wrote:

Thanks for the contribution @OndrejPopp!

I'll take a look to the changes and see which one I can merge directly. Most of the changes look alright and in fact the only thing that makes me hesitate to just merge it, is the change to a reference instead of a pointer. Mainly because that's a huge breaking change.

At the same time for those who use this as dependency for their app (myself in GitQlient). But I'm pretty sure that is a nice one to have.

I have one question about the QRecursiveMutex. I guess that you compile this with Qt6, right? If that's the case, I think I might have to keep some ifdefs in the code for now until Linux distros are up to speed migrating to Qt6.

I'll take a look soon and try to merge it after modifying GitQlient.

-- Reply to this email directly or view it on GitHub: https://github.com/francescmm/QLogger/pull/12#issuecomment-1186111292 You are receiving this because you were mentioned.

Message ID: @.***>

[OndrejPopp]

Ok, sounds like a good plan :)

⁣Get BlueMail for Android ​

On Jul 16, 2022, 10:39, at 10:39, "Francesc M." @.> wrote: @. I've changed the base so I'll work with it in a development

branch and when ready (in other repos) I'll merge all together :)

-- Reply to this email directly or view it on GitHub: https://github.com/francescmm/QLogger/pull/12#issuecomment-1186121402 You are receiving this because you were mentioned.

Message ID: @.***>

[OndrejPopp]

Hi Frances, I have been thinking today about the pointer to reference change. This one is not really required, but it makes the interface more neater in my opinion. But if it is really too much of a hassle, and it is the only thing that breaks those other repositories, and you don't see the added value in this, then please feel free to change it back to a pointer if you want to.

Ok.

[francescmm]

Hi @OndrejPopp, I agree that the interface is nicer with the reference instead of the pointer. But this will also require some changes in QLogger that are not in the PR so it works. For instance all the QLog_XXX macros that still use the arrow operand.

I'll start the review now :)

[OndrejPopp]

Dear Frances, I have decided not to merge with you. The main reason for this is that all you committed by yourself is my change of QMutex -> QRecursiveMutex. You already committed that like you figured that out all by yourself, without mentioning me.

So if that's all you are interested in, it is doubtful that you have the right commitment. In addition it appears to me that you are a person who takes advantage of others, or even steals from them. So although I am greatly helped by your (?) thread safe implementation, it remains to be seen where this work really originates from that you may have committed all by yourself.

In addition are you asking me to add Doxygen? Who do you think I am Frances?

Then there is this other issue about the angle brackets. Yes it is a CMake configuration issue, but if you want to use angle brackets then you should have provided CMake configuration for this library by yourself so it can be compiled as a seperate library, since you did not do that I can not use angle brackets. And I am not going to add a CMake configuration for you, for the same reason : who do you think I am?

So, I am going to use my own fork, and add some more features to it, such as lambda functions.

May you be interested, you can pick it up all by yourself and make any modifications to it you like. Such as adding Doxygen or whatever.

Have a nice afternoon, Kind regards, Ondrej

[francescmm]

Dear Frances, I have decided not to merge with you. The main reason for this is that all you committed by yourself is my change of QMutex -> QRecursiveMutex. You already committed that like you figured that out all by yourself, without mentioning me.

If you want you can check that tiny change is part of major change. Your changes would still be in the history if you wanted.

So if that's all you are interested in, it is doubtful that you have the right commitment.

My commitment is with my code. Yours it's clear what it is for this response and the general tone and manners.

In addition it appears to me that you are a person who takes advantage of others, or even steals from them.

You start to sound like a troll and I won't allow it in here. Your manners speak for you and I suggest you deal with your anger in other places.

So although I am greatly helped by your (?) thread safe implementation, it remains to be seen where this work really originates from that you may have committed all by yourself.

Chill dude :D

Your changes bring a lot of unnecessary breaking changes that I okay with mergin in, but need to get a weell done review.

In addition are you asking me to add Doxygen? Who do you think I am Frances?

Two things here:

1. Yes, of course I'm asking that. If you contribute to a project you follow the comments of the owner. And at the very least you keep the quality of the code and documentation of the API as it is. If you're not interested in making the effort of it, don't do it.
2. My name is Francesc. You have plenty of places were that is shown and you took no time to check that.

Then there is this other issue about the angle brackets. Yes it is a CMake configuration issue, but if you want to use angle brackets then you should have provided CMake configuration for this library by yourself so it can be compiled as a seperate library, since you did not do that I can not use angle brackets. And I am not going to add a CMake configuration for you, for the same reason : who do you think I am?

You're a contributor, or at least you were pretending to be one. If you don't want to do a proper job is fine. But I'm not going to merge something that changes an API because you're lazy to do a good work.

So, I am going to use my own fork, and add some more features to it, such as lambda functions.

Open source is that, so feel free.

May you be interested, you can pick it up all by yourself and make any modifications to it you like. Such as adding Doxygen or whatever. Have a nice afternoon, Kind regards, Ondrej

Last thing, you show very little respect when you are asked to do changes. I hope you realize that compromising is a fundamental part of contribution process not only here but in private companies.

[francescmm]

BTW, @OndrejPopp later I'll amend the commit message of the QRexursiveMutex change giving you attribution. I think it's fair to do so since you saw that improvement.