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

Avoid drafts in chats where canSend is false #4394

Merged
merged 5 commits into from
Dec 8, 2024

Conversation

nicodh
Copy link
Contributor

@nicodh nicodh commented Dec 8, 2024

resolves #4389

@nicodh
Copy link
Contributor Author

nicodh commented Dec 8, 2024

No changelog entry since bug was introduced in test release

@nicodh nicodh requested a review from WofWca December 8, 2024 07:34
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

I don't know if this is the right approach. The fact that useDraft returns draftState of the previous chat after you switch to a chat to which you cannot send, seems pretty wrong.

Maybe it would be simpler to just make the final return statement return dummy functions and objects instead. Hmmm, but then, the shortcut code is still active...

Or maybe just disable the shortcut and that's it, and wait for the composer refactor.

Also this doesn't actually disable the Ctrl + Up shortcut in !canSend chats. The messages still get highlighted.

No changelog entry since bug was introduced in test release

I don't think so. You can utilize the Ctrl + Up shortcut in !canSend chats in 1.48.0.

Also we'd probably want to merge #4395 first, because I think this MR makes it easier to crash when switching between chats.

@nicodh nicodh force-pushed the fix-4389-draft-in-readonly-chats branch from 206e641 to 07df8cf Compare December 8, 2024 13:28
Copy link
Collaborator

@WofWca WofWca left a comment

Choose a reason for hiding this comment

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

Ok, #4395 is merged and the shortcut no longer works where it shouldn't. Let's hope this can keep us afloat until the composer refactor!

@nicodh
Copy link
Contributor Author

nicodh commented Dec 8, 2024

I don't know if this is the right approach. The fact that useDraft returns draftState of the previous chat after you switch to a chat to which you cannot send, seems pretty wrong.

In a readonly chat there should be no calls to setDraft or load draft at all. So if you prefer a minimal approach we could add the !canSend condition only to the shortcut in https://github.com/deltachat/deltachat-desktop/pull/4394/files#diff-ca83809f74f6819ef5c7d61676ac6c9abd27cb5becc46cb08bc9c43bc87c0f61R596

btw: it seems clearDraft is nowhere used outside of useDraft, so no need to return it.

@WofWca
Copy link
Collaborator

WofWca commented Dec 8, 2024

In a readonly chat there should be no calls to setDraft or load draft at all

There was this MR #4342 which suggested making the draft editable even when !canSend.
But if we decide to make the draft non-editable when !canSend, we should probably make useDraft return a value where these functions (updateDraftText, removeQuote) can be null or something like that, instead of silently doing nothing.

But maybe I'm being too pedantic.

So if you prefer a minimal approach we could add the !canSend condition only to the shortcut

In this particular case yes, I'd prefer to be more conservative, but I won't insist.

@nicodh
Copy link
Contributor Author

nicodh commented Dec 8, 2024

Returning null would mean to add non null checks in many places. I also don't like that loadDraft is called even in !canSend chats. Maybe it makes sense to add another clearDraft() call in loadDraft before returning.

instead of silently doing nothing

I would say: these functions should never be called, but if called they should not add a draft to a readOnly chat

@nicodh nicodh requested a review from WofWca December 8, 2024 14:16
@WofWca
Copy link
Collaborator

WofWca commented Dec 8, 2024

Returning null would mean to add non null checks in many places.

Well yes, that's the point. You don't want to call a function that you expect to do something but that actually does nothing.

Maybe it makes sense to add another clearDraft() call in loadDraft before returning.

Yes, that's better than keeping the previous state of the draft. And clearDraft() doesn't clear it in core, so it's alright.

@nicodh nicodh merged commit 2b3cc19 into main Dec 8, 2024
11 checks passed
@nicodh nicodh deleted the fix-4389-draft-in-readonly-chats branch December 8, 2024 15:05
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.

Pressing Ctrl+Up in read-only mailing lists sets a draft
2 participants