PR for #149 location opens map #270
Conversation
…ltModel to use that.
| private void init() | ||
| { | ||
| mMapOpener = new MapOpener(getContext()); | ||
| mGestureDetector = new GestureDetector(getContext(), new GestureDetector.SimpleOnGestureListener() |
There was a problem hiding this comment.
Can't you just make the text view or the LocationFieldView clickable?
Why the GestureDetector?
There was a problem hiding this comment.
I spent about 2 hours trying to make the clickable thing work and I couldn't. There is of course a chance that I missed something, so let me know if I should check again and spend like 1 hour more with it.
(I tried with clickable, focusable combinations on parent and child views and some other suggestion I found on StackOverflow.)
There was a problem hiding this comment.
The problem is that you inherit from TextFieldView. TextFieldView "linkifies" the TextView which consumes any clicks instead of passing them to the underlying LinearLayout.
I suggest to not subclass TextFieldView. Either create an AbstractTextView that both classes inherit from or just duplicate the methods in TextFieldView. Remember that subclassing non-abstract classes is evil ;-) (yeah, I know that we probably still do that in other places of this code base).
In this particular case I'd actually prefer the code duplication, because we're dealing with a lot of mutable dependencies (the UI) and I'd like to avoid any side effects like this one.
There was a problem hiding this comment.
Ok, thanks for the tip, the culprit was android:textIsSelectable="true" in the style of the TextViews.
Setting that to false allows clicking on the LinearLayout now.
LocationFieldView is no not the child of TextFieldView in the new PR.
|
|
||
| public String getText() | ||
| { | ||
| return mText.getText().toString(); |
There was a problem hiding this comment.
Don't use UI components for business logic. UI is either output or input, you're using it like a variable.
There was a problem hiding this comment.
Ok, I see, that felt a little bit wrong to me too, but on the other hand I thought user would probably want to search for location that is displayed, whatever that is.
But I changed TextFieldView now to save the actual text in a field and use that for the getter. Please check if it's good like that.
| * | ||
| * @author Gabor Keszthelyi | ||
| */ | ||
| public class MapOpener |
There was a problem hiding this comment.
This class doesn't feel right and probably shouldn't exist. I suggest to make this a private method of the LocationFieldView.
There was a problem hiding this comment.
Moved it to private method.
…layed value on from the TextView.
|
@dmfs please check comments above, otherwise it's ready for review again from my side |
|
This just crashed in my emulator: |
|
If there is no app to open the |
|
Yeah, that's a mistake, I should have checked if the intent could be resolved.. fixed it. And it falls back to opening @dmfs Ready for review again. |
|
Please remove the |
…Detector, make LinerLayout onClick work by setting textIsSelectable=false on child views (overriding style).
|
Made the changes and squashed the commits, new PR is #285. |
Linking to #149.
@dmfs please review.
I haven't tested with
XmlModel, only with local tasks andDefaultModel. Should I test withXmlModeltoo, and if yes, could you please advise how can I do that, where to create account, etc?