Closed
Bug 998000
Opened 10 years ago
Closed 9 years ago
Create BrowserApp.cancelEditingMode for consistency with BrowserApp.enter/commitEditingMode
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: mcomella, Assigned: mcomella)
Details
(Whiteboard: [leave-open])
Attachments
(3 files, 3 obsolete files)
1.01 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
2.91 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
11.18 KB,
patch
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Updated•10 years ago
|
Summary: Add UI telemetry for editing go button → Add UI telemetry for editing mode go button
Assignee | ||
Comment 1•10 years ago
|
||
While functionally equivalent (for now), this was previously incorrect.
Attachment #8408659 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 2•10 years ago
|
||
Eclipse demands style assimilation.
Attachment #8408660 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
lucasr for the editing mode entry points; liuche for the telemetry bits.
Attachment #8408662 -
Flags: review?(lucasr.at.mozilla)
Attachment #8408662 -
Flags: review?(liuche)
Assignee | ||
Comment 5•10 years ago
|
||
Eclipse tricked me into putting in three spaces. :(
Attachment #8408665 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8408661 -
Attachment is obsolete: true
Attachment #8408661 -
Flags: review?(lucasr.at.mozilla)
Comment 6•10 years ago
|
||
Comment on attachment 8408662 [details] [diff] [review] Part 3: Add UITelemetry for editing mode go button. Drive by: I like this.
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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 9•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8408660 -
Flags: review?(lucasr.at.mozilla) → review+
Updated•10 years ago
|
Attachment #8408662 -
Flags: review?(lucasr.at.mozilla) → review+
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8408662 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
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]
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ca6c9728ed9 https://hg.mozilla.org/mozilla-central/rev/bdd4cddb50f8
Assignee | ||
Comment 14•10 years ago
|
||
(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`.
Assignee | ||
Comment 15•10 years ago
|
||
Updated the patch in accordance with nits (comment 10) and my responses (comment 14).
Attachment #8412270 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8408665 -
Attachment is obsolete: true
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
Let's reopen this (or file a new one) if we feel it's important in the future.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•