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

Ensure correct StarToggleButton signal handler state #933

Merged
merged 3 commits into from
Mar 13, 2020

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Mar 13, 2020

Description

StarToggleButton.on_authentication_changed would just keep connecting signal handlers without ever disconnecting those previously installed.

When not authenticated, we'd set a signal handler to show the "You must sign in" message when pressed. That handler was never disconnected once authentication was successful, so if the timing were right at startup, it would get installed permanently.

Similarly, we could end up with multiple on_toggle handlers, sending multiple UpdateStarJobs for a single click on the StarToggleButton.

This change disconnects all handlers for the previous authentication state when authentication state changes.

Also, the Controller.is_authenticated setter emitted the authentication_state signal before updating self.__is_authenticated. To prevent that ever being a problem for signal handlers that check the property and not the signal value, the order of those calls has been swapped.

Fixes #919.

Test Plan

  • Run the client with LOGLEVEL=debug, preferably in an environment where you were able to reproduce starring requiring login after already logged in #919 (I could only trigger the problem in sd-app talking to my staging environment; it never happened for me in the dev environment).
  • Log in to the client.
  • Try to star a source. It should work, without an error message saying you need to log in.
  • Log out.
  • Try to star a source. It should not work, and you should see an error message saying you need to log in.
  • Log back in and star a source.
  • Check the client log. Only one UpdateStarJob should have been sent for your last action.

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes

If these changes add or remove files other than client code, packaging logic (e.g., the AppArmor profile) may need to be updated. Please check as applicable:

  • I have submitted a separate PR to the packaging repo
  • No update to the packaging logic (e.g., AppArmor profile) is required for these changes
  • I don't know and would appreciate guidance

rmol added 2 commits March 13, 2020 16:42
StarToggleButton.on_authentication_changed would just keep connecting
signal handlers without ever disconnecting those previously installed.

When not authenticated, we'd set a signal handler to show the "You
must sign in" message when pressed. That handler was never
disconnected once authentication was successful, so if the timing were
right at startup, it would get installed permanently.

Similarly, we could end up with multiple on_toggle handlers, sending
multiple UpdateStarJobs for a single click on the StarToggleButton.

This change disconnects all handlers for the previous authentication
state when authentication state changes.

Also, the Controller.is_authenticated setter emitted the
authentication_state signal before updating
self.__is_authenticated. To prevent that ever being a problem for
signal handlers that check the property and not the signal value, the
order of those calls has been swapped.
@rmol rmol force-pushed the 919-star-handlers branch from bb3dc99 to 51bfcbc Compare March 13, 2020 20:42
self.set_icon(on='star_on.svg', off='star_on.svg')

try:
while True:
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this disconnect many times? why can't we just disconnect once? it seems like an exception has to be raised in order for this to exit the loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will disconnect all the connections of self.on_toggle_offline that have been made. There should only ever be one now. And yes, the exception raised when no such connection exists is what exits the loop.

else:
self.pressed.connect(self.on_toggle_offline)
Copy link
Contributor

Choose a reason for hiding this comment

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

did adding a self.toggled.disconnect() and self.pressed.disconnect() before this line not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It did.

@sssoleileraaa
Copy link
Contributor

This works as described. I'm just surprised that we have to diconnect in a while-true loop and hoping for more of an explanation for why we can't disconnect the toggled and pressed signals just once before we reassign slots?

@rmol
Copy link
Contributor Author

rmol commented Mar 13, 2020

The disconnect loop is just belt-and-braces caution, to ensure we never accumulate more than one connection for a handler.

@sssoleileraaa sssoleileraaa self-requested a review March 13, 2020 21:25
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.

starring requiring login after already logged in
2 participants