Failed and pending message to end of conversation. #818
Conversation
|
Current state of this problem:
I'd also add, that I believe we have stale code (the loop to delete out of date widgets) which should be refactored out. I also believe that we could improve the readability of this method for the benefit of future maintainers... |
There was a problem hiding this comment.
[posting my comment from earlier I couldn't due to GH 500s]
so the following situation can occur:
successful message A
failed message B (e.g. failed from previous session)
successful message C
failed message D
it seems like with this change we’d have items appearing out of order, i.e. A, C, B, D, when we should have A, B, C, D
| @@ -1776,7 +1776,8 @@ def __init__( | |||
| message_failed_signal.connect(self._on_reply_failure) | |||
|
|
|||
| # Set styles | |||
There was a problem hiding this comment.
nit: this comment seems out of place (i know this wasn't introduced by your code but just noticed it above your new lines of code and was like, this isn't setting styles!)
| for index, conversation_item in enumerate(collection): | ||
| item_widget = current_conversation.get(conversation_item.uuid) | ||
| if item_widget: | ||
| current_conversation.pop(conversation_item.uuid) | ||
| if item_widget.index != index: | ||
| if isinstance(item_widget, ReplyWidget) and item_widget.reply_status != "SUCCEEDED": |
There was a problem hiding this comment.
i think this if-elif block doesn't cover the case for reply widgets that are successful where item_widget.index == index
There was a problem hiding this comment.
this is what i see when running though STR#2 from #653:
which looks like the behavior you described where non-successful replies are pushed to the bottom?
|
The TL;DR from my re-reading of my changes and your feedback and commentary (many thanks for that!), is that the code is confusing (ripe for refactoring). If it were not so confusing, we'd know what's going on. ;-) A quick check tells me that @redshiftzero is correct in saying that the conversation ordering is handled elsewhere (see #653 (comment)) so we should always display the conversation in the current order as found from local storage. The obvious solution is just to re-draw the conversation, but this was one aspect of the cause of the UI freeze with lots of sources -- hence the desire to re-order the widgets in place. I believe my changes in this PR just add unnecessary complexity to an already complicated and often-re-written piece of code. We have two things that are ordered:
We've obviously not catered for all possible re-ordering of existing widgets, or "slotting in" of new widgets into the It also appears that |
|
After further investigation it appears that this was actually due to the ordering in
Indeed - though reply widgets can be added to ConversationView outside of update_conversation via add_reply_from_reply_box (see related #834) Gonna close this one for now, but feel free to open issues/wip for other improvements you think make sense to simplify ConversationView |
Description
Fixes #653 .
This is a spike. TODO: unit tests to confirm. Pushed and PR'd so folks can see/check my approach.
Status
Requesting feedback
Test Plan
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:
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: