[FIX] Select Rows: Keep condition rows in sync after removing one

[raashish1601]

Issue

Fixes #7333

Description of changes

Each condition's variable and operator combos remember their table row in a row attribute, set once in add_row. set_new_operators and set_new_values use it to find the cells to replace. When a condition is removed, the rows below move up but their combos keep the old number, so changing one of them rebuilds the widgets of the next line (or a line that no longer exists).

remove_one_row now updates row on the variable and operator combos of the rows that moved up. The remove button already used a QPersistentModelIndex, so it was not affected.

Tests: test_remove_condition_keeps_rows adds three conditions, removes the first, and then changes the variable of the new first row and the operator of the second row, checking that each change stays on its own line. It fails without the fix (on my machine the unpatched widget even crashes the test process) and passes with it. The whole test_owselectrows.py passes locally (run on Windows against the Orange 3.40 wheel with this patch applied to owselectrows.py).

Includes

• Code changes
• Tests
• Documentation

[krystofair]

It seems to be "hacky" solution.

This function is invoked as single responsibility to delete row - encapsulating for self.cond_list.removeRow and for no other conditions to disable button.

There is self.remove_one which is connected to button, and then you have self.conditions_changed where logic you want to fixed is done. So the place for this change is imo wrong - should be, metioned above, self.condition_changed.

[krystofair]

That was merely "seems" :) — this was my first look.
After some deeper analysis, there is no better place.
On my Linux machine pytest crashed before patch too.

btw: Changing row only on col = 0 works too (pass tests for this widget), at least until some code will not use oper_combo.row attribute.

[raashish1601]

Thanks for looking deeper. The crash before the patch matches what I see: on the unpatched widget the test process dies too. Only updating column 0 isn't enough, though: set_new_values places the value widgets with oper_combo.row, so changing the operator of a moved row would still rebuild the values in the wrong line. I extended the test to switch that row to "is between" and check it gets two inputs; with only column 0 updated it crashes again, with both it passes.

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.98%. Comparing base (eea7a75) to head (73b126f).
⚠️ Report is 5 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7335   +/-   ##
=======================================
  Coverage   88.98%   88.98%           
=======================================
  Files         337      337           
  Lines       74598    74604    +6     
=======================================
+ Hits        66378    66390   +12     
+ Misses       8220     8214    -6     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[raashish1601]

The "Scientific Python nightly wheels" failure is unrelated to this change: the nightly pyqtgraph turned LegendItem.layout into a method, so hundreds of widget tests error out, while the Select Rows tests all pass in that job. Master fails the same way (https://github.com/biolab/orange3/actions/runs/37914329734).

[janezd]

I agree with @krystofair that this is a hacky solution, but the problem is that underlying code was already hacky. I added another commit that uses persistent model indices instead of rows; this eliminates the need to update because Qt does it for us. Furthermore, this allows putting them into callback function's closure instead of having to store them as attributes.

@raashish1601, thanks for tests (and, of course, for reporting the problem and a fix that helped toward a more persistent fix.)

@VesnaT, could you please review this some time next week?