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

fix:Further refactoring to simplify control flow #2521

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Conversation

joernu76
Copy link
Member

@joernu76 joernu76 commented Sep 9, 2024

Fewer LOC to make things more maintainable.
Also Fix #2514

Purpose of PR?:

Fixes #2514

Does this PR introduce a breaking change?

If the changes in this PR are manually verified, list down the scenarios covered::

Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes

Checklist:

@joernu76 joernu76 force-pushed the mscolab_refactor branch 4 times, most recently from bbb3ee7 to ba5a1be Compare September 9, 2024 12:05
@joernu76
Copy link
Member Author

joernu76 commented Sep 9, 2024

The intent of this MR is again to enforce more consistent handling by use of decoration magic.

Several places had a pop_up messages saying that a new login was required without doing anything about this.
The new Exception shows consistently a popup message before triggering a logout.

The issue of #2521 was caused by a QT signal calling a MSColab function after logout, which caused a couple of issues.

The current code does not crash anymore on my end when the MSColab server is restarted. Now a test simulating server restarts and/or connection troubles would be next steps.

Fewer LOC to make things more maintanable.
Also Fix #2514
@joernu76
Copy link
Member Author

Finally passes checks. As a change, the App now complains more often in case that the active operations was disabled with a popup. I believe this is sensible, but should be explored further in the next MR.

@joernu76
Copy link
Member Author

So, for simplifying reviewing, hiding whitespace is essential.

  1. The major change is the use of Exceptions to logout in a consistent manner in less code.
  2. I try to properly set active_op_id to None, but this necessitated several changes in tests. I believe the behaviour is better now, but this is not over, yet.
  3. I made QMessageBox and some minor QT stuff consistent.

It passes testbench and several manual tests (certainly not all, but sensible stuff) on my computer behave in a clean, sensible fashion with two clients. Including restarting the server.

@ReimarBauer
Copy link
Member

ReimarBauer commented Sep 13, 2024

Thx @joernu76 I'll look at it later today.

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

Starting testing both cases

connect - restart server - select

with the default to skip the verification it crashes. Using the client side verification not

skip_verify True

skip_verify_connect_restart_select

skip_verify False

no_skip_verify_connect_restart_select

@ReimarBauer
Copy link
Member

ReimarBauer commented Sep 15, 2024

connect select restart select

with the default to skip the verification it crashes. Using the client side verification not

skip_verify True

skip_verify_connect_select_restart_select

skip_verify False

no_skip_verify_connect_select_restart_select

@ReimarBauer
Copy link
Member

connect select topview add restart add

with the default to skip the verification it crashes. Using the client side verification not

skip_verify True

skip_verify_connect_select_topview_add_restart_add

skip_verify False

no_skip_verify_connect_select_topview_add_restart_add

@ReimarBauer
Copy link
Member

ReimarBauer commented Sep 15, 2024

connect select chat restart message

with the default to skip the verification it does nothing. Using the client side verification it shows expired message

skip_verify True

skip_verify_connect_select_chat_mnessage

skip_verify False

no_skip_verify_connect_select_chat_mnessage

Also catch uncaught SocketIO-Exceptions to handle them gracefully.
@joernu76
Copy link
Member Author

So, two of these should be handled now in a general and robust fashion. I strongly dislike the "depth" counter, though Better ideas?
The CHAT is a separate class that I define as out-of-scope now. -> New issue.
@ReimarBauer

Copy link
Member

@ReimarBauer ReimarBauer left a comment

Choose a reason for hiding this comment

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

looks good.

I found another problem unrelated to these changes. A doubleclick on login gives a traceback and login works.
That happens for both cases.

Fatal error in MSS 9.2.0 on Linux-6.11.0-100005-tuxedo-x86_64-with-glibc2.35
Python 3.11.10 | packaged by conda-forge | (main, Sep 10 2024, 11:01:28) [GCC 13.3.0]

Please report bugs in MSS to https://github.com/Open-MSS/MSS

Information about the fatal error:

Traceback (most recent call last):
  File "/home/reimar/MAIN/MSS/mslib/msui/mscolab.py", line 385, in login_handler
    self.mscolab.after_login(data["email"], self.mscolab_server_url, r)
  File "/home/reimar/MAIN/MSS/mslib/msui/mscolab.py", line 694, in after_login
    self.connect_window.close()
    ^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'close'

@joernu76 joernu76 merged commit 457ff08 into develop Sep 18, 2024
10 of 11 checks passed
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.

unhandled crashes with mscolab verification shenanigangs
2 participants