Skip to content

FIX QtViewer._open_files_dialog handing of stack#7172

Merged
jni merged 2 commits intonapari:mainfrom
lucyleeow:fix_open_stack
Aug 9, 2024
Merged

FIX QtViewer._open_files_dialog handing of stack#7172
jni merged 2 commits intonapari:mainfrom
lucyleeow:fix_open_stack

Conversation

@lucyleeow
Copy link
Copy Markdown
Contributor

References and relevant issues

closes #7165

Description

Correctly handles stack parameter in _open_files_dialog. This was due to a bad refactor in #4865

I noticed that we don't test any of the QtViewer open_files... or open_folder... methods. (Some are tested with the file menu but none of these seem to be tested in isolation?)
I've added a new test file for these but only added a test for this instance. Maybe additional tests can be added in a follow up PR, in order to get this fix in quicker?

@lucyleeow lucyleeow added the bug Something isn't working as expected label Aug 9, 2024
@github-actions github-actions bot added tests Something related to our tests qt Relates to qt labels Aug 9, 2024
@lucyleeow
Copy link
Copy Markdown
Contributor Author

From #7165 (comment):

Why is there a for-loop at all?

I see the for loop was added in #4347 here https://github.com/napari/napari/pull/4347/files#diff-1807f13d5d575e770c4b24ce95223949ab74d5492ec5eb1a4f71adfcf2e2531c @DragaDoncila any recollection as to why?

I've removed the loop but happy to add it back...

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (9a9e7e8) to head (6859c71).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7172      +/-   ##
==========================================
- Coverage   93.01%   92.92%   -0.10%     
==========================================
  Files         619      620       +1     
  Lines       56757    56762       +5     
==========================================
- Hits        52792    52744      -48     
- Misses       3965     4018      +53     

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

Copy link
Copy Markdown
Collaborator

@Czaki Czaki left a comment

Choose a reason for hiding this comment

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

Works for me

@Czaki Czaki added this to the 0.5.2 milestone Aug 9, 2024
@Czaki Czaki added ready to merge Last chance for comments! Will be merged in ~24h bugfix PR with bugfix and removed bug Something isn't working as expected labels Aug 9, 2024
@jni jni merged commit 72d0de0 into napari:main Aug 9, 2024
@github-actions github-actions bot removed the ready to merge Last chance for comments! Will be merged in ~24h label Aug 9, 2024
@lucyleeow lucyleeow deleted the fix_open_stack branch August 9, 2024 09:34
@lucyleeow
Copy link
Copy Markdown
Contributor Author

#7172 (comment)

@DragaDoncila says it would be safer to keep the loop but we can add it back if problems 😅😅

@DragaDoncila
Copy link
Copy Markdown
Contributor

I'm just concerned that it's been there for a while now and I won't have time to properly look at this PR and the one that introduced it until I'm back, which won't be until Friday next week at the absolute earliest. If the PR without changing the loop is confirmed to fix the reported issue, I'd prefer to keep this PR to very minimal changes.

@jni
Copy link
Copy Markdown
Member

jni commented Aug 9, 2024

I think this PR is the minimal change.

Anyway if anything else breaks with 0.5.2 you'll be back just in time to fix for 0.5.3. 😂 (Less flippantly: with our faster release cycle bugs are less critical, this one closes an existing bug and adds a regression test for it, so we can fix any new bugs more confidently in the future. And I don't think this will introduce new bugs, I think it's the right way to think about this — find a single path through the code, rather than add even more branches at different levels.)

jni pushed a commit that referenced this pull request Aug 12, 2024
…and `False` (#7176)

# References and relevant issues
Follow up to #7172

# Description

Parametrizes test to check for stack True and False. I forgot to add
this to the original PR, sorry for the extra noise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix PR with bugfix qt Relates to qt tests Something related to our tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File -> Open image as stack broken from Napari 0.4.19 onwards.

4 participants