Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #3265: Add support for "undo" functionality. #8449

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

pocmo
Copy link
Contributor

@pocmo pocmo commented Sep 21, 2020

This is not the fancy version yet since we still need to restore into SessionManager. Once it is gone and
we rely on BrowserStore only, then we can make this better.

However moving this functionality into AC now helps us:

  • It will be easier to migrate to a better undo functionality since this code is already in AC.
  • Other code can interact with the "undo" actions. For example "recently closed tabs" now will
    only contain a tab if the removal was not undone.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@pocmo pocmo added the 🕵️‍♀️ needs review PRs that need to be reviewed label Sep 21, 2020
@grigoryk grigoryk self-requested a review September 21, 2020 17:26
@Amejia481
Copy link
Contributor

There is one test failing on build-browser-session

[task 2020-09-21T13:54:20.635Z]   TEST: Undo History gets cleared after time
[task 2020-09-21T13:54:20.635Z]   FAILURE
[task 2020-09-21T13:54:20.635Z] 
[task 2020-09-21T13:54:20.635Z] java.lang.AssertionError: expected:<https://reddit.com/r/firefox> but was:<Session(reddit, https://reddit.com/r/firefox)>
[task 2020-09-21T13:54:20.635Z] 	at org.junit.Assert.fail(Assert.java:88)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.Assert.failNotEquals(Assert.java:834)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.Assert.assertEquals(Assert.java:118)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.Assert.assertEquals(Assert.java:144)
[task 2020-09-21T13:54:20.635Z] 	at mozilla.components.browser.session.undo.UndoMiddlewareTest.Undo History gets cleared after time(UndoMiddlewareTest.kt:217)
[task 2020-09-21T13:54:20.635Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[task 2020-09-21T13:54:20.635Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[task 2020-09-21T13:54:20.635Z] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[task 2020-09-21T13:54:20.635Z] 	at java.lang.reflect.Method.invoke(Method.java:498)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
[task 2020-09-21T13:54:20.635Z] 	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
[task 2020-09-21T13:54:20.636Z] 	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[task 2020-09-21T13:54:20.636Z] 	at java.lang.reflect.Method.invoke(Method.java:498)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94)
[task 2020-09-21T13:54:20.636Z] 	at com.sun.proxy.$Proxy2.processTestClass(Unknown Source)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:118)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[task 2020-09-21T13:54:20.636Z] 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[task 2020-09-21T13:54:20.636Z] 	at java.lang.reflect.Method.invoke(Method.java:498)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:412)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
[task 2020-09-21T13:54:20.636Z] 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
[task 2020-09-21T13:54:20.636Z] 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
[task 2020-09-21T13:54:20.636Z] 	at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
[task 2020-09-21T13:54:20.636Z] 	at java.lang.Thread.run(Thread.java:748)

Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

It looks good, I like the creative way to solve it ✨

  • Should we add some docs around how to use the new undo functionality or are we going to wait for the BrowserStore version to do it? :)

  • Should we add this to the change log?

@pocmo pocmo force-pushed the undo-prototype branch 2 times, most recently from 4b5a6eb to 427f8e3 Compare September 22, 2020 10:19
This is not the fancy version yet since we still need to restore into SessionManager. Once it is gone and
we rely on BrowserStore only, then we can make this better.

However moving this functionality into AC now helps us:
- It will be easier to migrate to a better undo functionality since this code is already in AC.
- Other code can interact with the "undo" actions. For example "recently closed tabs" now will
  only contain a tab if the removal was not undone.
@pocmo
Copy link
Contributor Author

pocmo commented Sep 22, 2020

Added it to the changelog.

@pocmo pocmo added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Sep 22, 2020
@mergify mergify bot merged commit c5a4e0d into mozilla-mobile:master Sep 22, 2020
@pocmo pocmo deleted the undo-prototype branch September 22, 2020 17:59
pocmo added a commit to pocmo/fenix that referenced this pull request Sep 22, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
pocmo added a commit to pocmo/fenix that referenced this pull request Sep 22, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
pocmo added a commit to pocmo/fenix that referenced this pull request Sep 22, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
ekager pushed a commit to pocmo/fenix that referenced this pull request Sep 23, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
pocmo added a commit to pocmo/fenix that referenced this pull request Sep 28, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
ekager pushed a commit to mozilla-mobile/fenix that referenced this pull request Sep 28, 2020
This is not the super fancy version yet - since we still need to restore into SessionManager and
haven't fully switched to BrowserStore yet. However AC having knowledge about "undo" and whether
it was performed or not, will help us with features like "recently closed tabs". And once we
can improve "undo", Fenix will get all the nice things automatically.

Requires:
mozilla-mobile/android-components#8449
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants