Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175353003:
[Remoting Android] Define feedback animation size in dp (Closed)
Description[Remoting Android] Define feedback animation size in dp
Currently the radius of the feedback animation circle is defined in pixels,
which is DPI dependent and the size will vary on different devices. This CL
solves this problem by defining the size in dp (approximately matches the
current size on Nexus 6P) instead of pixel.
BUG=629686
Committed: https://crrev.com/b2a0c8eb47e750fadd12b2bc885474bf0aac4855
Cr-Commit-Position: refs/heads/master@{#407883}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Reviewer's Feedback #
Total comments: 3
Patch Set 3 : Set up radius inside AbstractDesktopView #
Total comments: 9
Patch Set 4 : Reviewer's Feedback #
Total comments: 6
Patch Set 5 : Move case NONE into the switch #Patch Set 6 : Reviewer's Feedback #
Messages
Total messages: 26 (9 generated)
yuweih@chromium.org changed reviewers: + joedow@chromium.org, lambroslambrou@chromium.org
PTAL :) CC'ed sergeyu@
https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/res/v... File remoting/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/res/v... remoting/android/java/res/values/dimens.xml:9: <dimen name="feedback_animation_radius_large">45dp</dimen> How did you choose 15/45? It might be good to add a comment or mention what these are based on in the CL description. https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:54: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); Can you cache these values? I'd assume they don't change within a session. https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:37: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); Why 2 * size? Same comment about caching as the other animator plus add a comment as to why the multiplier here.
https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/res/v... File remoting/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/res/v... remoting/android/java/res/values/dimens.xml:9: <dimen name="feedback_animation_radius_large">45dp</dimen> On 2016/07/25 20:24:18, joedow wrote: > How did you choose 15/45? It might be good to add a comment or mention what > these are based on in the CL description. This number will approximately match the current feedback size on my Nexus 6P (13/48). Just feel like the small radius can be slightly larger and the large radius can be slightly smaller so I just round them to the nearest 5... https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:54: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); On 2016/07/25 20:24:18, joedow wrote: > Can you cache these values? I'd assume they don't change within a session. Sure. It probably won't change through the whole lifetime of the app... https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:37: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); On 2016/07/25 20:24:18, joedow wrote: > Why 2 * size? Same comment about caching as the other animator plus add a > comment as to why the multiplier here. That's because the OpenGL implementation takes diameter (since it is also the width and height of the texture) as argument while what we set is the radius. I'll add a comment about this.
Description was changed from ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp instead of pixel. BUG=629686 ========== to ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp (approximately matches the current size on Nexus 6P) instead of pixel. BUG=629686 ==========
https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:54: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); On 2016/07/25 20:24:18, joedow wrote: > Can you cache these values? I'd assume they don't change within a session. Done. https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:37: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); On 2016/07/25 20:24:18, joedow wrote: > Why 2 * size? Same comment about caching as the other animator plus add a > comment as to why the multiplier here. Done. Added comment.
https://codereview.chromium.org/2175353003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:53: View view) { I think it's more idiomatic to pass a Context rather than a View ? https://codereview.chromium.org/2175353003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:58: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); The dp/pixel scaling factor is dependent on the Context which is tied to the Activity lifecycle, I think. So you need some way of invalidating the cache if the Activity is destroyed/re-created (for example, if a new display is hot-plugged?) If you keep a cache of the values, it's probably better to store the cache inside an Activity or an Activity-owned object. A static member is probably not the right thing? Also, if the cache is stored in an Activity, you could simply initialize the values in onCreate() so you wouldn't need the '< 0' logic.
lgtm https://codereview.chromium.org/2175353003/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:58: .getDimensionPixelSize(R.dimen.feedback_animation_radius_small); On 2016/07/25 22:14:32, Lambros wrote: > The dp/pixel scaling factor is dependent on the Context which is tied to the > Activity lifecycle, I think. So you need some way of invalidating the cache if > the Activity is destroyed/re-created (for example, if a new display is > hot-plugged?) > > If you keep a cache of the values, it's probably better to store the cache > inside an Activity or an Activity-owned object. A static member is probably not > the right thing? > > Also, if the cache is stored in an Activity, you could simply initialize the > values in onCreate() so you wouldn't need the '< 0' logic. Actually, I'm not sure about this. The scaling factor (from Android's DisplayMetrics class) might be global and not hot-pluggable. It might be different for different displays, if you can have more than one. I'm not sure.
lgtm
As discussed offline with Lambros, it is not a good idea to store the values in static variable since the DPI may change in run time... Since the activity and the desktop view being used will be recreated every time the DPI is changed, I think it is a good idea to retrieve these radius in the ctor of AbstractDesktopView. Hi Lambros, would you double check with this patch since it is mostly a rewrite. And hi Joe, do you have any other comments for this? Thanks! https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (left): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:228: if (feedbackToShow != InputFeedbackType.NONE) { Removed this check since NONE is not been used anywhere else...
lgtm https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:145: public abstract void showInputFeedback(float feedbackRadius, Point pos); It's better to keep the original abstract interface. This new interface forces implementations to render large or small feedback in the same way, as an object with a pixel size. If in future we wanted to render "large" feedback differently (such as flashing the screen or vibrating the buzzer), this would be impossible with this new interface. However, the FeedbackAnimator does somehow need to get the size in pixels, and it needs a View for that (or Context or whatever). So I would suggest adding a public method to DesktopView (or AbstractDesktopView): float getAnimationSizeInPixels(InputFeedbackType feedback) { .. } and have FeedbackAnimator call that (which it can, because you pass the DesktopView in as a parameter). I don't feel too strongly, though. Fine to land this as-is.
lgtm https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (left): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:228: if (feedbackToShow != InputFeedbackType.NONE) { On 2016/07/25 23:43:12, Yuwei wrote: > Removed this check since NONE is not been used anywhere else... It is set in a few places. The check was useful a few refactorings ago, I'd make sure it is not not needed for all input types and scenarios (IIRC the old animator had a problem if it was called with a zero radius which is why this was important). https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:40: if (feedbackRadius <= 0) { it might be clearer to compare with 0.0f here. https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:34: // to be multiplied by 2. You could remove the comment and just add a var: float feedbackDiameter = 2 * feedbackRadius; The var should be optimized away and the code is self-documenting.
https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:145: public abstract void showInputFeedback(float feedbackRadius, Point pos); On 2016/07/26 01:53:58, Lambros wrote: > It's better to keep the original abstract interface. This new interface forces > implementations to render large or small feedback in the same way, as an object > with a pixel size. If in future we wanted to render "large" feedback differently > (such as flashing the screen or vibrating the buzzer), this would be impossible > with this new interface. I'm not sure whether we want to have different feedback implementation > However, the FeedbackAnimator does somehow need to get the size in pixels, and > it needs a View for that (or Context or whatever). So I would suggest adding a > public method to DesktopView (or AbstractDesktopView): > float getAnimationSizeInPixels(InputFeedbackType feedback) { .. } > and have FeedbackAnimator call that (which it can, because you pass the > DesktopView in as a parameter). > > I don't feel too strongly, though. Fine to land this as-is. Done. Leaved a function in the abstract class for getting the radius. https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (left): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:228: if (feedbackToShow != InputFeedbackType.NONE) { On 2016/07/26 03:03:24, joedow wrote: > On 2016/07/25 23:43:12, Yuwei wrote: > > Removed this check since NONE is not been used anywhere else... > > It is set in a few places. The check was useful a few refactorings ago, I'd > make sure it is not not needed for all input types and scenarios (IIRC the old > animator had a problem if it was called with a zero radius which is why this was > important). You are right. Looks like it is set in several places. We probably still need to check this value unless it is actually removed from the code... Just added back the check. https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/FeedbackAnimator.java:40: if (feedbackRadius <= 0) { On 2016/07/26 03:03:24, joedow wrote: > it might be clearer to compare with 0.0f here. Done. https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:34: // to be multiplied by 2. On 2016/07/26 03:03:24, joedow wrote: > You could remove the comment and just add a var: > float feedbackDiameter = 2 * feedbackRadius; > > The var should be optimized away and the code is self-documenting. Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2175353003/#ps80001 (title: "Move case NONE into the switch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:123: /** Returns the radius of the given feedback type. 0.0f will be returned if no feedback should Format this as multi-line: /** * Returns... * blah... */ https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:126: if (feedbackToShow == InputFeedbackType.NONE) { Move this into the switch..case. https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:38: mDisplay.showCursorInputFeedback(pos.x, pos.y, 2 * diameter / scaleFactor); Not 2.0f ? Personally, I prefer just 0 and 2 :)
The CQ bit was unchecked by yuweih@chromium.org
https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:123: /** Returns the radius of the given feedback type. 0.0f will be returned if no feedback should On 2016/07/26 18:54:17, Lambros wrote: > Format this as multi-line: > /** > * Returns... > * blah... > */ Done. https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/AbstractDesktopView.java:126: if (feedbackToShow == InputFeedbackType.NONE) { On 2016/07/26 18:54:17, Lambros wrote: > Move this into the switch..case. Done. https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2175353003/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:38: mDisplay.showCursorInputFeedback(pos.x, pos.y, 2 * diameter / scaleFactor); On 2016/07/26 18:54:17, Lambros wrote: > Not 2.0f ? > > Personally, I prefer just 0 and 2 :) Oops... I think I just found a bug here for multiplying 2 by twice... Yeah.. I think the compiler will optimize away unnecessary type conversions... For being consistent with the diameter <= 0.0f check, I will change it to 2.0...
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/2175353003/#ps100001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp (approximately matches the current size on Nexus 6P) instead of pixel. BUG=629686 ========== to ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp (approximately matches the current size on Nexus 6P) instead of pixel. BUG=629686 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp (approximately matches the current size on Nexus 6P) instead of pixel. BUG=629686 ========== to ========== [Remoting Android] Define feedback animation size in dp Currently the radius of the feedback animation circle is defined in pixels, which is DPI dependent and the size will vary on different devices. This CL solves this problem by defining the size in dp (approximately matches the current size on Nexus 6P) instead of pixel. BUG=629686 Committed: https://crrev.com/b2a0c8eb47e750fadd12b2bc885474bf0aac4855 Cr-Commit-Position: refs/heads/master@{#407883} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b2a0c8eb47e750fadd12b2bc885474bf0aac4855 Cr-Commit-Position: refs/heads/master@{#407883} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
