search

CBICA / CaPTk2.0

Other
5 stars 5 forks source link

Feature17/more roi construction3 #90

Closed dboun closed 4 years ago

dboun commented 4 years ago

Resolves #17

dboun commented 4 years ago

where will the std::cout show in case of gui usage?

Nowhere.. (but when developing it will show in the terminal)

Also we cant be writing our own logger. better to use mitk logger.

We won't be writing our own logger of course. For the code that I've written, the logs go to MITK_INFO. This is code I copied from the "legacy" captk. After our talks in the meetings, it was pointed out to not change the legacy code too much it hopes to keep the codebases compatible. In the spirit of this I opted to not use MITK_INFO inside this code (or any other MITK function for that matter). Again, only inside the legacy code.

Also, in this particular case you saw an edit because I changed std::cerr to std::cout because the message was not an error, I'm not sure why it was like that in the first place

Although, as I said it would be trivial to add MITK logs inside it. If that's what you want, let me know and I will update accordingly.

ashishsingh18 commented 4 years ago

where will the std::cout show in case of gui usage?

Nowhere.. (but when developing it will show in the terminal)

Also we cant be writing our own logger. better to use mitk logger.

We won't be writing our own logger of course. For the code that I've written, the logs go to MITK_INFO. This is code I copied from the "legacy" captk. After our talks in the meetings, it was pointed out to not change the legacy code too much it hopes to keep the codebases compatible. In the spirit of this I opted to not use MITK_INFO inside this code (or any other MITK function for that matter). Again, only inside the legacy code.

Also, in this particular case you saw an edit because I changed std::cerr to std::cout because the message was not an error, I'm not sure why it was like that in the first place

Although, as I said it would be trivial to add MITK logs inside it. If that's what you want, let me know and I will update accordingly.

Please add the MITK_INFO. Logging is always good. The code bases will diverge since we will be using many things from MITK. You have anyways created a new class.

ashishsingh18 commented 4 years ago

Please also move the functions definitions from .h to .cpp/.hxx. I pointed one instance of this in my earlier comments.

dboun commented 4 years ago

(I'm aware of the issues of this PR, I will deal with it Tuesday+)

ashishsingh18 commented 4 years ago

Okay.

dboun commented 4 years ago

(@ashishsingh18 everything here is addressed)