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

Refactor existing application to use signals/slots for updating UI (either via MVC or directly) #208

Closed
redshiftzero opened this issue Dec 4, 2018 · 5 comments

Comments

@redshiftzero
Copy link
Contributor

Description

One pain point with the application currently is that in many places in the application we explicitly run methods to update the UI (e.g. update_conversation_view), leading to some confusion.

What we want is a single place for updating the state of the underlying data, and a single place that defines how that underlying data will be represented in the UI. Fortunately, there is an MVC pattern in qt that we can use to implement this, where changes in the models emit signals that view + delegate functions use to "automatically" update. This obviates the need for us to write additional code to get the various UI elements to refresh.

In this pattern, we use AbstractItemModelClass to represent each data object (probably the List class is appropriate for the various items we have in SecureDropLand). These models should contain the logic for e.g. adding items to the database / removing items from the database / pretty much most of the logic in storage.py.

Each UI element that reflects the state of the underlying data should have a view and delegate function that references the relevant model. Where we are using SDK or ORM objects in the existing application logic we should be interacting with the qt model objects. For example, in the sync logic when we add a local source to the database, it should be done via the AbstractModel corresponding to a source (such that the corresponding UI elements update automatically).

Additional references

@heartsucker found this qtalchemy project which combines the MVC approach with sqlalchemy and this nice presentation explaining the MVC pattern.

@kushaldas shared the calibre project that is using qt's MVC pattern.

@redshiftzero redshiftzero added this to the 0.1.0beta milestone Dec 4, 2018
@redshiftzero redshiftzero changed the title Refactor existing application to use qt's MVC framework Refactor existing application to use signals/slots for updating UI state (either via MVC or directly) Dec 5, 2018
@redshiftzero redshiftzero changed the title Refactor existing application to use signals/slots for updating UI state (either via MVC or directly) Refactor existing application to use signals/slots for updating UI (either via MVC or directly) Dec 5, 2018
@redshiftzero
Copy link
Contributor Author

So, @joshuathayer and I did some further investigation into more examples of MVC usage this afternoon and started trying to figure specifically what abstract objects we would use in the client - and some issues are presenting themselves.

How we'd use the MVC framework, for example, if we want to rewrite the conversation view as a view that displays an abstract list model representing the source's conversation history is as follows. In each place where we want to change the model, it looks like we need to instead replace our current update_conversation_view call with a conversation_view.setModel(new_source) where new_source represents the messages/replies/files associated with that source. If we want to add messages to the conversation view, we now will have to either redraw the entire view via conversation_view.setModel(new_source) where new_source represents the conversation view with the change, or (better) call addData to add a new row to the data associated with new_source (as this new_source object only represents a subset of the data in the database, only the parts that are relevant for the specific conversation). This is the common pattern for the MVC framework afaict and as such doesn't simplify things very much for us.

Having a direct mapping between DB models and qt models has its own problems:

  1. we would then need to re-implement any filtering/sorting logic we need ourselves instead of just using a relational database as we do now and
  2. we'd need to have a parent container object (likely itself an abstract list model) with child abstract models that e.g. the conversation view displays (not sure how to do this yet, examples welcome)

It looks more straightforward and cleaner implementation-wise to emit a custom signal in the various places when we want to indicate the relevant bit of data has changed throughout the application, which we can listen for in the relevant UI elements (the source list and conversation view), and abandon this intermediate model representation. So to resolve the conversation view flicker (which was implemented such that the messages/replies appear as they are decrypted), we'd emit a signal once we are done decrypting the message (with the id of the object in question), which would be connected to a slot on the SpeechBubble.

It will be instructive to implement a couple of spikes doing this (or competing approaches if there are better ideas) with:

  • Source List View
  • Conversation View

Please comment if you're attempting a spike with which bit of the GUI and with which approach so we don't duplicate work ^

@heartsucker @kushaldas let us know if you find something different in your investigations, thoughts welcome here

@heartsucker
Copy link
Contributor

This is the common pattern for the MVC framework afaict

Can you link to an example in some code somewhere for this? Because if my faffing about, it seemed that widgets would be created ahead of time then "cached". To swap between them, hide() and show() would be used. Perhaps this only works in the very simple examples I was looking at.

@heartsucker
Copy link
Contributor

That said, the while the MVC model seems to the thing I came across the most during my investigation into concurrent UI things, it also seems like we'd have to do a fair amount of cobbling things together to make the UI work with it.

@redshiftzero
Copy link
Contributor Author

I think right now these are the only two areas of the application that need to be modified for this UI ticket to be closed (we can file followups for other refactoring).

@redshiftzero
Copy link
Contributor Author

closing, in the future for issues like #658, we should emit signals to update the relevant UI widgets (as we are doing for e.g. updating the content of a message or reply, moving a pending reply to failed or succeeded)

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

No branches or pull requests

2 participants