From 310ec8f2963c9522fa38ebe4daacc88da7ed6e2c Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 1 May 2020 12:44:53 -0400 Subject: [PATCH] Remove CellServiceConstraint in favor of NetworkOrCellServiceConstraint. If a job was enqueued with a CellServiceConstraint (which is currently only SMS jobs), then it'll never run until it gets service, even if you flip the "enable SMS sending over wifi" toggle. This has created bad situations in the past, where SMS jobs just get stuck on devices that never report having cell service (like VM's or wifi only devices). This fixes it by *always* using NetworkOrCellServiceConstraint, and then deciding whether a constraint is met by checking the "wifi SMS" setting at decision-time. --- .../conversation/ConversationItem.java | 3 +- .../impl/CellServiceConstraint.java | 51 ------------------- .../impl/NetworkOrCellServiceConstraint.java | 24 ++++++--- .../securesms/jobs/JobManagerFactories.java | 9 ++-- .../securesms/jobs/SmsSendJob.java | 15 +++--- .../securesms/jobs/SmsSentJob.java | 2 +- .../securesms/sms/MessageSender.java | 6 +-- 7 files changed, 32 insertions(+), 78 deletions(-) delete mode 100644 app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/CellServiceConstraint.java diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationItem.java b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationItem.java index 423ea0b2b..79e8251da 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationItem.java +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationItem.java @@ -1416,8 +1416,7 @@ public class ConversationItem extends LinearLayout implements BindableConversati database.markAsOutbox(messageRecord.getId()); database.markAsForcedSms(messageRecord.getId()); - ApplicationDependencies.getJobManager().add(new SmsSendJob(context, - messageRecord.getId(), + ApplicationDependencies.getJobManager().add(new SmsSendJob(messageRecord.getId(), messageRecord.getIndividualRecipient())); } }); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/CellServiceConstraint.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/CellServiceConstraint.java deleted file mode 100644 index 6c0fda477..000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/CellServiceConstraint.java +++ /dev/null @@ -1,51 +0,0 @@ -package org.thoughtcrime.securesms.jobmanager.impl; - -import android.app.Application; -import android.app.job.JobInfo; -import android.telephony.ServiceState; -import android.telephony.TelephonyManager; - -import androidx.annotation.NonNull; - -import org.thoughtcrime.securesms.jobmanager.Constraint; -import org.thoughtcrime.securesms.sms.TelephonyServiceState; -import org.thoughtcrime.securesms.util.ServiceUtil; - -public class CellServiceConstraint implements Constraint { - - public static final String KEY = "CellServiceConstraint"; - - private final Application application; - - public CellServiceConstraint(@NonNull Application application) { - this.application = application; - } - - @Override - public @NonNull String getFactoryKey() { - return KEY; - } - - @Override - public boolean isMet() { - return CellServiceConstraintObserver.getInstance(application).hasService(); - } - - @Override - public void applyToJobInfo(@NonNull JobInfo.Builder jobInfoBuilder) { - } - - public static final class Factory implements Constraint.Factory { - - private final Application application; - - public Factory(@NonNull Application application) { - this.application = application; - } - - @Override - public CellServiceConstraint create() { - return new CellServiceConstraint(application); - } - } -} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/NetworkOrCellServiceConstraint.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/NetworkOrCellServiceConstraint.java index c17931f97..990bde340 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/NetworkOrCellServiceConstraint.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/impl/NetworkOrCellServiceConstraint.java @@ -5,17 +5,19 @@ import android.app.job.JobInfo; import androidx.annotation.NonNull; import org.thoughtcrime.securesms.jobmanager.Constraint; +import org.thoughtcrime.securesms.util.TextSecurePreferences; public class NetworkOrCellServiceConstraint implements Constraint { - public static final String KEY = "NetworkOrCellServiceConstraint"; + public static final String KEY = "NetworkOrCellServiceConstraint"; + public static final String LEGACY_KEY = "CellServiceConstraint"; - private final NetworkConstraint networkConstraint; - private final CellServiceConstraint serviceConstraint; + private final Application application; + private final NetworkConstraint networkConstraint; - public NetworkOrCellServiceConstraint(@NonNull Application application) { - networkConstraint = new NetworkConstraint.Factory(application).create(); - serviceConstraint = new CellServiceConstraint.Factory(application).create(); + private NetworkOrCellServiceConstraint(@NonNull Application application) { + this.application = application; + this.networkConstraint = new NetworkConstraint.Factory(application).create(); } @Override @@ -25,13 +27,21 @@ public class NetworkOrCellServiceConstraint implements Constraint { @Override public boolean isMet() { - return networkConstraint.isMet() || serviceConstraint.isMet(); + if (TextSecurePreferences.isWifiSmsEnabled(application)) { + return networkConstraint.isMet() || hasCellService(application); + } else { + return hasCellService(application); + } } @Override public void applyToJobInfo(@NonNull JobInfo.Builder jobInfoBuilder) { } + private static boolean hasCellService(@NonNull Application application) { + return CellServiceConstraintObserver.getInstance(application).hasService(); + } + public static class Factory implements Constraint.Factory { private final Application application; diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java index bcafd58bd..00450d5ff 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/JobManagerFactories.java @@ -9,7 +9,6 @@ import org.thoughtcrime.securesms.jobmanager.Constraint; import org.thoughtcrime.securesms.jobmanager.ConstraintObserver; import org.thoughtcrime.securesms.jobmanager.Job; import org.thoughtcrime.securesms.jobmanager.JobMigration; -import org.thoughtcrime.securesms.jobmanager.impl.CellServiceConstraint; import org.thoughtcrime.securesms.jobmanager.impl.CellServiceConstraintObserver; import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraint; import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraintObserver; @@ -142,10 +141,10 @@ public final class JobManagerFactories { public static Map getConstraintFactories(@NonNull Application application) { return new HashMap() {{ - put(CellServiceConstraint.KEY, new CellServiceConstraint.Factory(application)); - put(NetworkConstraint.KEY, new NetworkConstraint.Factory(application)); - put(NetworkOrCellServiceConstraint.KEY, new NetworkOrCellServiceConstraint.Factory(application)); - put(SqlCipherMigrationConstraint.KEY, new SqlCipherMigrationConstraint.Factory(application)); + put(NetworkConstraint.KEY, new NetworkConstraint.Factory(application)); + put(NetworkOrCellServiceConstraint.KEY, new NetworkOrCellServiceConstraint.Factory(application)); + put(NetworkOrCellServiceConstraint.LEGACY_KEY, new NetworkOrCellServiceConstraint.Factory(application)); + put(SqlCipherMigrationConstraint.KEY, new SqlCipherMigrationConstraint.Factory(application)); }}; } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSendJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSendJob.java index d83bb3fdd..5f35d5c0a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSendJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSendJob.java @@ -12,7 +12,6 @@ import android.telephony.SmsManager; import org.thoughtcrime.securesms.jobmanager.Data; import org.thoughtcrime.securesms.jobmanager.Job; import org.thoughtcrime.securesms.jobmanager.impl.NetworkOrCellServiceConstraint; -import org.thoughtcrime.securesms.jobmanager.impl.CellServiceConstraint; import org.thoughtcrime.securesms.database.DatabaseFactory; import org.thoughtcrime.securesms.database.NoSuchMessageException; @@ -39,12 +38,12 @@ public class SmsSendJob extends SendJob { private long messageId; private int runAttempt; - public SmsSendJob(Context context, long messageId, @NonNull Recipient destination) { - this(context, messageId, destination, 0); + public SmsSendJob(long messageId, @NonNull Recipient destination) { + this(messageId, destination, 0); } - public SmsSendJob(Context context, long messageId, @NonNull Recipient destination, int runAttempt) { - this(constructParameters(context, destination), messageId, runAttempt); + public SmsSendJob(long messageId, @NonNull Recipient destination, int runAttempt) { + this(constructParameters(destination), messageId, runAttempt); } private SmsSendJob(@NonNull Job.Parameters parameters, long messageId, int runAttempt) { @@ -226,13 +225,11 @@ public class SmsSendJob extends SendJob { } } - private static Job.Parameters constructParameters(@NonNull Context context, @NonNull Recipient destination) { - String constraint = TextSecurePreferences.isWifiSmsEnabled(context) ? NetworkOrCellServiceConstraint.KEY - : CellServiceConstraint.KEY; + private static Job.Parameters constructParameters(@NonNull Recipient destination) { return new Job.Parameters.Builder() .setMaxAttempts(MAX_ATTEMPTS) .setQueue(destination.getId().toQueueKey()) - .addConstraint(constraint) + .addConstraint(NetworkOrCellServiceConstraint.KEY) .build(); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSentJob.java b/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSentJob.java index cbd669811..a5f455868 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSentJob.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/SmsSentJob.java @@ -103,7 +103,7 @@ public class SmsSentJob extends BaseJob { case SmsManager.RESULT_ERROR_NO_SERVICE: case SmsManager.RESULT_ERROR_RADIO_OFF: Log.w(TAG, "Service connectivity problem, requeuing..."); - ApplicationDependencies.getJobManager().add(new SmsSendJob(context, messageId, record.getIndividualRecipient(), runAttempt + 1)); + ApplicationDependencies.getJobManager().add(new SmsSendJob(messageId, record.getIndividualRecipient(), runAttempt + 1)); break; default: database.markAsSentFailed(messageId); diff --git a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java index b3544def6..3790228da 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java +++ b/app/src/main/java/org/thoughtcrime/securesms/sms/MessageSender.java @@ -380,7 +380,7 @@ public class MessageSender { } else if (!forceSms && isPushTextSend(context, recipient, keyExchange)) { sendTextPush(recipient, messageId); } else { - sendSms(context, recipient, messageId); + sendSms(recipient, messageId); } } @@ -411,9 +411,9 @@ public class MessageSender { } } - private static void sendSms(Context context, Recipient recipient, long messageId) { + private static void sendSms(Recipient recipient, long messageId) { JobManager jobManager = ApplicationDependencies.getJobManager(); - jobManager.add(new SmsSendJob(context, messageId, recipient)); + jobManager.add(new SmsSendJob(messageId, recipient)); } private static void sendMms(Context context, long messageId) {