Skip to content
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.

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

active_op_id needs to be shared #2326

Merged
merged 3 commits into from
May 6, 2024
Merged

Conversation

ReimarBauer
Copy link
Member

store widget data corresponding to docks

Purpose of PR?:

Fixes #2322
including tests

store widget data corresponding to docks
@ReimarBauer ReimarBauer marked this pull request as draft April 17, 2024 10:29
@ReimarBauer ReimarBauer marked this pull request as ready for review April 17, 2024 10:52
@ReimarBauer ReimarBauer requested a review from matrss April 18, 2024 05:55
mslib/msui/topview.py Outdated Show resolved Hide resolved
@@ -221,7 +224,7 @@ def __init__(self, parent=None, mainwindow=None, model=None, _id=None,
self.active_flighttrack = active_flighttrack

# Stores active mscolab operation id
self.active_op_id = None
self.active_op_id = mainwindow.mscolab.active_op_id
Copy link
Collaborator

@matrss matrss Apr 18, 2024

Choose a reason for hiding this comment

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

In which situation should the topview's active_op_id be something different than the mainwindow.mscolab.active_op_id? I.e. why does the topview need a copy of this attribute, instead of always using the one from the mainwindow?

Copy link
Member Author

Choose a reason for hiding this comment

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

topview won't need it. The current implementation of the multiple_flighpath_dockingwidget has no access to the mainwindow, e.g.
https://github.com/Open-MSS/MSS/blob/develop/mslib/msui/multiple_flightpath_dockwidget.py#L507
There were more vars introduced which are already known in the mainwindow https://github.com/Open-MSS/MSS/blob/develop/mslib/msui/topview.py#L204

Currently I do prefer first a kind of logic test coverage before we refactor the inner. It is somehow improving the Proof-Of-Concept before we can do the cleanup.

One of the bigger refactoring steps is to remove all the dups and focus on a better understandable API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Couldn't the topview just hold a reference to the mainwindow then? Holding another reference to all of the mainwindow's signals is also something that feels wrong to me, and could be eliminated if the mainwindow was just accessible through the topview.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that would be a better solution and has to be done in a refactoring. @Jatin2020-24 did you have a reason not to do it that way?

This PR is related to fix a bug in the order for opening the dockingwidget when a mscolab operation was selected. We need a few more tests to figure that any combination for the docking widget works.

I guess there is at least one more bug which needs to be test covered and fixed.

When this part is refactored we also have to refactor tests.

@ReimarBauer
Copy link
Member Author

@matrss something more missing or should be improved?

@matrss
Copy link
Collaborator

matrss commented Apr 25, 2024

I still think we shouldn't duplicate this attribute everywhere. What is wrong with exposing a reference to the mainwindow on the topview? Then it would be possible for the MultipleFlightpathControlWidget to just access self.ui.mainwindow.active_op_id, or something like that.

@matrss
Copy link
Collaborator

matrss commented Apr 25, 2024

Alternatively, make the topview's active_op_id a getter/@property for the one from the mainwindow so that it can never be out of sync. Depends on how much of the mainwindow we want to have accessible everywhere.

@matrss
Copy link
Collaborator

matrss commented May 6, 2024

@ReimarBauer and I talked about this a bit this morning and we came to the conclusion that the PR should be merged as-is, but we need to take a look at things like this in the future and make an effort to refactor these things and clean up some of it.

@ReimarBauer ReimarBauer merged commit afa3061 into Open-MSS:develop May 6, 2024
8 checks passed
@ReimarBauer ReimarBauer deleted the i2322 branch May 6, 2024 09:27
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.

active_op_id needs a better implementation in topview (mscolab)
2 participants