ui: fixed rules views selection and refreshing issues

ui/opensnitch/customwidgets/generictableview.py

@@ -12,6 +12,7 @@
     QObject,
     pyqtSignal,
     QEvent,
+    QTimer,
     Qt)
 
 class GenericTableModel(QStandardItemModel):
@@ -35,6 +36,10 @@ class GenericTableModel(QStandardItemModel):
     prevQueryStr = ''
     # modified query object
     realQuery = QSqlQuery()
+    # set when the query changes, to force the next viewport refresh.
+    # Otherwise the view would keep displaying the rows of the previous
+    # query when the scrollbar is not at the top/bottom of the view.
+    forceNextRefresh = False
 
     items = []
     lastItems = []
@@ -169,6 +174,7 @@ def setQuery(self, q, db, binds=None, limit=None, offset=None):
 
         if self.prevQueryStr != self.origQueryStr:
             self.realQuery = tmpQuery
+            self.forceNextRefresh = True
 
         self.update_row_count()
         self.update_col_count()
@@ -201,6 +207,9 @@ def refreshViewport(self, scrollValue, maxRowsInViewport, force=False):
         force var will force a refresh if the scrollbar is at the top or bottom of the
         viewport, otherwise skip it to allow rows analyzing without refreshing.
         """
+        if self.forceNextRefresh:
+            force = True
+            self.forceNextRefresh = False
         if not force:
             return
 
@@ -325,6 +334,10 @@ def __init__(self, parent):
         # current selected rows
         self._rows_selection = set()
 
+        # tracking-column text of the current (focused) row, used to
+        # restore currentIndex after a viewport refresh
+        self._current_row_text = None
+
         # selection range to highlight the rows of the viewport, that is,
         # the rows of the current sql query (offset + limit).
         self._first_row_selected = None
@@ -388,14 +401,11 @@ def selectDbRows(self, first, last):
         if selrows is None:
             return
         self._rows_selection.clear()
-        for rid, row in enumerate(selrows):
-            key = row[self.trackingCol]
-            self._rows_selection.add(key)
-            idx = self.model().index(rid, self.trackingCol)
-            self.selectionModel().setCurrentIndex(
-                idx,
-                QItemSelectionModel.SelectionFlag.Rows | QItemSelectionModel.SelectionFlag.SelectCurrent
-            )
+        for row in selrows:
+            self._rows_selection.add(row[self.trackingCol])
+        # the visual selection of the visible rows is applied by
+        # selectIndices(); selecting db-range positions here would
+        # highlight wrong viewport rows when the range is scrolled.
         self.selectIndices()
 
     def getMinViewportRow(self):
@@ -509,8 +519,8 @@ def mouseMoveEvent(self, event):
     def mousePressEvent(self, event):
         # we need to call upper class to paint selections properly
         super().mousePressEvent(event)
-        self.mousePressed = True
         rightBtnPressed = event.button() != Qt.MouseButton.LeftButton
+        self.mousePressed = not rightBtnPressed
 
         self.keySelectAll = False
         if not self.shiftPressed:
@@ -522,11 +532,15 @@ def mousePressEvent(self, event):
         pos = event.pos()
         item = self.indexAt(pos)
         row = self.rowAt(pos.y())
-        if item is None:
-            return
 
         clickedItem = self.model().index(row, self.trackingCol)
-        if clickedItem.data() is None:
+        if not item.isValid() or clickedItem.data() is None:
+            # Qt clears the visual selection when pressing on an empty
+            # area; keep the tracked selection in sync, otherwise menu
+            # actions keep operating on rows no longer highlighted.
+            if not self.ctrlPressed:
+                self._rows_selection.clear()
+                self._current_row_text = None
             return
 
         flags = QItemSelectionModel.SelectionFlag.Rows | QItemSelectionModel.SelectionFlag.SelectCurrent
@@ -607,6 +621,8 @@ def mousePressEvent(self, event):
             clickedItem,
             flags
         )
+        isDeselect = bool(flags & QItemSelectionModel.SelectionFlag.Deselect)
+        self._current_row_text = None if isDeselect else clickedItem.data()
 
     def handleShiftPressed(self):
         # in the viewport, the rows start at 1, but in the db at 0
@@ -683,6 +699,7 @@ def clearSelection(self):
         self.selectionModel().reset()
         self.selectionModel().clearCurrentIndex()
         self._rows_selection.clear()
+        self._current_row_text = None
         self._first_row_selected = None
         self._last_row_selected = None
         self._db_selection_range = {
@@ -743,6 +760,44 @@ def selectIndices(self):
                 sel.append(QItemSelectionRange(i.index()))
         self.selectionModel().clear()
         self.selectionModel().select(sel, QItemSelectionModel.SelectionFlag.Select | QItemSelectionModel.SelectionFlag.Rows)
+        self._restoreCurrentIndex()
+
+    def _restoreCurrentIndex(self):
+        """Re-apply the current (focused) row after the viewport has been
+        refreshed. selectionModel().clear() drops currentIndex, so keyboard
+        navigation would lose its position on every refresh otherwise.
+        """
+        if self._current_row_text is None:
+            return
+        items = self.model().findItems(self._current_row_text, column=self.trackingCol)
+        if len(items) == 0:
+            return
+        self.selectionModel().setCurrentIndex(
+            items[0].index(),
+            QItemSelectionModel.SelectionFlag.NoUpdate
+        )
+
+    def _syncSelectionFromCurrentRow(self):
+        """Sync the tracked rows with the row that became current after Qt
+        processed a navigation key press. The view handles the key AFTER
+        our eventFilter runs, so reading currentIndex there returns the row
+        the user navigated AWAY from, leaving the tracked selection (and
+        thus context-menu actions) one row behind the visible selection.
+        """
+        if self.ctrlPressed:
+            # ctrl+navigation moves the current row without changing the
+            # selection
+            return
+        curIdx = self.selectionModel().currentIndex()
+        if not curIdx.isValid():
+            return
+        rowText = self.model().index(curIdx.row(), self.trackingCol).data()
+        if rowText is None:
+            return
+        if not self.shiftPressed:
+            self._rows_selection.clear()
+        self._rows_selection.add(rowText)
+        self._current_row_text = rowText
 
     def _selectLastRow(self):
         internalId = self.getCurrentIndex()
@@ -781,17 +836,12 @@ def onScrollbarValueChanged(self, vSBNewValue):
 
     def onKeyUp(self):
         curIdx = self.selectionModel().currentIndex()
-        if not self.shiftPressed:
-            self._rows_selection.clear()
-        self._rows_selection.add(curIdx.data())
-
         viewport_row = self.getViewportRowPos(curIdx.row())
         self._last_row_selected = viewport_row
         if self._first_row_selected is None:
             self._first_row_selected = viewport_row
 
         offset = self.model().queryOffset
-        limit = self.model().queryLimit
         if curIdx.row() == 0:
             self.vScrollBar.setValue(max(0, self.vScrollBar.value() - 1))
         if curIdx.row() == 0 and viewport_row+offset-1 == offset:
@@ -800,21 +850,21 @@ def onKeyUp(self):
     def onKeyDown(self):
         curIdx = self.selectionModel().currentIndex()
         curRow = curIdx.row()
-        if not self.shiftPressed:
-            self._rows_selection.clear()
-        self._rows_selection.add(curIdx.data())
+        viewport_row = self.getViewportRowPos(curRow)
 
         viewport_row = self.getViewportRowPos(curRow)
         newValue = self.vScrollBar.value()
 
-        offset = self.model().queryOffset
         limit = self.model().queryLimit
         if curRow >= self.maxRowsInViewport-2:
             # this change will fire onScrollbarValueChanged, which will refresh the
             # view (the rows and the rows numbers)
             self.vScrollBar.setValue(newValue+1)
             self._selectLastRow()
-        if (offset == 0 and viewport_row == limit) or viewport_row+offset == limit+offset:
+        # wrap the selection to the first row after paginating to the next
+        # records window. The query offset cancels out on both sides of the
+        # comparison, so checking against the limit alone is enough.
+        if viewport_row == limit:
             self._selectRow(0)
 
     def onKeyHome(self):
@@ -884,12 +934,16 @@ def eventFilter(self, obj, event):
             # some pyqt versions.
             if event.key() == Qt.Key.Key_Up:
                 self.onKeyUp()
+                QTimer.singleShot(0, self._syncSelectionFromCurrentRow)
             elif event.key() == Qt.Key.Key_Down:
                 self.onKeyDown()
+                QTimer.singleShot(0, self._syncSelectionFromCurrentRow)
             elif event.key() == Qt.Key.Key_Home:
                 self.onKeyHome()
+                QTimer.singleShot(0, self._syncSelectionFromCurrentRow)
             elif event.key() == Qt.Key.Key_End:
                 self.onKeyEnd()
+                QTimer.singleShot(0, self._syncSelectionFromCurrentRow)
             elif event.key() == Qt.Key.Key_PageUp:
                 self.onKeyPageUp()
             elif event.key() == Qt.Key.Key_PageDown:

ui/opensnitch/dialogs/events/menu_actions.py

@@ -372,6 +372,11 @@ def table_menu_edit(self, cur_idx, model, selection):
                         QtWidgets.QMessageBox.Icon.Warning)
                     return
                 print(node, name)
+                # the editor runs its own event loop, and table refreshes
+                # are discarded while the context menu is flagged as
+                # active, so saving from the editor wouldn't update the
+                # views otherwise.
+                self.set_context_menu_active(False)
                 r = RulesEditorDialog(modal=False)
                 r.edit_rule(records, node)
 
@@ -386,6 +391,9 @@ def table_menu_edit(self, cur_idx, model, selection):
                             QC.translate("stats", "Rule not found by that name and node"),
                             QtWidgets.QMessageBox.Icon.Warning)
                     return
+                # see the TAB_MAIN branch: allow table refreshes while the
+                # editor is open.
+                self.set_context_menu_active(False)
                 r = RulesEditorDialog(modal=False)
                 r.edit_rule(records, node)
                 break

ui/opensnitch/dialogs/events/menus.py

@@ -1,3 +1,5 @@
+import traceback
+
 from PyQt6 import QtCore, QtWidgets, QtGui
 from PyQt6.QtCore import QCoreApplication as QC
 
@@ -251,6 +253,8 @@ def configure_rules_contextual_menu(self, pos):
             model = table.model()
 
             selection = table.selectedRows()
+            if not selection:
+                return False
 
             menu = QtWidgets.QMenu()
             durMenu = QtWidgets.QMenu(self.COL_STR_DURATION)
@@ -362,10 +366,11 @@ def configure_rules_contextual_menu(self, pos):
             elif action == _toDisk:
                 self.table_menu_export_disk(cur_idx, model, selection)
 
+            return True
         except Exception as e:
             print("rules contextual menu exception:", e)
-        finally:
-            return True
+            traceback.print_exc()
+            return False
 
     def configure_alerts_contextual_menu(self, pos):
         try:

ui/opensnitch/dialogs/ruleseditor/dialog.py

@@ -271,15 +271,17 @@ def cb_save_clicked(self):
         if self._old_rule_name is not None and self._old_rule_name != self.rule.name:
             self.delete_rule()
 
-        self._old_rule_name = rule_name
+        # use the saved rule name, not the one typed in the field: save_rule()
+        # may rename the rule (e.g. when the action of an auto-named rule
+        # changes). Otherwise _old_rule_name would lag behind and the next
+        # save would wrongly report a name conflict with the just saved rule.
+        self._old_rule_name = self.rule.name
 
         # after adding a new rule, we enter into EDIT mode, to allow further
         # changes without closing the dialog.
         if constants.WORK_MODE == constants.ADD_RULE:
             constants.WORK_MODE = constants.EDIT_RULE
 
-        self._rules.updated.emit(0)
-
     @QtCore.pyqtSlot(str, ui_pb2.NotificationReply)
     def cb_notification_callback(self, addr, reply):
         #print(self.LOG_TAG, "Rule notification received: ", reply.id, reply.code)
@@ -390,8 +392,6 @@ def delete_rule(self):
             # if the rule name has changed, we need to remove the old one
             if self._old_rule_name != self.rule.name:
                 node = nodes.get_node_addr(self)
-                old_rule = self.rule
-                old_rule.name = self._old_rule_name
                 if self.nodeApplyAllCheck.isChecked():
                     nid, noti = self._nodes.delete_rule(rule_name=self._old_rule_name, addr=None, callback=self._notification_callback)
                     self.notifications_sent[nid] = noti

ui/opensnitch/rules.py

@@ -94,7 +94,7 @@ def __init__(self):
         QObject.__init__(self)
         self._db = Database.instance()
 
-    def add(self, time, node, name, description, enabled, precedence, nolog, action, duration, op_type, op_sensitive, op_operand, op_data, created):
+    def add(self, time, node, name, description, enabled, precedence, nolog, action, duration, op_type, op_sensitive, op_operand, op_data, created, notify=True):
         # don't add rule if the user has selected to exclude temporary
         # rules
         if duration in Config.RULES_DURATION_FILTER:
@@ -104,6 +104,8 @@ def add(self, time, node, name, description, enabled, precedence, nolog, action,
                   "(time, node, name, description, enabled, precedence, nolog, action, duration, operator_type, operator_sensitive, operator_operand, operator_data, created)",
                   (time, node, name, description, enabled, precedence, nolog, action, duration, op_type, op_sensitive, op_operand, op_data, created),
                         action_on_conflict="REPLACE")
+        if notify:
+            self.updated.emit(0)
 
     def add_rules(self, addr, rules):
         try:
@@ -123,8 +125,12 @@ def add_rules(self, addr, rules):
                          r.operator.type,
                          str(r.operator.sensitive),
                          r.operator.operand, r.operator.data,
-                         str(datetime.fromtimestamp(r.created).strftime(DBDateFieldFormat)))
+                         str(datetime.fromtimestamp(r.created).strftime(DBDateFieldFormat)),
+                         notify=False)
 
+            # notify once per batch, to avoid a refresh storm when a node
+            # connects and sends all its rules.
+            self.updated.emit(0)
             return True
         except Exception as e:
             log.warning("exception adding node rules to db: %s", repr(e))
@@ -142,6 +148,7 @@ def delete(self, name, addr, callback):
         if not self._db.delete_rule(rule.name, addr):
             return None
 
+        self.updated.emit(0)
         return rule
 
     def delete_by_field(self, field, values):
@@ -184,6 +191,7 @@ def disable(self, addr, name):
             "name=? AND node=?",
             action_on_conflict="OR REPLACE"
         )
+        self.updated.emit(0)
 
     def update_time(self, time, name, addr):
         """Updates the time of a rule, whenever a new connection matched a

ui/opensnitch/service.py

@@ -23,6 +23,7 @@
 from opensnitch.notifications import DesktopNotifications
 from opensnitch.firewall import Rules as FwRules
 from opensnitch.nodes import Nodes
+from opensnitch.rules import Rules
 from opensnitch.config import Config
 from opensnitch.version import version
 from opensnitch.database import Database
@@ -129,6 +130,7 @@ def __init__(self, app, on_exit, start_in_bg=False):
 
         self._nodes = Nodes.instance()
         self._nodes.reset_status()
+        self._rules = Rules.instance()
 
         self._last_stats = {}
         self._last_items = {
@@ -898,7 +900,9 @@ def _disable_temp_rule(args):
                 ost.start()
 
         elif kwargs['action'] == self.DELETE_RULE:
-            self._db.delete_rule(kwargs['name'], kwargs['addr'])
+            # route it through Rules, so the views are notified of the
+            # change.
+            self._rules.delete(kwargs['name'], kwargs['addr'], None)
 
         elif kwargs['action'] == self.NODE_DELETE:
             self._delete_node(kwargs['peer'])