1086983 - Restore editing mode text when switching tabs on new tablet

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Some ideas:
  * The hack: we can store the text as a variable and restore it when the tab is switched back to
  * Better, but perhaps v2?: Have an editing mode per tab and we technically don't have to "restore" state - editing mode is essentially its own page

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Restore editing text when switching between tabs in editing mode

Part 2 will be to restore the editing mode state nearly exactly (browser search, editing text, and selection).

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Restore editing text when switching between tabs in editing mode

Restores all of the editing mode state now, folded into the original part 1. Still to go: restored text includes the auto-complete so removing that. Note that I wasn't sure where to put TabEditingState.

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8518211 [details] [diff] [review]
Part 1: Restore editing text when switching between tabs in editing mode

Review of attachment 8518211 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +235,5 @@
>      // The tab to be selected on editing mode exit.
>      private Integer mTargetTabForEditingMode;
>  
> +    // The edited text in the toolbar from the most recent tab to be unselected.
> +    private String unselectedTabEditingText;

mUnselectedTabEditingText for consistency.

@@ +332,5 @@
> +
> +        super.onTabChanged(tab, msg, data);
> +    }
> +
> +    private void updateEditingMode(final Tab selectedTab) {

updateEditingModeForTab?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +803,5 @@
>       */
>      public String cancelEdit() {
> +        final Tab selectedTab = Tabs.getInstance().getSelectedTab();
> +        if (selectedTab != null) {
> +            selectedTab.setIsEditing(false);

Instead of update the 'isEditing' tab state in different spots, maybe it should all be done in BrowserApp's stop/start editing listeners.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 8518249 [details] [diff] [review]
Part 1: Restore editing text when switching between tabs in editing mode

Review of attachment 8518249 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good with the stop/start listener changes.

::: mobile/android/base/BrowserApp.java
@@ +333,5 @@
> +
> +        super.onTabChanged(tab, msg, data);
> +    }
> +
> +    private void updateEditingMode(final Tab selectedTab) {

updateEditingModeForTab?

@@ +342,5 @@
> +        storeEditingState(mLastEditingState);
> +
> +        if (selectedTab.isEditing()) {
> +            enterEditingMode();
> +            restoreEditingState(selectedTab.mEditingState);

Add a getter to the editing state? To avoid breaking encapsulation and all.

@@ +348,5 @@
>              mBrowserToolbar.cancelEdit();
>          }
> +    }
> +
> +    private void storeEditingState(final TabEditingState editingState) {

saveEditingState? store/restore look confusingly similar :-)

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +415,5 @@
>              });
>          }
>      }
>  
> +    public void storeEditingState(final TabEditingState editingState) {

Maybe storeTabEditingState for clarity?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Restore editing text when switching between tabs in editing mode

Updated for nits.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 2: Do not restore autocomplete text from editing mode

Well, that was easy.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8518271 [details] [diff] [review]
Part 1: Restore editing text when switching between tabs in editing mode

Review of attachment 8518271 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8518298 [details] [diff] [review]
Part 2: Do not restore autocomplete text from editing mode

Review of attachment 8518298 [details] [diff] [review]:
-----------------------------------------------------------------

Great.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/17dd668f5444
https://hg.mozilla.org/integration/fx-team/rev/3357e9e81614

Comment 10

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1129492&repo=fx-team

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Seems to have hit hit my assertion that we have the selected tab when calling updateEditingModeFromTab(Tab):

06:31:04 WARNING - PROCESS-CRASH | java-exception | java.lang.IllegalStateException: Expected given tab to be selected at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:331)

The issue is that `selectTab` is called from the gecko thread in `Tabs.handleMessage`, which updates `mSelectedTab`, then posts to the UI thread to notify `onTabChanged(TabEvents.SELECTED)` listeners. Before the listeners can run again (using that selected tab as their argument), `Tabs.handleMessage` can be called again, selecting a new tab, changing `mSelectedTab` directly, and invalidating the tab passed to the listeners.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Part 1: Restore editing text when switching between tabs in editing mode

Decided to log instead of throw; also rebase.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #12)
> Decided to log instead of throw; also rebase.

Motivation from a comment in the patch:

> (bug 1086983 comment 11) Because the tab may be selected from the gecko thread and we're
> running this code on the UI thread, the selected tab argument may not still refer to the
. selected tab. However, that means this code should be run again and the initial state
> changes will be overridden. As an optimization, we can skip this update, but it may have
> unknown side-effects so we don't.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/53d7570e20f2
https://hg.mozilla.org/integration/fx-team/rev/36e434d7d990

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53d7570e20f2
https://hg.mozilla.org/mozilla-central/rev/36e434d7d990