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.

Sign up for GitHub

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

Jump to bottom

Visual dicom browser enhs #1217

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Visual dicom browser enhs #1217

merged 2 commits into from
Sep 9, 2024

Conversation

Punzo
Copy link
Contributor

@Punzo Punzo commented Aug 30, 2024 •
edited
Loading

New PR for visual DICOM browser enhancements. This is a work in progress.

@Punzo Punzo marked this pull request as draft August 30, 2024 11:17
This was referenced Aug 30, 2024
Merged
Open
@Punzo Punzo changed the title Visual dicom browser enh Visual dicom browser enhs Aug 30, 2024
@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 6 times, most recently from ae76be0 to 54cbfbe Compare September 2, 2024 16:32
@Punzo Punzo marked this pull request as ready for review September 2, 2024 16:34
@Punzo
Copy link
Contributor Author

Punzo commented Sep 2, 2024

@lassoan this is ready for review.

@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 4 times, most recently from 60cd97f to 6b0df27 Compare September 6, 2024 13:40
@Punzo
ENH: Refactor and clean up manual retry infrastructure
8afb5c3 
ENH: Add job status logging to the detail logging window
ENH: Enable thumbnail generation in worker processes
BUG: Fix missing update of server settings UI
BUG: Add missing Python wrapping methods for ctkJobDetail
ENH: Enhance job logging for better traceability
PERF: Optimize UI updates by directly calling component slots
lassoan
lassoan reviewed Sep 6, 2024
View reviewed changes
Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Looks very nice, I've added mostly trivial comments.

Libs/Core/ctkAbstractJob.cpp Outdated Show resolved Hide resolved
Libs/Core/ctkAbstractJob.h Outdated Show resolved Hide resolved
Libs/Core/ctkAbstractJob.h Outdated Show resolved Hide resolved
Libs/Core/ctkCorePythonQtDecorators.h Outdated Show resolved Hide resolved
Libs/Core/ctkJobScheduler.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp Outdated Show resolved Hide resolved
Libs/DICOM/Widgets/ctkDICOMVisualBrowserWidget.cpp Outdated Show resolved Hide resolved
@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 4 times, most recently from 6277fe5 to 6a6b99a Compare September 9, 2024 12:21
@Punzo @lassoan
ENH: Apply Review
a18e66c 
Co-authored-by: Andras Lasso <[email protected]>
lassoan
lassoan approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Thank you, it looks good to me.

@lassoan lassoan merged commit f3b6cb6 into commontk:master Sep 9, 2024
4 checks passed
@Punzo
Copy link
Contributor Author

Punzo commented Sep 9, 2024

Thank you, it looks good to me.

hei @lassoan that was not a final commit. I am sorry, I should have marked the PR as draft as I was working on it.

I just finished now and I have fixed also the perfomances issue (#1162 (comment))

Can you revert this PR/merge, please? I have opened a new PR at #1218.

@lassoan
Copy link
Member

lassoan commented Sep 9, 2024

I cannot revert, sorry, I'm not an admin in this repository. Could you make all changes on top of the latest main version in your pull request?

@Punzo
Copy link
Contributor Author

Punzo commented Sep 9, 2024 •
edited
Loading

I cannot revert, sorry, I'm not an admin in this repository. Could you make all changes on top of the latest main version in your pull request?

not really, there are too many conflicts. Maybe @jcfr can help?
Again sorry, next time I will remember to mark it as draft while working on it.

@Punzo
Copy link
Contributor Author

Punzo commented Sep 9, 2024 •
edited
Loading

@jcfr can yoiu please revert commit f3b6cb6 ?

this is the cleaned PR to be merged #1218

@lassoan
Copy link
Member

lassoan commented Sep 9, 2024

It is fine if automatic rebase does not work, as we know that there are no other changes in the main branch. You can simply checkout the latest main version, overwrite the files with the desired content, commit, and push.

@Punzo
Copy link
Contributor Author

Punzo commented Sep 9, 2024

It is fine if automatic rebase does not work, as we know that there are no other changes in the main branch. You can simply checkout the latest main version, overwrite the files with the desired content, commit, and push.

OK i will see to fix the conflicts in #1218

@Punzo
Copy link
Contributor Author

Punzo commented Sep 9, 2024

ok done in 06bd7dc

@Punzo Punzo mentioned this pull request Sep 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lassoan lassoan lassoan approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Punzo @lassoan