Closed Bug 998000 Opened 6 years ago Closed 4 years ago

Create BrowserApp.cancelEditingMode for consistency with BrowserApp.enter/commitEditingMode

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mcomella, Assigned: mcomella)

Details

(Whiteboard: [leave-open])

Attachments

(3 files, 3 obsolete files)

If we want to yank the go button, we should make sure nobody uses it first. We should check total editing mode commits vs. commits via the go button.
Assignee: nobody → michael.l.comella
Summary: Add UI telemetry for editing go button → Add UI telemetry for editing mode go button
While functionally equivalent (for now), this was previously incorrect.
Attachment #8408659 - Flags: review?(lucasr.at.mozilla)
Eclipse demands style assimilation.
Attachment #8408660 - Flags: review?(lucasr.at.mozilla)
For consistency with BrowserApp.enterEditingMode/commitEditingMode as the primary entry points of editing mode, I created BrowserApp.cancelEditingMode (rather than BrowserToolbar.cancelEdit), which should make the telemetry start/stop points more maintainable.
Attachment #8408661 - Flags: review?(lucasr.at.mozilla)
lucasr for the editing mode entry points; liuche for the telemetry bits.
Attachment #8408662 - Flags: review?(lucasr.at.mozilla)
Attachment #8408662 - Flags: review?(liuche)
Eclipse tricked me into putting in three spaces. :(
Attachment #8408665 - Flags: review?(lucasr.at.mozilla)
Attachment #8408661 - Attachment is obsolete: true
Attachment #8408661 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8408662 [details] [diff] [review]
Part 3: Add UITelemetry for editing mode go button.

Drive by: I like this.
Comment on attachment 8408662 [details] [diff] [review]
Part 3: Add UITelemetry for editing mode go button.

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

Simple, nice.

(Side note, some of these patches are a tiny bit bitrotted from the refactoring in bug 965548.)

::: mobile/android/base/TelemetryContract.java
@@ +51,5 @@
>          // Started when a user enters a given home panel.
>          // Session name is dynamic, encoded as "homepanel.1:<panel_id>"
>          public static final String HOME_PANEL = "homepanel.1:";
> +
> +        // Started when a user enters editing mode.

Nit: I realize that I don't like this comment style because it only describes when session is started, not the session itself ("User in urlbar editing mode"?).

@@ +61,5 @@
>       * Telemetry.stopUISession() as the "reason" parameter.
>       */
> +    public interface Reason {
> +        // Changes were cancelled.
> +        public static final String CANCEL = "cancel";

I thought about whether we should also version reasons, and decided that shouldn't be necessary because it'll always be tied to a session, which can carry the version.
Attachment #8408662 - Flags: review?(liuche) → review+
Comment on attachment 8408660 [details] [diff] [review]
Part 1: Fix whitespace.

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

If you're touching this file anyways, I forgot to mention that the comment for enterEditingMode() is wrong, so you could fix that in this patch, too.
Comment on attachment 8408659 [details] [diff] [review]
Part 0: cancelEdit on back pressed, rather than stopEdit.

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

Makes sense.
Attachment #8408659 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8408660 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8408662 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8408665 [details] [diff] [review]
Part 2: Create BrowserApp.cancelEditingMode, for consistency.

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

Giving f+ to understand how the dismissListener changes relate to cancelEditingMode().

::: mobile/android/base/BrowserApp.java
@@ +1613,5 @@
>          });
>      }
>  
> +    private void cancelEditingMode() {
> +        if (!mBrowserToolbar.isEditing()) {

Doesn't cancelEdit() (through stopEditing()) already checks that for us?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +141,5 @@
>  
>      private boolean shouldShrinkURLBar = false;
>  
>      private OnActivateListener activateListener;
> +    private OnDismissListener dismissListener;

I don't follow. What does the dismissListener changes have to do with creating the cancelEditingMode() method?

@@ +369,5 @@
>          if (editCancel != null) {
>              editCancel.setOnClickListener(new OnClickListener() {
>                  @Override
>                  public void onClick(View v) {
> +                    dismissListener.onDismiss();

Always do a null check before using listeners.

@@ +397,5 @@
>      }
>  
>      public boolean onBackPressed() {
>          if (isEditing()) {
> +            this.dismissListener.onDismiss();

Ditto for null check. Also, 'this' is not necessary here.

@@ +810,5 @@
>          urlEditLayout.setOnCommitListener(listener);
>      }
>  
>      public void setOnDismissListener(OnDismissListener listener) {
> +        this.dismissListener = listener;

Ditto for 'this'.
Attachment #8408665 - Flags: review?(lucasr.at.mozilla) → feedback+
We decided to yank the go button in bug 1000149, so I'm changing this bug to be limited with the refactoring in parts 0-2.
Status: NEW → ASSIGNED
Summary: Add UI telemetry for editing mode go button → Create BrowserApp.cancelEditingMode for consistency with BrowserApp.enter/commitEditingMode
Attachment #8408662 - Attachment is obsolete: true
remote:   https://hg.mozilla.org/integration/fx-team/rev/2ca6c9728ed9
remote:   https://hg.mozilla.org/integration/fx-team/rev/bdd4cddb50f8

Landed parts 0 and 1 because it was convenient to have part 1 in bug 1000149.
Whiteboard: [leave-open]
(In reply to Lucas Rocha (:lucasr) from comment #10)
> I don't follow. What does the dismissListener changes have to do with
> creating the cancelEditingMode() method?

BrowserApp.enterEditingMode/commitEditingMode are the primary entry points into editing mode for those functions (i.e. BrowserToolbar.startEditing/commitEditing are only called by the BrowserApp methods). Having a single entry point for each function ensures much better consistency. The intent of this patch is create such an entry point for cancelEditingMode.

Since BrowserToolbar does not have a reference to the BrowserApp instance, it can cancel editing mode via the `onDismiss` listener, as it does with commits (`onCommit`) and entry into editing mode (`onActivate`).

This makes me think this is confusing - I'll add some comments. 

> Doesn't cancelEdit() (through stopEditing()) already checks that for us?

Yes, good catch. Given what I said above, I would say we should remove the call in `cancelEdit` (note that `BrowserApp.commitEditingMode` makes the same mistake I did), and move the call into `BrowserApp.enterEditingMode` from `BrowserToolbar.startEditing`.
Updated the patch in accordance with nits (comment 10) and my responses (comment 14).
Attachment #8412270 - Flags: review?(lucasr.at.mozilla)
Attachment #8408665 - Attachment is obsolete: true
Comment on attachment 8412270 [details] [diff] [review]
Part 2: Create BrowserApp.cancelEditingMode, for consistency.

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

I think I need a bit more background to review this one. At first sight, it feels a bit backwards. The toolbar methods for switching to edit/display modes should be idempotent, independently of the toolbar user -- BrowserApp in this case. For example, what happens if some other code path calls startEditing() while the toolbar is already in editing mode? With this change, it seems we'll end up showing some glitchy animation to 'switch' to the same state, no?
Attachment #8412270 - Flags: review?(lucasr.at.mozilla)
Let's reopen this (or file a new one) if we feel it's important in the future.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.