Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1003893003:
Basic Android UI for host-offline-reason. (Closed)
|
Created:
5 years, 9 months ago by Łukasz Anforowicz Modified:
5 years, 9 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBasic Android UI for host-offline-reason.
BUG=247245
TEST=Manually verify UI changes in an emulator.
Committed: https://crrev.com/169cd7a1c2b202035401edcbb291a61d891ca4eb
Cr-Commit-Position: refs/heads/master@{#321136}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Addressed most feedback from Sergey and Lambros. #Patch Set 3 : Rebasing... #
Total comments: 2
Patch Set 4 : Fixed placeholder format. #
Messages
Total messages: 16 (3 generated)
lukasza@chromium.org changed reviewers: + lambroslambrou@chromium.org
Lambros, could you please take a look? I've tested in an emulator with a test account that had one of the host shut down because of an invalid policy. I can help set that up or share a screenshot if this helps. I am still working on adding test coverage for the new UI, but this is going slowly because of the need to add Espresso dependency. I think we can already start discussing and/or land the product changes while I keep working on the tests. Thanks, Lukasz
https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:42: // Locale.US seems to be the closest equivalent of C#'s InvariantCulture. Just using Locale.US is fine, I don't think you need to explain it. You're using it on a fixed set of strings where it is known to work. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: String fieldName = "offline_reason_" + hostOfflineReason.toLowerCase(Locale.US); I'd prefer to avoid using reflection in Android code if possible. It might work today, on your local debug build, but it might fail in future with an official release build. For example, this article seems to indicate that ProGuard requires configuration when the app uses reflection: http://stackoverflow.com/questions/4447145/proguard-and-reflection-in-android In this case, getResources().getIdentifier(...) might be a better alternative. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:44: Field resourceIdField = R.string.class.getField(fieldName); Where do we guarantee that the strings passed to this function will match the resource IDs defined in remoting_strings.grd ? Does the webapp use a similar mechanism?
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:42: // Locale.US seems to be the closest equivalent of C#'s InvariantCulture. On 2015/03/13 18:54:37, Lambros wrote: > Just using Locale.US is fine, I don't think you need to explain it. You're using > it on a fixed set of strings where it is known to work. +1. C# is not relevant in this context. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: String fieldName = "offline_reason_" + hostOfflineReason.toLowerCase(Locale.US); nit: It probably doesn't make any difference in this context, but I'd prefer using Locale.ENGLISH instead of Locale.US https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:79: Toast.makeText(mChromoting, tooltip, Toast.LENGTH_SHORT).show(); Instead of showing the offline reason only when user clicks on the host would it be better to show it directly in the list, under the host name?
https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:79: Toast.makeText(mChromoting, tooltip, Toast.LENGTH_SHORT).show(); On 2015/03/13 19:06:26, Sergey Ulanov wrote: > Instead of showing the offline reason only when user clicks on the host would it > be better to show it directly in the list, under the host name? Also, in the webapp we show last online date for the host. It would be great if we could do that on Android too
Thank you for the feedback. Can you please take a look at patchset #2? https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:42: // Locale.US seems to be the closest equivalent of C#'s InvariantCulture. On 2015/03/13 19:06:26, Sergey Ulanov wrote: > On 2015/03/13 18:54:37, Lambros wrote: > > Just using Locale.US is fine, I don't think you need to explain it. You're > using > > it on a fixed set of strings where it is known to work. > > +1. C# is not relevant in this context. Done. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: String fieldName = "offline_reason_" + hostOfflineReason.toLowerCase(Locale.US); On 2015/03/13 18:54:37, Lambros wrote: > I'd prefer to avoid using reflection in Android code if possible. > It might work today, on your local debug build, but it might fail in future with > an official release build. For example, this article seems to indicate that > ProGuard requires configuration when the app uses reflection: > http://stackoverflow.com/questions/4447145/proguard-and-reflection-in-android > In this case, > getResources().getIdentifier(...) > might be a better alternative. > Thanks for pointing this out. I should have thought twice before using reflection (I looked at getString methods, didn't find one that would take a resource name and then went down the reflection route, rather than looking more carefully for the getIdentifier option). Fixed. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:43: String fieldName = "offline_reason_" + hostOfflineReason.toLowerCase(Locale.US); On 2015/03/13 19:06:26, Sergey Ulanov wrote: > nit: It probably doesn't make any difference in this context, but I'd prefer > using Locale.ENGLISH instead of Locale.US Ok. OTOH, we wouldn't have to think twice about this if Java had Invariant culture... :-( https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:44: Field resourceIdField = R.string.class.getField(fieldName); On 2015/03/13 18:54:37, Lambros wrote: > Where do we guarantee that the strings passed to this function will match the > resource IDs defined in remoting_strings.grd ? Does the webapp use a similar > mechanism? There is no guarantee that strings from the host will match resource names in the webapp and android :-/ I am not sure how we could ensure these are kept in sync - custom build script? Yes - the webapp uses a similar mechanism - please see crrev.com/882463003 for details. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:79: Toast.makeText(mChromoting, tooltip, Toast.LENGTH_SHORT).show(); On 2015/03/13 19:08:02, Sergey Ulanov wrote: > On 2015/03/13 19:06:26, Sergey Ulanov wrote: > > Instead of showing the offline reason only when user clicks on the host would > it > > be better to show it directly in the list, under the host name? > > Also, in the webapp we show last online date for the host. It would be great if > we could do that on Android too Good point. Let's do this in a separate changelist - I opened crbug.com/467142 to track this. https://codereview.chromium.org/1003893003/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:79: Toast.makeText(mChromoting, tooltip, Toast.LENGTH_SHORT).show(); On 2015/03/13 19:06:26, Sergey Ulanov wrote: > Instead of showing the offline reason only when user clicks on the host would it > be better to show it directly in the list, under the host name? I opened crbug.com/467140 to track this (you could argue that I should fix this in the current changelist under review, but I want to track this separately given that 1) the same issue is present in the webapp and 2) there are a few details to settle on first (i.e. is it ok to just add more text after the host name VS need to add text *below* the host name VS need to make the details hidden and show them only when used clicks "more details" or a "down arrow" button).
https://codereview.chromium.org/1003893003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:34: resourceName, "string", mChromoting.getPackageName()); This won't work on an official build, I don't think. You'll need to use "org.chromium.chromoting" here, because getPackageName() will return "com.google.chromeremotedesktop" in an Official build. I know, it's really bad, but getPackageName() returns the Play Store identifier for our official app, which is not an actual Java package used in Chromium source code. I might be mistaken, though. Please do an Official build and check it still works.
tl;dr - getPackageName works fine, but "$1" as placeholder doesn't. https://codereview.chromium.org/1003893003/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java (right): https://codereview.chromium.org/1003893003/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/HostListAdapter.java:34: resourceName, "string", mChromoting.getPackageName()); On 2015/03/13 21:37:11, Lambros wrote: > This won't work on an official build, I don't think. You'll need to use > "org.chromium.chromoting" here, because getPackageName() will return > "com.google.chromeremotedesktop" in an Official build. > > I know, it's really bad, but getPackageName() returns the Play Store identifier > for our official app, which is not an actual Java package used in Chromium > source code. > > I might be mistaken, though. Please do an Official build and check it still > works. I did... GYP_DEFINES="...goma... OS=android target_arch=ia32 branding=Chrome buildtype=Official" gclient sync ninja -C out/Debug remoting_apk -j 100 ...and tested out/Debug/apks/ChromeRemoteDesktop.apk. Resource strings for host-offline-reason got resolved correctly. In the apk built above, I see (Log.i) that getPackageName returns "com.google.chromeremotedesktop". In the apk built above, passing "org.chromium.chromoting" to getIdentifier fails to find the resource. So - we need to keep getPackageName. At the same time, testing with "org.chromium.chromoting" made me discover an error with how the placeholders are filled out - right now the final string reads "Unrecognized host error: $1" (i.e. the placeholder got messed up). I'll put this changelist on hold, until I figure out how to fix this (I asked for recommendations in a post here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/7S_tJvL7lTY).
Lambros - could you please take another look? The main change since your last review is in the way the placeholders are formatted in the grd file.
lgtm The string duplication is unfortunate - let's hope we don't do this too often :) The alternative is to manually replace the placeholder in Java. I don't know which is least bad.
The CQ bit was checked by lukasza@chromium.org
On 2015/03/17 23:32:00, Lambros wrote: > lgtm > > The string duplication is unfortunate - let's hope we don't do this too often :) Yes. OTOH, I can think of at least one other case (host was last seen online $1[timestamp]) :-/ > The alternative is to manually replace the placeholder in Java. I'll probably try writing it this way one day and see how it looks. On one hand a simple String.replace would work, but I feel that to make this approach appropriate for the long term, it would need to: 1. do some kind of validation whether the number of placeholders is the same as the number of strings being replaced 2. be in Chromium's base Java libraries > I don't know which is least bad. Yes - I feel the same :-) Still - I also feel that the current change is a very simple approach that gets the job done - let's land it (unblocking unit tests work) and then see if we can improve the situation. FWIW, I chose string duplication (vs custom placeholder replacement in Java) based on the following: Cons of string duplication in the grd file (vs custom placeholder replacement in Java): - Strings in WebApp and Android can unintentionally get out of sync Pros of string duplication in the grd file (vs custom placeholder replacement in Java): - By using standard Java/Android resource APIs, we might potentially get benefit of standard lint/analysis tools - By using standard Java/Android resource APIs, we get code that is easily readable by Java / Android devs - Less custom code = less maintenance burden and less custom bugs
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1003893003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/169cd7a1c2b202035401edcbb291a61d891ca4eb Cr-Commit-Position: refs/heads/master@{#321136} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
