Skip to content

fixed MSCOLAB_timeout definition by list_operation_structure #2815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ReimarBauer merged 4 commits into from
Jun 23, 2025
Merged

fixed MSCOLAB_timeout definition by list_operation_structure #2815

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
47ccf17
list_operation_structure fixed
ReimarBauer May 11, 2025
df88302
milestone check fixed
ReimarBauer May 11, 2025
cc8ab5a
introduced fixed list options
ReimarBauer May 13, 2025
e333ff3
take care for fixed_list_options
ReimarBauer May 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mslib/utils/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ class MSUIDefaultConfig:
"new_flighttrack_template": ["new-location"],
"gravatar_ids": ["example@email.com"],
"WMS_preload": ["https://wms-preload-url.com"],
"MSCOLAB_timeout": [[2, 10]],
"MSCOLAB_timeout": [int(), int()],
Copy link
Copy Markdown
Collaborator

@matrss matrss May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int() == 0, what's the reasoning for changing it to that? The main difference seems to be removing the inner list, but is that correct? Afterall, the timeout setting can be a tuple of two values, or something equivalent.

Copy link
Copy Markdown
Member Author

@ReimarBauer ReimarBauer May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it can be zero. These entries are in place to guarantee that new values have the same data type as existing ones. I’m hesitant to keep both default values because we used only placeholders elsewhere.

The inner list definition is wrong, it should only have 2 values of type integer.

Copy link
Copy Markdown
Collaborator

@matrss matrss May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner list definition is wrong, it should only have 2 values of type integer.

Are you sure about that? The other definitions all have one element in the outer list, which defines a placeholder value for the config option. And in the case of MSCOLAB_timeout that config option is supposed to be a sequence of two elements, right?

Copy link
Copy Markdown
Member Author

@ReimarBauer ReimarBauer May 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is the same as with gravatar_ids, new_flightrack_template and some
more.

The list_option_structure is used by the config_editor, to create a new row for e.g. automated_plotting_flights [[]] with 6 elements.

Bildschirmfoto 2025-05-13 um 13 08 55

using [] creates for the gravatar_id a new row with 1 element.
Bildschirmfoto 2025-05-13 um 15 49 30

Because MSCOLAB_timeout is implemented as a list allowing additions and removals, this introduces potential problems in the UI. We need to ensure the UI behaves consistently with how it handles non-list values – for example

Bildschirmfoto 2025-05-13 um 13 16 56

"automated_plotting_flights": [["", "", "", "", "", ""]],
"automated_plotting_hsecs": [["http://www.your-wms-server.de", "", "", ""]],
"automated_plotting_vsecs": [["http://www.your-wms-server.de", "", "", ""]],
Expand Down
2 changes: 0 additions & 2 deletions tests/_test_msui/test_mscolab.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,10 @@ def setup(self, qtbot, mscolab_app, mscolab_server):
# close all hanging operation option windows
self.window.mscolab.close_external_windows()

@pytest.mark.xfail(reason="https://github.com/Open-MSS/MSS/issues/2716", strict=True)
def test_modify_mscolab_timeout(self, qtbot):
data = {"MSCOLAB_timeout": [5, 10]}
modify_config_file(data)
self.window.open_config_editor()
# the assert fails because last_saved returns [[2, 10], [2, 10]]
assert self.window.config_editor.last_saved["MSCOLAB_timeout"] == data["MSCOLAB_timeout"]

def test_activate_operation(self, qtbot):
Expand Down
2 changes: 1 addition & 1 deletion tests/_test_msui/test_msui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def test_milestone_url(self):
with urlopen(self.window.milestone_url) as f:
text = f.read().decode("utf-8")
expected_version = __version__
pattern = rf'value="is:closed milestone:{re.escape(expected_version)}"'
pattern = rf'value="is:closed milestone:{re.escape(expected_version)} "'
Copy link
Copy Markdown
Member Author

@ReimarBauer ReimarBauer May 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems gh has changed the content.

The test is then very fragile and needs a better solution

Copy link
Copy Markdown
Member Author

@ReimarBauer ReimarBauer May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2816

assert re.search(pattern, text), f"Expected milestone format not found: {expected_version}"


Expand Down
Loading