FIX QtViewer._open_files_dialog handing of stack#7172
Conversation
|
From #7165 (comment):
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 ReportAll modified and coverable lines are covered by tests ✅
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. |
|
@DragaDoncila says it would be safer to keep the loop but we can add it back if problems 😅😅 |
|
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. |
|
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.) |
References and relevant issues
closes #7165
Description
Correctly handles
stackparameter in_open_files_dialog. This was due to a bad refactor in #4865I noticed that we don't test any of the
QtVieweropen_files...oropen_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?