From 19534a138da90461733ce33e464c386eab204ae0 Mon Sep 17 00:00:00 2001 From: Mikhail Lopatkin Date: Mon, 25 Dec 2023 19:48:43 +0100 Subject: [PATCH] Suppress disconnect warning dialog until adb restart completes Issue: #197 --- .../ddmlib/DeviceDisconnectedHandler.java | 30 ++++++++++++- .../AdbServicesInitializationPresenter.java | 23 +++++++--- .../ddmlib/DeviceDisconnectedHandlerTest.java | 44 +++++++++++++++++++ ...dbServicesInitializationPresenterTest.java | 15 +++++-- 4 files changed, 102 insertions(+), 10 deletions(-) diff --git a/src/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandler.java b/src/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandler.java index 6372b3d4..72ab313e 100644 --- a/src/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandler.java +++ b/src/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandler.java @@ -20,6 +20,8 @@ import name.mlopatkin.andlogview.ui.mainframe.ErrorDialogs; import name.mlopatkin.andlogview.ui.mainframe.MainFrameScoped; +import org.checkerframework.checker.nullness.qual.Nullable; + import java.util.concurrent.Executor; import javax.inject.Inject; @@ -39,6 +41,9 @@ public interface DeviceAwaiter { private final AdbConfigurationPref adbConfigurationPref; private final Executor uiExecutor; + private boolean isSuppressed; + private @Nullable String suppressedError; + @Inject DeviceDisconnectedHandler( DeviceAwaiter deviceAwaiter, @@ -69,9 +74,11 @@ public void onDataSourceInvalidated(AdbDataSource.InvalidationReason reason) { private void onDeviceDisconnected(String message) { if (adbConfigurationPref.isAutoReconnectEnabled()) { deviceAwaiter.waitForDevice(); - } else { + } else if (!isSuppressed) { // The dialog is modal, and must be shown in async manner. Otherwise, we'll block all other listeners. uiExecutor.execute(() -> showNotificationDialog(message)); + } else { + suppressedError = message; } } @@ -88,4 +95,25 @@ private void showNotificationDialog(String message) { public void startWatching(AdbDataSource dataSource) { dataSource.asStateObservable().addObserver(this); } + + /** + * Suppresses error dialogs until {@link #resumeDialogs()} is called. + */ + public void suppressDialogs() { + isSuppressed = true; + } + + /** + * Resumes showing error dialogs. Can show the last suppressed error if any. + * + */ + public void resumeDialogs() { + isSuppressed = false; + var suppressedError = this.suppressedError; + this.suppressedError = null; + + if (suppressedError != null) { + onDeviceDisconnected(suppressedError); + } + } } diff --git a/src/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenter.java b/src/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenter.java index ffaea040..18b65e02 100644 --- a/src/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenter.java +++ b/src/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenter.java @@ -21,6 +21,7 @@ import name.mlopatkin.andlogview.AppExecutors; import name.mlopatkin.andlogview.device.AdbDeviceList; +import name.mlopatkin.andlogview.liblogcat.ddmlib.DeviceDisconnectedHandler; import name.mlopatkin.andlogview.ui.mainframe.MainFrameScoped; import name.mlopatkin.andlogview.utils.Cancellable; import name.mlopatkin.andlogview.utils.MyFutures; @@ -63,6 +64,7 @@ public interface View { private final AdbServicesBridge bridge; private final View view; private final Executor uiExecutor; + private final DeviceDisconnectedHandler deviceDisconnectedHandler; // Maintains set of the current requests to show loading progress. Useful, when new show and old hide requests // overlap because of the delayed execution of hide callback. private final Set progressTokens = new HashSet<>(); @@ -70,11 +72,15 @@ public interface View { private boolean hasShownErrorMessage; @Inject - AdbServicesInitializationPresenter(AdbServicesBridge bridge, View view, - @Named(AppExecutors.UI_EXECUTOR) Executor uiExecutor) { + AdbServicesInitializationPresenter( + View view, + AdbServicesBridge bridge, + @Named(AppExecutors.UI_EXECUTOR) Executor uiExecutor, + DeviceDisconnectedHandler deviceDisconnectedHandler) { this.bridge = bridge; this.view = view; this.uiExecutor = uiExecutor; + this.deviceDisconnectedHandler = deviceDisconnectedHandler; } /** @@ -93,7 +99,7 @@ public AdbDeviceList withAdbDeviceList() { * Initializes the adb services and executes the action. This method should be used when the user triggers the * action, it takes care of indicating the pause. The actions are executed on the UI executor. *

- * The initialization may be cancelled by using the returned handle. When cancelled, no callbacks are invoked. + * The initialization may be cancelled by using the returned handle. When cancelled, the failure handler is invoked. * * @param action the action to execute * @param failureHandler the failure handler @@ -111,7 +117,7 @@ public Cancellable withAdbServicesInteractive(Consumer acti uiExecutor); } future.handleAsync( - consumingHandler(action, ignoreCancellations(adbErrorHandler().andThen(failureHandler))), + consumingHandler(action, ignoreCancellations(adbErrorHandler()).andThen(failureHandler)), uiExecutor) .exceptionally(MyFutures::uncaughtException); // TODO(mlopatkin) Should we always show a failure message if the ADB fails/is failed for the interactive @@ -142,8 +148,10 @@ private void hideProgressWithToken(Object token) { private Consumer adbErrorHandler() { return ignored -> { if (!hasShownErrorMessage) { - view.showAdbLoadingError(); hasShownErrorMessage = true; + // showAdbLoadingError blocks and opens a nested message pump. It is important to set up the flag before + // showing the dialog to prevent re-entrance and double dialog. + view.showAdbLoadingError(); } }; } @@ -154,6 +162,9 @@ private Consumer adbErrorHandler() { public void restartAdb() { bridge.stopAdb(); hasShownErrorMessage = false; - withAdbServicesInteractive(adb -> {}, failure -> {}); + deviceDisconnectedHandler.suppressDialogs(); + withAdbServicesInteractive( + adbServices -> deviceDisconnectedHandler.resumeDialogs(), + failure -> deviceDisconnectedHandler.resumeDialogs()); } } diff --git a/test/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandlerTest.java b/test/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandlerTest.java index 7ce3afb5..126eca74 100644 --- a/test/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandlerTest.java +++ b/test/name/mlopatkin/andlogview/liblogcat/ddmlib/DeviceDisconnectedHandlerTest.java @@ -17,6 +17,7 @@ package name.mlopatkin.andlogview.liblogcat.ddmlib; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -65,6 +66,49 @@ void waitsForDeviceWhenDeviceDisconnectsAndAutoReconnectEnabled(AdbDataSource.In assertWaitForDevice(); } + @ParameterizedTest + @EnumSource(AdbDataSource.InvalidationReason.class) + void errorMessagesCanBeSuppressed(AdbDataSource.InvalidationReason reason) { + when(adbConfigurationPref.isAutoReconnectEnabled()).thenReturn(false); + + var handler = createHandler(); + handler.suppressDialogs(); + handler.onDataSourceInvalidated(reason); + + assertNoErrorDialogShown(); + } + + @ParameterizedTest + @EnumSource(AdbDataSource.InvalidationReason.class) + void errorMessagesAreShownAfterResuming(AdbDataSource.InvalidationReason reason) { + when(adbConfigurationPref.isAutoReconnectEnabled()).thenReturn(false); + + var handler = createHandler(); + handler.suppressDialogs(); + handler.resumeDialogs(); + handler.onDataSourceInvalidated(reason); + + assertErrorDialogShown(); + } + + @ParameterizedTest + @EnumSource(AdbDataSource.InvalidationReason.class) + void suppressedErrorMessagesAreShownAfterResuming(AdbDataSource.InvalidationReason reason) { + when(adbConfigurationPref.isAutoReconnectEnabled()).thenReturn(false); + + var handler = createHandler(); + handler.suppressDialogs(); + handler.onDataSourceInvalidated(reason); + handler.resumeDialogs(); + + assertErrorDialogShown(); + } + + private void assertNoErrorDialogShown() { + verify(errorDialogs, never()).showDeviceDisconnectedWarning(any()); + verifyNoInteractions(deviceAwaiter); + } + private void assertErrorDialogShown() { verify(errorDialogs).showDeviceDisconnectedWarning(any()); verifyNoInteractions(deviceAwaiter); diff --git a/test/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenterTest.java b/test/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenterTest.java index 85962cf4..b57d7cd9 100644 --- a/test/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenterTest.java +++ b/test/name/mlopatkin/andlogview/ui/device/AdbServicesInitializationPresenterTest.java @@ -27,6 +27,7 @@ import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; +import name.mlopatkin.andlogview.base.MyThrowables; import name.mlopatkin.andlogview.base.concurrent.TestExecutor; import name.mlopatkin.andlogview.device.AdbException; import name.mlopatkin.andlogview.test.ThreadTestUtils; @@ -42,10 +43,12 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import org.mockito.Answers; +import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import java.util.concurrent.CancellationException; import java.util.concurrent.CompletableFuture; import java.util.concurrent.Executor; import java.util.function.Consumer; @@ -107,7 +110,7 @@ void cancelledRequestShowsNoMessagesIfAdbFails(ServiceRequest request, AdbInitRe adbInit.tryInitAdb(this); - thenRequestNotCompleted(); + thenRequestCancelled(); thenNoErrorIsShown(); } @@ -313,7 +316,7 @@ private AdbServicesInitializationPresenter createPresenter(Executor uiExecutor) var mockBridge = mock(AdbServicesBridge.class); lenient().when(mockBridge.getAdbServicesAsync()) .thenAnswer(invocation -> servicesFuture.thenApply(Function.identity())); - return new AdbServicesInitializationPresenter(mockBridge, view, uiExecutor); + return new AdbServicesInitializationPresenter(view, mockBridge, uiExecutor, mock()); } private void whenAdbInitFailed() { @@ -351,6 +354,12 @@ private void thenRequestFailed() { verify(errorConsumer).accept(any()); } + private void thenRequestCancelled() { + verify(servicesConsumer, never()).accept(any()); + verify(errorConsumer).accept(ArgumentMatchers.argThat( + failure -> MyThrowables.unwrapUninteresting(failure) instanceof CancellationException)); + } + private void thenNoProgressIsShown() { verify(view, never()).showAdbLoadingProgress(); } @@ -427,7 +436,7 @@ public String toString() { } @FunctionalInterface - interface AdbInitRef { + interface AdbInitRef { void tryInitAdb(AdbServicesInitializationPresenterTest test); }