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

Labeling GUI: Copy colortable #2786

Merged
merged 1 commit into from Jan 18, 2024

Conversation

btbest
Copy link
Contributor

@btbest btbest commented Dec 15, 2023

Not copying the list means that when a value is changed (in this case, by the user selecting another color in the ColorDialog), the original list in the colortables module import is modified. This persists until ilastik is restarted.

Fixes #2728

Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5116115) 54.95% compared to head (6068654) 54.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2786      +/-   ##
==========================================
+ Coverage   54.95%   54.96%   +0.01%     
==========================================
  Files         530      530              
  Lines       62227    62226       -1     
  Branches     8503     8503              
==========================================
+ Hits        34197    34204       +7     
+ Misses      26271    26269       -2     
+ Partials     1759     1753       -6     

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

Copy link
Contributor

@k-dominik k-dominik left a comment

Choose a reason for hiding this comment

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

I guess that is the shortest route for a fix one could take!

I would honestly prefer to make all the mutable colormap constants in volumina.colortables private and do the copying in some create_COLORTABLENAME functions, just to make it harder to run into such an issue again. What do you think @btbest

@btbest
Copy link
Contributor Author

btbest commented Jan 5, 2024

I agree with the volumina change. In the meantime, I found the bug here is even sillier than I realised. There's already self._colorTable16 = list(colortables.default16_new) literally 20 lines above, which was overwritten by the line that I'm now "fixing" in this PR 🤦 When the fix is just to remove that line...

So step 1, remove that line and merge. Step 2, fix volumina exports and release. Step 3, dependency bump and replace the usages in ilastik.

The overwrite in line 184 was buggy as it didn't copy the colortable imported from volumina.

Fixes ilastik#2728
@btbest btbest force-pushed the avoid-persisting-label-color-change branch from c4622e7 to 6068654 Compare January 5, 2024 10:07
@btbest btbest merged commit e9217d3 into ilastik:main Jan 18, 2024
16 checks passed
@btbest btbest deleted the avoid-persisting-label-color-change branch January 18, 2024 13:09
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.

Changing label drawing color preserved during active sessing between projects
2 participants