Closed Bug 1100742 Opened 7 years ago Closed 7 years ago

Long back press vibration is too strong

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox36 fixed, firefox37 fixed, fennec36+)

RESOLVED FIXED
Firefox 37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed
fennec 36+ ---

People

(Reporter: bnicholson, Assigned: vivek)

References

Details

Attachments

(1 file, 4 obsolete files)

The long back press vibration should last as long as the vibration for other long presses, such as the URL bar. It's noticeably stronger than that.

Vivek, do you want to take this? This should just be a matter of testing different durations to find a value that feels consistent. Sorry, I should have double-checked a build before giving r+!
Flags: needinfo?(vivekb.balakrishnan)
Patch for milder vibration on long press:
* Declared long press duration value similar to that of android framework.
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8524887 - Flags: review?(bnicholson)
Attachment #8524887 - Attachment is patch: true
Comment on attachment 8524887 [details] [diff] [review]
Long presses vibration intensity reduced

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

Just tested this patch and it feels perfect. Just needs some cleanup as described below.

Also, it's usually a good idea to include the bug #, description, and r= in your patch header before requesting review. Then you won't need to post another one after r+!

::: mobile/android/base/GeckoAppShell.java
@@ +1424,5 @@
> +        }
> +        return output;
> +    }
> +
> +

Nit: remove extra newline (although you should remove this method entirely anyway as described below)

@@ +1431,2 @@
>          if (Settings.System.getInt(getContext().getContentResolver(), Settings.System.HAPTIC_FEEDBACK_ENABLED, 0) > 0) {
> +            vibrate(convertIntToLongArray(milliseconds), -1);

Instead of convertIntToLongArray, you should be able to use getLongIntArray: http://androidxref.com/5.0.0_r2/xref/frameworks/base/policy/src/com/android/internal/policy/impl/PhoneWindowManager.java#1084

::: mobile/android/base/resources/values/arrays.xml
@@ +146,5 @@
>      <string-array name="pref_titlebar_mode_values">
>          <item>0</item>
>          <item>1</item>
>      </string-array>
> +    <!-- This value is similar to config_longPressVibePattern in android frameworks/base/core/res/res/values/config.xml-->

similar to -> equal to
Attachment #8524887 - Flags: review?(bnicholson) → feedback+
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Instead of convertIntToLongArray, you should be able to use getLongIntArray:
> http://androidxref.com/5.0.0_r2/xref/frameworks/base/policy/src/com/android/
> internal/policy/impl/PhoneWindowManager.java#1084

Err, ignore this comment. I see now that this method is defined exactly the same way at http://androidxref.com/5.0.0_r2/xref/frameworks/base/policy/src/com/android/internal/policy/impl/PhoneWindowManager.java#5236.
Attached patch milder long back press vibration (obsolete) — Splinter Review
Review nit corrected
Attachment #8524887 - Attachment is obsolete: true
Attachment #8525420 - Flags: review?(bnicholson)
Comment on attachment 8525420 [details] [diff] [review]
milder long back press vibration

Looks great, thanks!
Attachment #8525420 - Flags: review?(bnicholson) → review+
Assignee: nobody → vivekb.balakrishnan
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #8525420 - Attachment is patch: true
Hi,

this patch didn't apply cleanly:

applying file_1100742.txt
patching file mobile/android/base/BrowserApp.java
Hunk #1 FAILED at 523
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/BrowserApp.java.rej
patching file mobile/android/base/GeckoAppShell.java
Hunk #1 FAILED at 1409
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/GeckoAppShell.java.rej
patching file mobile/android/base/resources/values/arrays.xml
Hunk #1 FAILED at 141
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/arrays.xml.rej
patching file mobile/android/base/resources/values/integers.xml
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/integers.xml.rej

could you take a look thanks!
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
Attached file 1100742 patch (obsolete) —
Patch rebased with latest tree
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
hm still don't apply cleanly

patching file mobile/android/base/BrowserApp.java
Hunk #1 FAILED at 523
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/BrowserApp.java.rej
patching file mobile/android/base/GeckoAppShell.java
Hunk #1 FAILED at 1409
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/GeckoAppShell.java.rej
patching file mobile/android/base/resources/values/arrays.xml
Hunk #1 FAILED at 141
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/arrays.xml.rej
patching file mobile/android/base/resources/values/integers.xml
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/resources/values/integers.xml.rej
patch failed, unable to continue (try -v)

does the patch apply for you cleanly ?
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
Attached file 1100742 patch (obsolete) —
A new rebased patch.
Attachment #8526201 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
I hate to do this, but it's bitrotted again. :(
Flags: needinfo?(vivekb.balakrishnan)
Keywords: checkin-needed
Vivek, it looks like the patch hasn't changed any of the times you've tried to unbitrot. Please do "hg pull --rebase" to rebase your patch to tip, then fix any merge conflicts (if there are any). Feel free to ping me if you need more help!
Attached patch 1100742.patchSplinter Review
A patch after rebase as per bnicholson's comment.
Hope it works
Attachment #8526818 - Attachment is obsolete: true
Flags: needinfo?(vivekb.balakrishnan)
Attachment #8525420 - Attachment is obsolete: true
tracking-fennec: --- → ?
https://hg.mozilla.org/mozilla-central/rev/64ba905a6180
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment on attachment 8529128 [details] [diff] [review]
1100742.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1087673
[User impact if declined]: The long press back menu vibration is too strong.
[Describe test coverage new/current, TBPL]: Tested locally
[Risks and why]: Very low risk. Simply changes the vibration pattern for the long back press.
[String/UUID change made/needed]: None
Attachment #8529128 - Flags: review+
Attachment #8529128 - Flags: approval-mozilla-aurora?
Attachment #8529128 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: ? → 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.