Skip to content

Conversation

@po-nuvai
Copy link

@po-nuvai po-nuvai commented Jan 4, 2026

Summary

Completes the fix for #1428 - Shift+click doesn't select multiple items in tables

Building on the partial implementation from commit 427b37c, this PR addresses the TODO mentioned by @gustavo-iniguez-goya:

The problem is that we only render the rows that are visible on screen. So if we select for example row 4 with 50 visible rows, and then scroll down to row 100, we no longer have previous rows in memory (that is, the data below row 80), only last 50.

Root Cause

The existing implementation stored viewport row indices (0 to maxRowsInViewport-1) for Shift+click selection. When the user scrolled between the first click and the Shift+click, these indices became meaningless because the viewport now showed different database rows.

Solution

  1. Store absolute database row indices instead of viewport indices:

    • Absolute index = vScrollBar.value() + viewport_row
    • Added _viewportToAbsoluteRow() helper method
  2. Fetch rows directly from database for the selection range:

    • Added getRowsInRange(startRow, endRow, trackingCol) to GenericTableModel
    • This seeks directly to the start position in the database query and reads the requested rows
    • Works regardless of what's currently visible in the viewport
  3. Updated selection handling:

    • mousePressEvent now stores absolute row positions
    • handleShiftPressed() uses database query instead of trying to read from viewport

Example Flow

  1. User clicks row 4 → _first_row_selected = 4 (absolute)
  2. User scrolls down 100 rows
  3. User Shift+clicks row 10 in viewport → _last_row_selected = 110 (absolute = 100 + 10)
  4. handleShiftPressed() calls model.getRowsInRange(4, 110, trackingCol)
  5. All 107 rows are fetched from database and stored in _rows_selection

Test Plan

  • Click on row 4 in Rules tab
  • Scroll down so row 4 is no longer visible
  • Shift+click on row 100
  • Verify all rows 4-100 are selected (check via right-click menu or copy operation)
  • Scroll back up - selected rows should remain highlighted as they become visible
  • Test selection from bottom to top (Shift+click above first selection)
  • Test normal click-and-drag selection still works
  • Test Ctrl+click for individual selection still works

🤖 Generated with Claude Code

@gustavo-iniguez-goya
Copy link
Collaborator

hey @po-nuvai ,

You're mixing up many things here. daemon code and UI code. ¿?

@po-nuvai
Copy link
Author

po-nuvai commented Jan 4, 2026

@gustavo-iniguez-goya You're right, apologies for the confusion!

The branch picked up unrelated commits from my local master. The only commit that should be in this PR is the last one:

  • 53cdc3d - fix(ui): complete Shift+click selection with virtual scrolling

I'll recreate this PR with a clean branch containing only the shift+click fix. Sorry for the noise!

The existing implementation stored viewport row indices for Shift+click
selection, which broke when the user scrolled between clicks since only
visible rows are kept in memory with virtual scrolling.

Changes:
- Added getRowsInRange() to GenericTableModel to fetch rows directly from
  the database by absolute position, regardless of what's in the viewport
- Added _viewportToAbsoluteRow() helper to convert viewport indices to
  absolute database row indices
- Updated mousePressEvent to store absolute row indices
- Updated handleShiftPressed() to use database query instead of viewport data

This allows selecting rows like 1-100 even when only ~50 rows fit in the
viewport at a time.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@po-nuvai po-nuvai force-pushed the fix/shift-select-1428 branch from 53cdc3d to b25afdb Compare January 4, 2026 11:21
@po-nuvai
Copy link
Author

po-nuvai commented Jan 4, 2026

Done! I've rebased the PR to only include the single shift+click fix commit:

The PR now only modifies ui/opensnitch/customwidgets/generictableview.py with the virtual scrolling selection fix.

@gustavo-iniguez-goya
Copy link
Collaborator

Have you manually tested these changes?

@po-nuvai
Copy link
Author

po-nuvai commented Jan 4, 2026

@gustavo-iniguez-goya No, I haven't manually tested these changes. I analyzed the existing code and the issue description to understand the problem and develop the fix, but I don't have OpenSnitch running to verify.

I understand this is a concern - would you prefer I close these PRs until someone can properly test them? Or if you have time to test, I'd appreciate any feedback on whether the approach is sound.

The logic is:

  1. Store vScrollBar.value() + viewportRow instead of just viewportRow
  2. Query database directly with getRowsInRange() instead of reading from viewport memory

But I recognize that untested code is a risk, especially for UI interactions.

@po-nuvai po-nuvai closed this by deleting the head repository Jan 4, 2026
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.

2 participants