Closed Bug 1086983 Opened 10 years ago Closed 10 years ago

Restore editing mode text when switching tabs on new tablet

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(2 files, 3 obsolete files)

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
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Part 2 will be to restore the editing mode state nearly exactly (browser search, editing text, and selection).
Attachment #8518211 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8518249 - Flags: review?(lucasr.at.mozilla)
Attachment #8518211 - Attachment is obsolete: true
Attachment #8518211 - Flags: review?(lucasr.at.mozilla)
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 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?
Attachment #8518249 - Flags: review?(lucasr.at.mozilla) → feedback+
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.
Attachment #8518271 - Flags: review?(lucasr.at.mozilla) → review+
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.
Attachment #8518298 - Flags: review?(lucasr.at.mozilla) → review+
Flags: needinfo?(michael.l.comella)
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.
Flags: needinfo?(michael.l.comella)
(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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: