Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Develop to Master merge #100

Open
wants to merge 251 commits into
base: master
Choose a base branch
from
Open

Develop to Master merge #100

wants to merge 251 commits into from

Conversation

iwanlevin
Copy link
Contributor

Merge develop to master

iwanlevin and others added 30 commits November 22, 2021 16:36
`POSTGRES_HOST_AUTH_METHOD: trust` allows all connections to be established without needing the password. Removing this before I forget about it.
Copy link
Contributor

@cppcooper cppcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works, and is good.. but I think a few of these things should be addressed before merging to master. Most are fine as is.

Comment on lines 2 to 8
<project version="4">
<component name="ProjectModuleManager">
<modules>
<module fileurl="file://$PROJECT_DIR$/.idea/Orthanc-TMI.iml" filepath="$PROJECT_DIR$/.idea/Orthanc-TMI.iml" />
<module fileurl="file://$PROJECT_DIR$/.idea/Orthanc_TMI.iml" filepath="$PROJECT_DIR$/.idea/Orthanc_TMI.iml" />
</modules>
</component>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how any .idea/ files got in. I thought the directory was ignored in .gitignore.

We should delete this file and any others in there.

CMakeLists.txt Show resolved Hide resolved

ADD ./setup-environment.sh ./setup-environment.sh
RUN dos2unix ./setup-environment.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surely there is a better solution

Comment on lines +17 to 36
"DataAnon": {
"HardlinksUseHashBins" : true,
"Hardlinks": {
"/by-study-date/": "0008,0020",
"/by-pid/": "0010,0020",
"/by-dob/": "0010,0030"
},
"DateTruncation": {
"default": "YYYYMM01",
"0010,0030": "YYY0MM01"
},
"Filter": {
"blacklist": [
"0010"
],
"whitelist": [
"0010,0030"
]
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have been nice to move into its own config file. Then we could have detected it's presence and created a default file if missing.

include/dicom/dicom-element-view.h Show resolved Hide resolved
Comment on lines +23 to +26
static void Emplace(const void* instance_data, std::string md5, size_t size);
static void Emplace(const void* instance_data, std::string uuid);
static void Emplace(const void* instance_data, const DicomFile& file);
static bool Emplace(std::string md5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These in particular really need better names.. I think. I think?

Comment on lines +1 to +24
#pragma once
/*
#include <mutex>
#include <atomic>
#include <queue>
#include <condition_variable>
#include <functional>

class JobQueue {
private:
std::atomic_bool keep_running;
std::atomic_bool has_work;
std::condition_variable cv;
std::mutex queue_lock;
std::queue<std::function<void()>> jqueue;
// this class is the only thing that can instantiate itself now
JobQueue(){}
public:
static JobQueue& GetInstance();
void AddJob(std::function<void()> job);
void Process();
void Stop();
};
/**/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two files should be deleted before merging to master

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually 3 (.h .cpp, and the unit test .cpp)

Comment on lines +47 to +52
db_init_job = std::thread([](){
std::this_thread::sleep_for(std::chrono::seconds(2));
if(!DBInterface::Initialize()){
std::cerr << "DB Initialize job failed. Terminating.." << std::endl;
std::abort(); // abnormal termination
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented. It's there because Orthanc's tables haven't yet initialized (potentially) yet and so we can't initialize ours yet. But we need it to initialize from our plugin initialize function, so we spawn a thread with a delay. A comment should explain this

@@ -0,0 +1,241 @@
CREATE TABLE IF NOT EXISTS crosswalk (
Copy link
Contributor

@cppcooper cppcooper Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an ideal world, we break this file up into smaller portions and load those files instead of copy pasting the code in this file as string in numerous locations.

//#define UNIT_TEST
#include <core.h>
#include <functional>

extern fs::path GetProjRoot();
extern void TestWithDicomFiles(std::function<void(const fs::path&)> test);
namespace uuid {
extern std::string generate_uuid_v4();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this function is used. It should be removed if that's the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants