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

Remove source db object from SourceWidget #967

Closed
wants to merge 4 commits into from
Closed

Conversation

sssoleileraaa
Copy link
Contributor

@sssoleileraaa sssoleileraaa commented Mar 20, 2020

Description

After #964 is merged, this will need a rebase and will be ready for review.

Towards #906 and minimizing access of database objects in the GUI in order to avoid crashes. This removes the source database object from the SourceWidget and fixes #1 in #906.

The PR that removes the source object from the star will either need to be merged after this PR or if before, I'll need to do a rebase.

Test Plan

  1. Regression test around the source widget
  2. See how we no longer can raise an exception in the constructor of SourceWidget

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

@sssoleileraaa sssoleileraaa changed the title 906 update Remove source db object from SourceWidget Mar 20, 2020
@redshiftzero
Copy link
Contributor

Let's merge #952 next and then this

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

took a look through and did some testing, this looks good! a couple questions inline but otherwise I think after a rebase this is good to go

if source_widget:
return self.controller.get_source(source_widget.source_uuid)
except sqlalchemy.exc.InvalidRequestError:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

pass so that we let the next sync delete the SourceWidget?

list_item = self.item(i)
source_widget = self.itemWidget(list_item)
if source_widget and source_widget.source_uuid == source_uuid:
return source_widget
Copy link
Contributor

Choose a reason for hiding this comment

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

clarifying: under which circumstances can the source widget be in the cache but not the source list?

Copy link
Contributor Author

@sssoleileraaa sssoleileraaa Mar 24, 2020

Choose a reason for hiding this comment

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

I think we could rely completely on the cache. I abstracted getting the source widget here because I was thinking we didn't need both the qt list and the cache since we have to loop through all items in the qt list (SourceList) anyway. But after thinking about this more, we have future plans to modify server code so that it only returns sources that have updates, and that's when the cache will come in handy (because we won't have to loop through the entire source list anymore and update all the source widgets). Long story short: I can update this method so that there's no fall back if we try to get a source widget that hasn't been added to the cache yet (since we add items to the cache before adding them to the qt list, this currently can't happen, but, if we ever switch the order of this by accident, having a fallback would ensure that we'll be able to get the selected source widget in the list)

@sssoleileraaa
Copy link
Contributor Author

This is on hold until this PR is finished and merged: #944

@redshiftzero
Copy link
Contributor

marking as blocked so that we don't introduce more conflicts for #944 as @creviera mentions above

@eloquence
Copy link
Member

eloquence commented Mar 31, 2020

#944 is merged so this is technically no longer blocked, but last week we discussed a smaller scoped first iteration of using try/catch blocks to prevent the crashers described in #906. Moving back to backlog pending a final decision on whether this should land.

@sssoleileraaa
Copy link
Contributor Author

Closing for now and will revisit in another sprint

@eloquence
Copy link
Member

@creviera Do you still want to keep this PR in our near-term backlog, for consideration in another sprint?

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.

3 participants