Fix: Double tapping of TimeTravel button #238
Conversation
| @@ -108,7 +108,7 @@ public GeocentricCoordinates getSearchLocation() { | |||
| ArrayList<TextSource> points = new ArrayList<TextSource>(proto.getLabelCount()); | |||
| for (LabelElementProto element : proto.getLabelList()) { | |||
| points.add(new TextSourceImpl(getCoords(element.getLocation()), | |||
| resources.getString(element.getStringIndex()), | |||
| "TestElement", | |||
There was a problem hiding this comment.
It can be seen that further iterating through the implementation resource variable we find that it is an empty collection declared here. Also errors were popping up when building without this fix. Please correct me if I am wrong.
| @@ -76,7 +76,7 @@ public ProtobufAstronomicalSource(AstronomicalSourceProto proto, Resources resou | |||
| if (names == null) { | |||
| names = new ArrayList<String>(proto.getNameIdsCount()); | |||
| for (int id : proto.getNameIdsList()) { | |||
| names.add(resources.getString(id)); | |||
| names.add("TestElement"); | |||
There was a problem hiding this comment.
It can be seen that further iterating through the implementation resource variable we find that it is an empty collection declared here. Also errors were popping up when building without this fix. Please correct me if I am wrong.
There was a problem hiding this comment.
Without diving into the code again (it's been a while), I believe that this is where the names are extracted from the protocol buffer data that's bundled with the app. If you replace this with a hard-coded string I believe it will remove all astronomical names from the app, so the change should be reverted.
There was a problem hiding this comment.
@barbeau here's the Logcat if the change is reverted
2019-03-21 20:33:33.156 19958-20073/? E/AndroidRuntime: FATAL EXCEPTION: pool-1-thread-1 Process: com.google.android.stardroid, PID: 19958 android.content.res.Resources$NotFoundException: String resource ID #0x7f0d01e1 at android.content.res.Resources.getText(Resources.java:351) at android.content.res.MiuiResources.getText(MiuiResources.java:97) at android.content.res.Resources.getString(Resources.java:445) at com.google.android.stardroid.source.proto.ProtobufAstronomicalSource.getLabels(ProtobufAstronomicalSource.java:112) at com.google.android.stardroid.layers.AbstractSourceLayer.initialize(AbstractSourceLayer.java:74) at com.google.android.stardroid.layers.AbstractFileBasedLayer.access$201(AbstractFileBasedLayer.java:46) at com.google.android.stardroid.layers.AbstractFileBasedLayer$1.run(AbstractFileBasedLayer.java:65) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1162) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:636) at java.lang.Thread.run(Thread.java:764)
What may had happened that Resources would have been removed after the Application was made open-source.
There was a problem hiding this comment.
Have you run the build_skymap.sh script?
https://github.com/sky-map-team/stardroid/tree/master/tools
There was a problem hiding this comment.
@jaydeetay would need to weigh in, but in general it would be great to have build_skymap.sh integrated as part of the primary app Gradle build process, so when loaded into Android Studio everything would magically work. But it would need to be done carefully to ensure that all stages aren't repeated on every debug build, and some only occur on first execution or when the data changes.
There was a problem hiding this comment.
@barbeau Could you please guide me why Travis CI build is failing?
There was a problem hiding this comment.
Looks like the emulator startup isn't working - I'm guessing it's the same long-running problem I've seen here:
- Travis builds are failing again OneBusAway/onebusaway-android#720
- Emulator-based unit tests no longer work on Travis googlemaps/android-maps-utils#371
I haven't revisited this myself in a while, but it looks like there are other projects now running emulator-based tests on Travis - see OneBusAway/onebusaway-android#720 (comment)
There was a problem hiding this comment.
So do we need to change the .travis.yml file to add support for higher API emulators?
There was a problem hiding this comment.
Thanks Utkarsh for your fixes, and Sean for answering. Yes, the current flow is pretty hideous, but it is documented in tools/README. The only reason it hasn't been integrated into gradle is I'd much rather make it go away entirely and revamp the way we store the data. As for the Travis CI build, this has been a frustration for a while.
utkarsh530
force-pushed
the
beta
branch
2 times, most recently
from
8ac029d to
98c6e69
Compare
| @@ -57,6 +58,7 @@ | |||
| // This is the date we will apply to the controller when the user hits go. | |||
| private Calendar calendar = Calendar.getInstance(); | |||
| private AstronomerModel model; | |||
| private long LastClickTime = 0; | |||
There was a problem hiding this comment.
Style nit: java variables usually start with lower case.
| @@ -76,13 +78,17 @@ protected void onCreate(Bundle savedInstanceState) { | |||
| Button changeDateButton = (Button) findViewById(R.id.pickDate); | |||
| changeDateButton.setOnClickListener(new View.OnClickListener() { | |||
| public void onClick(View v) { | |||
| if (SystemClock.elapsedRealtime() - LastClickTime < 1000) return; | |||
| LastClickTime = SystemClock.elapsedRealtime(); | |||
There was a problem hiding this comment.
Someone else has come up with a similar solution. I'm not sure it's the way to go - what's special about 1000ms?
There was a problem hiding this comment.
Actually 1000 ms act like limit on simultaneous clicks. It differentiates two clicks if and only if their time difference is less than 1000.
There was a problem hiding this comment.
Yes, I can see that. I'm just wondering how the number was picked.
|
High level comment: it's generally better to have PRs focused on one particular issue (ie fix the build OR do a bug fix). Otherwise you can get a PR blocked on agreement on one part of it when the other part might be easy to agree on and merge. (Also, if you ever need to roll something back, it's easier to focus on rolling back one thing). Lastly - an apology for my slowness at replying. Sky Map is kinda dormant and only gets attention from me now and again. |
|
@jaydeetay No problem at all! Its really good that you are reviewing our PRs. |
|
@jaydeetay PR is updated. |
utkarsh530
changed the title
|
Thanks! And again, sorry for slow responses. I'm probably only going to be able to do reviews once per week at best. BTW - question for you - there seems to have been a recent uptick in people creating pull requests for Sky Map. I'm very curious - why is this? What prompted you to want to to work on this? (I'll pose the same question to the other recent contributors). |
|
OK, it's good that the 1000ms is now a named constant. However, this seems like a 'band aid' solution rather than a complete fix. I was wondering if you'd considered disabling the buttons on click, and then re-enabling them when the dialog closes. Is that not feasible? |
|
See also: #242 |
|
@jaydeetay So I should proceed with that fix to disable button after clicking? |
|
Ah thanks, that explains it. There was a thread about that here #180 last year that went cold. I don't recall signing up to anything - I wouldn't have had the time to do a decent job unfortunately. I'm a little surprised that Sky Map is on the list for GSoC without my knowledge. Or perhaps I've just forgotten. |
|
Here's the AOSSIE 2019 project: ...and Gitter channel: Looks like it's proposing a hard fork, patterned after an iOS version that was developed. |
|
Thanks Sean. Regarding this PR, I do think it's a bit of a bandaid that we might want to revisit, but I'm happy to try it for now and revisit. |
Fixes #237
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the unit tests with
gradlew app:connectedGmsDebugAndroidTestto make sure you didn't break anything (with an emulator running or phone connected)If you have multiple commits please combine them into one commit by squashing them.