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

Write channel id and name to display settings nodes #106

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

soyers
Copy link
Contributor

@soyers soyers commented Aug 8, 2024

Description

This PR extends libczi to set the Id and Name attributes in the DisplaySettings of the image metadata. For more informatione please see the description of the corresponding issue.

Fixes #105

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Ths PR adds a unit test. Furthermore the changes were tested with a developer build of pylibczirw and manually verifying that the resulting image is loaded in ZEN as expected.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 66.18%. Comparing base (4a60e22) to head (3b9b522).
Report is 1 commits behind head on main.

Files Patch % Lines
Src/libCZI/CziMetadataBuilder.cpp 98.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #106      +/-   ##
==========================================
+ Coverage   66.01%   66.18%   +0.16%     
==========================================
  Files          85       85              
  Lines       10664    10705      +41     
==========================================
+ Hits         7040     7085      +45     
+ Misses       3624     3620       -4     
Flag Coverage Δ
windows-latest 66.18% <98.07%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@soyers soyers requested a review from ptahmose August 9, 2024 10:03
@ptahmose
Copy link
Contributor

bumping the version-number (in CMakeLists.txt) and an entry in version-history,markdown would be missing.

@ptahmose ptahmose added the cla Contributor License Agreement sent to Admin label Aug 12, 2024
* when adding the display-settings with the metadata-builder, by default copy the attributes "Id" and "Name" from the corresponding channel in the channels-node
* add an overload of MetadataUtils::WriteDisplaySettings which allows to control what is put into the respective "Id" and "Name" attributes
@ptahmose
Copy link
Contributor

I added a proposal for a revised version. The idea here is:

  • When adding the Display-Settings (with MetadataUtils::WriteDisplaySettings), we check the channel-nodes and copy their Id- and Name-attributes if they exist.
  • This "copy" happens by default with the existing MetadataUtils::WriteDisplaySettings functions. In addition, there is a new overload which allows to control explicitly what is put into the attributes.
  • Adapted and added unit-tests.

The line `std::tuple<std::string, std::tuple<bool, std::string>> channelIdAndName;` has been removed from the loop that processes channel display settings. This indicates that the variable `channelIdAndName` is no longer necessary or its usage has been refactored out of this section of the code.
@ptahmose ptahmose requested a review from a team August 19, 2024 12:34
@ptahmose ptahmose merged commit 87c06cb into main Aug 19, 2024
15 checks passed
@ptahmose ptahmose deleted the ssoyer/id_and_name_in_ds branch August 21, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla Contributor License Agreement sent to Admin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DisplaySettings meta data do not contain channel Id and Name
3 participants