From c4a9ce8b459e37eaef77c48abf589a83de0e64ca Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Fri, 20 Dec 2024 15:56:31 -0500 Subject: [PATCH 1/6] Add current_loaction_label and update table on rows/page changed --- .../PyMcaGui/io/QTiledCatalogSelectorDialog.py | 16 +++++++++++++++- PyMca5/PyMcaGui/io/TiledCatalogSelector.py | 11 ++++++++++- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py index 21cbc8fc3..ed60883ea 100644 --- a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py +++ b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py @@ -218,6 +218,15 @@ def reset_rows_per_page(self) -> None: ) self.rows_per_page_selector.setCurrentIndex(self.model._rows_per_page_index) + def _set_current_location_label(self): + starting_index = self.model._current_page * self.model.rows_per_page() + 1 + ending_index = min( + self.model.rows_per_page() * (self.model._current_page + 1), + len(self.model.get_current_node()), + ) + current_location_text = f"{starting_index}-{ending_index} of {len(self.model.get_current_node())}" + self.current_location_label.setText(current_location_text) + def populate_table(self): original_state = {} # TODO: may need if condition if we implement a disconnect button @@ -235,7 +244,7 @@ def populate_table(self): self.catalog_table.setItem(0, 0, self.catalog_breadcrumbs) # Then add new rows - rows_per_page = self.model._rows_per_page_options[self.model._rows_per_page_index] + rows_per_page = self.model.rows_per_page() for _ in range(rows_per_page): last_row_position = self.catalog_table.rowCount() self.catalog_table.insertRow(last_row_position) @@ -337,8 +346,12 @@ def on_client_connection_error(error_msg: str): @self.model.table_changed.connect def on_table_changed(node_path_parts: Tuple[str]): _logger.debug(f"on_table_changed(): {node_path_parts = }") + if self.model.client is None: + # TODO: handle disconnecting from tiled client later + return self.populate_table() self._rebuild_current_path_layout() + self._set_current_location_label() @self.model.url_validation_error.connect def on_url_validation_error(error_msg: str): @@ -358,6 +371,7 @@ def connect_model_slots(self) -> None: self.connect_button.clicked.connect(model.on_connect_clicked) self.next_page.clicked.connect(model.on_next_page_clicked) self.previous_page.clicked.connect(model.on_prev_page_clicked) + self.rows_per_page_selector.currentIndexChanged.connect(self.model.on_rows_per_page_changed) def connect_self_signals(self): # TODO find another way to do this? diff --git a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py index 436b20a05..913fbed01 100644 --- a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py +++ b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py @@ -198,13 +198,22 @@ def on_item_selected(self, child_node_path): else: self.load_button_enabled = False + def on_rows_per_page_changed(self, index): + self._rows_per_page_index = index + self._current_page = 0 + self.table_changed.emit(self.node_path_parts) + + # TODO: make this a property + def rows_per_page(self): + return self._rows_per_page_options[self._rows_per_page_index] + def on_prev_page_clicked(self): if self._current_page != 0: self._current_page -= 1 self.table_changed.emit(self.node_path_parts) def on_next_page_clicked(self): - rows_per_page = self._rows_per_page_options[self._rows_per_page_index] + rows_per_page = self.rows_per_page() if ( self._current_page * rows_per_page ) + rows_per_page < len(self.get_current_node()): From 5627878ea604cc9fb8319eb42bfe4fa37fbaf033 Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Tue, 7 Jan 2025 14:29:24 -0500 Subject: [PATCH 2/6] Add first and last navigation pages --- PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py | 6 ++++++ PyMca5/PyMcaGui/io/TiledCatalogSelector.py | 9 +++++++++ 2 files changed, 15 insertions(+) diff --git a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py index ed60883ea..9dc1db4ee 100644 --- a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py +++ b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py @@ -92,8 +92,10 @@ def create_layout(self) -> None: self.rows_per_page_selector = QComboBox() self.current_location_label = QLabel() + self.first_page = ClickableQLabel("<<") self.previous_page = ClickableQLabel("<") self.next_page = ClickableQLabel(">") + self.last_page = ClickableQLabel(">>") self.navigation_widget = QWidget() # Navigation layout @@ -101,8 +103,10 @@ def create_layout(self) -> None: navigation_layout.addWidget(self.rows_per_page_label) navigation_layout.addWidget(self.rows_per_page_selector) navigation_layout.addWidget(self.current_location_label) + navigation_layout.addWidget(self.first_page) navigation_layout.addWidget(self.previous_page) navigation_layout.addWidget(self.next_page) + navigation_layout.addWidget(self.last_page) self.navigation_widget.setLayout(navigation_layout) # Current path layout @@ -369,8 +373,10 @@ def connect_model_slots(self) -> None: self.url_entry.textEdited.connect(model.on_url_text_edited) self.url_entry.editingFinished.connect(model.on_url_editing_finished) self.connect_button.clicked.connect(model.on_connect_clicked) + self.first_page.clicked.connect(model.on_first_page_clicked) self.next_page.clicked.connect(model.on_next_page_clicked) self.previous_page.clicked.connect(model.on_prev_page_clicked) + self.last_page.clicked.connect(model.on_last_page_clicked) self.rows_per_page_selector.currentIndexChanged.connect(self.model.on_rows_per_page_changed) def connect_self_signals(self): diff --git a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py index 913fbed01..1d7e9edc0 100644 --- a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py +++ b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py @@ -1,4 +1,5 @@ import functools +from math import ceil import logging from collections import defaultdict from datetime import date, datetime @@ -207,6 +208,10 @@ def on_rows_per_page_changed(self, index): def rows_per_page(self): return self._rows_per_page_options[self._rows_per_page_index] + def on_first_page_clicked(self): + self._current_page = 0 + self.table_changed.emit(self.node_path_parts) + def on_prev_page_clicked(self): if self._current_page != 0: self._current_page -= 1 @@ -220,6 +225,10 @@ def on_next_page_clicked(self): self._current_page += 1 self.table_changed.emit(self.node_path_parts) + def on_last_page_clicked(self): + self._current_page = ceil(len(self.get_current_node()) / self.rows_per_page()) - 1 + self.table_changed.emit(self.node_path_parts) + def get_current_node(self) -> BaseClient: """Fetch a Tiled client corresponding to the current node path.""" return self.get_node(self.node_path_parts) From 692ad89158ceb55beee09321aab99b55a29c05c3 Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Tue, 7 Jan 2025 14:53:23 -0500 Subject: [PATCH 3/6] Make rows_per_page a property --- PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py | 6 +++--- PyMca5/PyMcaGui/io/TiledCatalogSelector.py | 15 +++++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py index 9dc1db4ee..49e5ee6f8 100644 --- a/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py +++ b/PyMca5/PyMcaGui/io/QTiledCatalogSelectorDialog.py @@ -223,9 +223,9 @@ def reset_rows_per_page(self) -> None: self.rows_per_page_selector.setCurrentIndex(self.model._rows_per_page_index) def _set_current_location_label(self): - starting_index = self.model._current_page * self.model.rows_per_page() + 1 + starting_index = self.model._current_page * self.model.rows_per_page + 1 ending_index = min( - self.model.rows_per_page() * (self.model._current_page + 1), + self.model.rows_per_page * (self.model._current_page + 1), len(self.model.get_current_node()), ) current_location_text = f"{starting_index}-{ending_index} of {len(self.model.get_current_node())}" @@ -248,7 +248,7 @@ def populate_table(self): self.catalog_table.setItem(0, 0, self.catalog_breadcrumbs) # Then add new rows - rows_per_page = self.model.rows_per_page() + rows_per_page = self.model.rows_per_page for _ in range(rows_per_page): last_row_position = self.catalog_table.rowCount() self.catalog_table.insertRow(last_row_position) diff --git a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py index 1d7e9edc0..04693e2df 100644 --- a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py +++ b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py @@ -118,6 +118,10 @@ def client(self): def client(self, _): """Do not directly replace the root Tiled client.""" raise NotImplementedError("Call connect_client() instead") + + @property + def rows_per_page(self): + return self._rows_per_page_options[self._rows_per_page_index] def connect_client(self) -> None: """Connect the model's Tiled client to the Tiled server at URL. @@ -204,10 +208,6 @@ def on_rows_per_page_changed(self, index): self._current_page = 0 self.table_changed.emit(self.node_path_parts) - # TODO: make this a property - def rows_per_page(self): - return self._rows_per_page_options[self._rows_per_page_index] - def on_first_page_clicked(self): self._current_page = 0 self.table_changed.emit(self.node_path_parts) @@ -218,7 +218,7 @@ def on_prev_page_clicked(self): self.table_changed.emit(self.node_path_parts) def on_next_page_clicked(self): - rows_per_page = self.rows_per_page() + rows_per_page = self.rows_per_page if ( self._current_page * rows_per_page ) + rows_per_page < len(self.get_current_node()): @@ -226,7 +226,10 @@ def on_next_page_clicked(self): self.table_changed.emit(self.node_path_parts) def on_last_page_clicked(self): - self._current_page = ceil(len(self.get_current_node()) / self.rows_per_page()) - 1 + # TODO: math.ceil gives the wrong answer for really large numbers + # Solution 4 in this answer: https://stackoverflow.com/a/54585138 + # Do we worry about this for our use case? + self._current_page = ceil(len(self.get_current_node()) / self.rows_per_page) - 1 self.table_changed.emit(self.node_path_parts) def get_current_node(self) -> BaseClient: From 455997a4f99c9341531bd6d1bd72f805408df60b Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Fri, 10 Jan 2025 13:51:01 -0500 Subject: [PATCH 4/6] Add tests for current location label and rows per page selector --- .../test_q_tiled_catalog_selector_dialog.py | 85 +++++++++++++++++-- .../tests/ut/test_tiled_catalog_selector.py | 12 ++- 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py b/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py index c92943a20..364fb7d6c 100644 --- a/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py +++ b/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py @@ -109,7 +109,7 @@ def test_navigation( qtbot: QtBot, tiled_client_dialog_model: TiledCatalogSelector, ): - """Check table page contents when going to next/previous pages.""" + """Check table page contents when going to first/next/previous/last pages.""" dialog = QTiledCatalogSelectorDialog(model=tiled_client_dialog_model) dialog.show() qtbot.addWidget(dialog) @@ -120,8 +120,10 @@ def test_navigation( dialog.model.on_next_page_clicked() - # Check last value shows in new page - expected_text = ["f"] + # Check last values show in new page + expected_text = ["f", "structured_data"] + + assert len(expected_text) == dialog.catalog_table.rowCount() for row_num, text in enumerate(expected_text): assert dialog.catalog_table.item(row_num, 0).text() == text @@ -130,13 +132,37 @@ def test_navigation( expected_text = ["a", "b", "c", "d", "e"] + assert len(expected_text) == dialog.catalog_table.rowCount() + + for row_num, text in enumerate(expected_text): + assert dialog.catalog_table.item(row_num, 0).text() == text + + dialog.model.on_next_page_clicked() + + # Check last values show in last page + dialog.model.on_last_page_clicked() + + expected_text = ["f", "structured_data"] + + assert len(expected_text) == dialog.catalog_table.rowCount() + + for row_num, text in enumerate(expected_text): + assert dialog.catalog_table.item(row_num, 0).text() == text + + # Check first values show in first page + dialog.model.on_first_page_clicked() + + expected_text = ["a", "b", "c", "d", "e"] + + assert len(expected_text) == dialog.catalog_table.rowCount() + for row_num, text in enumerate(expected_text): assert dialog.catalog_table.item(row_num, 0).text() == text def test_current_page_layout( - qtbot: QtBot, - tiled_client_dialog_model: TiledCatalogSelector + qtbot: QtBot, + tiled_client_dialog_model: TiledCatalogSelector, ): """Check current page layout of breadcrumbs updates correctly.""" dialog = QTiledCatalogSelectorDialog(model=tiled_client_dialog_model) @@ -186,8 +212,8 @@ def test_current_page_layout( def test_clicking_breadcrumbs( - qtbot: QtBot, - tiled_client_dialog_model: TiledCatalogSelector + qtbot: QtBot, + tiled_client_dialog_model: TiledCatalogSelector, ): """Check clicking breadcrumbs displays correct contents.""" dialog = QTiledCatalogSelectorDialog(model=tiled_client_dialog_model) @@ -209,3 +235,48 @@ def test_clicking_breadcrumbs( expected_text = ["a", "b", "c", "d", "e"] for row_num, text in enumerate(expected_text): assert dialog.catalog_table.item(row_num, 0).text() == text + + +def test_current_location_label( + qtbot: QtBot, + tiled_client_dialog_model: TiledCatalogSelector, +): + """Check the current location is properly displayed.""" + dialog = QTiledCatalogSelectorDialog(model=tiled_client_dialog_model) + dialog.show() + qtbot.addWidget(dialog) + + assert dialog.rows_per_page_selector.currentText() == "5" + assert f"of {len(dialog.model.client)}" in dialog.current_location_label.text() + + assert "1-5" in dialog.current_location_label.text() + + # Go to last page + dialog.model.on_last_page_clicked() + assert f"6-{len(dialog.model.client)}" in dialog.current_location_label.text() + + # Go to first page + dialog.model.on_first_page_clicked() + assert "1-5" in dialog.current_location_label.text() + +def test_rows_per_page_selector_changed_updates_table( + qtbot: QtBot, + tiled_client_dialog_model: TiledCatalogSelector, +): + """Check the table is updated when the rows per page is changed.""" + dialog = QTiledCatalogSelectorDialog(model=tiled_client_dialog_model) + dialog.show() + qtbot.addWidget(dialog) + + assert dialog.rows_per_page_selector.currentText() == "5" + assert dialog.catalog_table.rowCount() == 5 + + # Change to 10 rows per page (index 1) + dialog.rows_per_page_selector.setCurrentIndex(1) + assert dialog.rows_per_page_selector.currentText() == "10" + assert dialog.catalog_table.rowCount() == len(dialog.model.client) + + # Change back to 5 rows per page (index 0) + dialog.rows_per_page_selector.setCurrentIndex(0) + assert dialog.rows_per_page_selector.currentText() == "5" + assert dialog.catalog_table.rowCount() == 5 diff --git a/PyMca5/tests/ut/test_tiled_catalog_selector.py b/PyMca5/tests/ut/test_tiled_catalog_selector.py index a159e2982..cdd8103e5 100644 --- a/PyMca5/tests/ut/test_tiled_catalog_selector.py +++ b/PyMca5/tests/ut/test_tiled_catalog_selector.py @@ -235,7 +235,7 @@ def test_rows_per_page_options(input_options: Optional[List[int]], expected: Lis def test_page_navigation(tiled_client: BaseClient): - """Check navigation to next/previous pages updates current_page and emits table_changed.""" + """Check navigation to/from pages updates current_page and emits table_changed.""" model = TiledCatalogSelector(client=tiled_client) assert model._current_page == 0 @@ -265,6 +265,16 @@ def test_page_navigation(tiled_client: BaseClient): assert model._current_page == 0 assert mock_signal.emit.call_count == 1 + # Check last and first page navigation + mock_signal.emit.reset_mock() + model.on_last_page_clicked() + assert model._current_page == 1 + assert mock_signal.emit.call_count == 1 + + model.on_first_page_clicked() + assert model._current_page == 0 + assert mock_signal.emit.call_count == 2 + def test_node_navigation(tiled_client: BaseClient): """Check navigating between nodes updates node_path_parts/_current_page and emits table_changed.""" From ef95989ec32bc43e989aba1cadca3ed0ec3ccb98 Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Fri, 10 Jan 2025 14:48:54 -0500 Subject: [PATCH 5/6] Update comments about math.ceil --- PyMca5/PyMcaGui/io/TiledCatalogSelector.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py index 04693e2df..f4f640461 100644 --- a/PyMca5/PyMcaGui/io/TiledCatalogSelector.py +++ b/PyMca5/PyMcaGui/io/TiledCatalogSelector.py @@ -226,9 +226,8 @@ def on_next_page_clicked(self): self.table_changed.emit(self.node_path_parts) def on_last_page_clicked(self): - # TODO: math.ceil gives the wrong answer for really large numbers + # NOTE: math.ceil gives the wrong answer for really large numbers # Solution 4 in this answer: https://stackoverflow.com/a/54585138 - # Do we worry about this for our use case? self._current_page = ceil(len(self.get_current_node()) / self.rows_per_page) - 1 self.table_changed.emit(self.node_path_parts) From 7cb7dfd81589db1473914a9618efec1a77c353de Mon Sep 17 00:00:00 2001 From: Abigail Giles Date: Tue, 14 Jan 2025 13:30:04 -0500 Subject: [PATCH 6/6] Remove unnecessary next page navigation in test --- PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py b/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py index 364fb7d6c..3c64188fb 100644 --- a/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py +++ b/PyMca5/tests/ut/test_q_tiled_catalog_selector_dialog.py @@ -137,8 +137,6 @@ def test_navigation( for row_num, text in enumerate(expected_text): assert dialog.catalog_table.item(row_num, 0).text() == text - dialog.model.on_next_page_clicked() - # Check last values show in last page dialog.model.on_last_page_clicked()