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 ConversationView.add_reply_from_reply_box #834

Closed
wants to merge 2 commits into from

Conversation

redshiftzero
Copy link
Contributor

Description

Fixes #821.

I had a suspicion that adding widgets to the conversation view outside of ConversationView.update_conversation might be a cause for the odd bubbles appearing on top of each other behavior that some are seeing (I've not seen this myself yet), so putting this diff up which I had locally (because I don't think this method is required anymore) in case it's related to that. Will look more tomorrow to try to reproduce the underlying behavior if no one beats me to it.

Test Plan

Check you can still send replies and they are confirmed as usual

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

@redshiftzero redshiftzero changed the title [wip] remove conversation_view.add_reply_from_reply_box [wip] remove ConversationView.add_reply_from_reply_box Feb 28, 2020
@redshiftzero redshiftzero modified the milestones: 0.2.0beta, Post-Beta Mar 16, 2020
@eloquence
Copy link
Member

eloquence commented Jun 18, 2020

Checked in with @redshiftzero about this PR before her leave; this is still a valuable change, we can pick this up from her in coming weeks.

@emkll emkll changed the base branch from master to main July 15, 2020 12:53
@sssoleileraaa sssoleileraaa marked this pull request as ready for review March 10, 2022 00:33
@sssoleileraaa sssoleileraaa requested a review from a team as a code owner March 10, 2022 00:33
@sssoleileraaa
Copy link
Contributor

I just updated the work that @redshiftzero did to get it ready for review. I don't think it fixes an error at this point, but it is a nice refactor that simplifies our logic around adding reply widgets. I tried to change very little so that this remains a simply refactor.

@sssoleileraaa sssoleileraaa requested a review from rocodes March 10, 2022 01:12
@sssoleileraaa sssoleileraaa changed the title [wip] remove ConversationView.add_reply_from_reply_box Remove ConversationView.add_reply_from_reply_box Mar 10, 2022
@sssoleileraaa sssoleileraaa marked this pull request as draft March 10, 2022 01:47
@sssoleileraaa
Copy link
Contributor

sssoleileraaa commented Mar 10, 2022

@rocodes - Feel free to test this alongside #1432, it's still a WIP, which you can take over, review, or just incorporate it into #1432. After today's conversation, I think this could help separate out some of the complexity around reply widgets for draftreplies.

@sssoleileraaa
Copy link
Contributor

(tests should fail because I didn't even run them locally yet after my update)

@rocodes
Copy link
Contributor

rocodes commented Mar 10, 2022

This is a nice change that definitely simplifies the reply and draft logic, tests look good, and replies still show up in the reply box.

I did once notice a reply that seemed to briefly disappear after being sent and re-appear on sync, but I haven't been able to reproduce it despite a lot of attempts. I also noticed this issue, but it's not a regression since it's also present on main.

From a preliminary review it looks good to me.

@sssoleileraaa
Copy link
Contributor

Closing because commits from here are being cherry-picked into #1432

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.

remove ConversationView.add_reply_from_reply_box
4 participants