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

Initial focus in message compose screen #8664

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shamim-emon
Copy link
Contributor

@wmontwe wmontwe self-assigned this Jan 6, 2025
Copy link
Member

@wmontwe wmontwe left a comment

Choose a reason for hiding this comment

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

For future changes: I think breaking down the code changes into separate commits would help us to review your changes better. Consider applying each change individually: move Java to a new class, then convert to Kotlin, and finally refactor.

Besides that it works as expected and it seems that the corner cases still work.

Just the RecipientMvpView doesn't need to be a member of MessageCompose as it's only needed during onCreate

@@ -222,6 +223,7 @@ public class MessageCompose extends K9Activity implements OnClickListener,

private Action action;

private RecipientMvpView recipientMvpView;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of creating a class member that could leak, it's better to keep it as is and change the messageLoaderCallbacks to a private class and inject the recipientMvpView when creating it.

    private MessageLoaderCallbacks messageLoaderCallbacks = new MessageLoaderCallbacks() {
    private class MessageComposeMessageLoaderCallback implements MessageLoaderCallbacks {

        private final RecipientMvpView recipientMvpView;

        public MessageComposeMessageLoaderCallback(RecipientMvpView recipientMvpView) {
            this.recipientMvpView = recipientMvpView;
        }
    ...

This is possible as recipientMvpView is only needed in onCreate.

@@ -304,7 +306,7 @@ public void onCreate(Bundle savedInstanceState) {
ReplyToView replyToView = new ReplyToView(this);
replyToPresenter = new ReplyToPresenter(replyToView);

RecipientMvpView recipientMvpView = new RecipientMvpView(this);
recipientMvpView = new RecipientMvpView(this);
Copy link
Member

Choose a reason for hiding this comment

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

See above regarding recipientMvpView

@@ -1677,6 +1598,14 @@ public void onMessageDataLoadFailed() {
public void onMessageViewInfoLoadFinished(MessageViewInfo messageViewInfo) {
internalMessageHandler.sendEmptyMessage(MSG_PROGRESS_OFF);
loadLocalMessageForDisplay(messageViewInfo, action);

Copy link
Member

Choose a reason for hiding this comment

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

See above regarding recipientMvpView

import org.openintents.openpgp.util.OpenPgpApi

@Suppress("NestedBlockDepth", "MaxLineLength")
class IntentDataMapper {
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be covered by tests.

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.

Initial focus in message compose screen
2 participants