-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Image map #1807
Image map #1807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a great feature! Thank you, @grzesiek2010. I imagine it's slow on older devices but I think that's ok. That also means it's not so important to support Android < 5 as long as the limitation is clearly documented.
Overall the implementation is simple and clean. Could you please add a brief header comment on each new file that at least identifies what it's for at a high level?
One area of concern is the issue with webviews blocking swipes that you pointed out in #1063. When the image takes up all the vertical space it's possible to be "stuck" on the screen. On Android 7 I get a side scroll bar I can use but on Android 5.1.1 there's nothing I can do. Any ideas on how to make that experience better?
private void addOnClickAttributes(NodeList node) { | ||
for (int i = 0; i < node.getLength(); i++) { | ||
Node path = node.item(i); | ||
if (path.getNodeType() == Node.ELEMENT_NODE && path.getAttributes().getNamedItem("id") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use .equals
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we use short
(primitive type) not Short
(class) so we can't use equals
.
return convertDocumentToString(document); | ||
} | ||
|
||
private void addOnClickAttributes(NodeList node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodes
would be better since it's more than one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
return null; | ||
} | ||
|
||
protected void refreshSelectedItemsLabel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears glitchy for select one on my device -- it flashes when the selection changes. It looks good for select multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
selectedAreasLabel.setText(Html.fromHtml(stringBuilder.toString()))); | ||
} | ||
|
||
protected abstract void adjustWebView(WebView view); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name suggests it's going to change the webview itself in some way. Maybe highlightSelections
would be a more descriptive name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@lognaturel thanks for your review!
It's done.
It's not blocked by default. I did it deliberately because if I try to scroll the map horizontally without that fix it's really sensitive and in many cases, I'm moved to next question. A case with scrolling is always a problem when you have nested scrollViews like here. To solve the issue maybe we should keep a small part of the screen at the bottom - I mean we can, for example, force the webView to take not 100% of available space but 90% then you can swipe to another question using that place. A similar issue is when you want to scroll vertically. Then if you want to scroll the map it works well but if you want to scroll the whole widget you need to use the bar what might be hard in some devices (do you remember the bug with scrolling in Android 6?) |
@lognaturel It doesn't look well and we can remove them and allow zooming only using fingers. |
14bb0c8
to
d85bf86
Compare
That sounds like a really good idea, let's try it.
They're certainly not very attractive. If they're easy to suppress, let's do that. |
605e680
to
06097d4
Compare
06097d4
to
2dd8eb5
Compare
I added a space (right margin) and now we can easier scroll vertically an horizontally.
It's done. |
@grzesiek2010 Extra white space is visible when the image is enlarged. |
The bug with blank space is fixed. I've also added changes to better handle not supported Android 4. Now I display a message and use an ordinary SelectWidget instead. Please continue testing. |
This is the only weakness I can see. The size should be specified in svg file otherwise I use default 1000x1000 what is not perfect because I wasn't able to find a solution how to calculate the size (maybe it's possible but I didn't want to get stuck on it). We can open a separate issue just to investigate it. |
Nice comments! Sounds good about filing a separate issue to investigate scaling. |
@mmarciniak90 Where did you find the body SVG? I was looking for something like it but couldn't find one as nice! |
Hi, thank you for developping this feature, it will be very helpful on the field. It seems that in Android 4.4 Webview does not support Map() and Set() used in collect/collect_app/src/main/assets/svg_map_helper.js By replacing these with array and object the following may enable area selection in older versions of Android: var selectedAreas = [];
var originalColors = {};
var lastSelectedAreaId;
var isSingleSelect;
function selectArea(areaId) {
imageMapInterface.selectArea(areaId);
}
function unselectArea(areaId) {
imageMapInterface.unselectArea(areaId);
}
function notifyChanges() {
imageMapInterface.notifyChanges();
}
function addSelectedArea(selectedAreaId) {
selectedAreas.push[selectedAreaId];
document.getElementById(selectedAreaId).setAttribute('style', 'fill: #E65100');
if (Boolean(isSingleSelect)) {
lastSelectedAreaId = selectedAreaId;
}
}
function addArea(areaId) {
originalColors[areaId] = document.getElementById(areaId).style.color;
}
function setSelectMode(isSingleSelect) {
this.isSingleSelect = isSingleSelect;
}
function clickOnArea(areaId) {
if (selectedAreas.indexOf(areaId) !== -1) {
document.getElementById(areaId).setAttribute('style', 'fill: ' + originalColors[areaId]);
selectedAreas.splice(areaId, 1);
unselectArea(areaId);
} else {
if (Boolean(isSingleSelect) && !!lastSelectedAreaId) {
document.getElementById(lastSelectedAreaId).setAttribute('style', 'fill: ' + originalColors[lastSelectedAreaId]);
selectedAreas.splice(lastSelectedAreaId, 1);
unselectArea(lastSelectedAreaId);
}
document.getElementById(areaId).setAttribute('style', 'fill: #E65100');
selectedAreas.push(areaId);
selectArea(areaId);
lastSelectedAreaId = areaId;
}
notifyChanges();
} |
Thank you for that insight, @ymlambert! Would you like to submit a pull request with the change? It would also be wonderful to learn more about your intended usage of the feature. What kind of data will you collect with it? This context may spark good ideas for adding to it and/or other related functionality. |
@lognaturel I will submit a pull request as soon as I find how to achieve this :) I am a French medical resident specializing in Public Health; I am currently involved in a project aiming to fight malaria in French Guiana. I am not a professional dev, right now my job is to help find or create solutions regarding data collection in this project. We will be using odk collect along with kobotoolbox to collect data on the field. This will be done in several well-identified areas, but participants may come from remote places of the amazon forest that might not always be clearly geographically localized and/or bear different names depending on the language. I first developed a custom application based on odk.counter, “namefinder”, because we need a more complex autocomplete form that uses phonetic matching (similar to soundex or doublemetaphone; I use a home-made algorithm specific to French) and allows free text when no option matches. Right now this behaviour is not possible with odk collect. I guess that autocomplete extends select_one and therefore displays all the possible options below the text field. This is not quite appropriate when many options are available (in our case several hundreds). When the name of the place is unknown (no option matches despite phonetics) or in case of homonyms, we want to go one step further and either document or confirm the area in which this place is located. This could be achieved with geoshape but we are afraid that the map might not always display correctly because we will be working mostly offline. Tapping a predetermined area is more straightforward than manually entering several points, in that aspect image_map seems to be a more interesting solution than geo_shape. Unfortunately the device I am using is running Android 4.4, so I was a bit disappointed (but not discouraged) to learn the image_map feature may not work for me. However, once implemented this solution seemed to work nicely: https://stackoverflow.com/questions/17543389/android-image-map-displaying-an-svg-and-using-it-as-an-image-map-touch-zone I concluded that Android version should not be the issue and then adapted your approach in my custom app to address our needs. Yet I have to admit that Webview rendering is quite slow on my device, as you predicted earlier. If this is of interest, I will publish our custom app on my github account once it is somewhat functional; it will be launched from a form in odk.collect thanks to the “ex:org.odk.namefinder(…)” appearance. I am quite new to GitHub and java so my code might not be ‘state of the art’ or secure enough, but for our project it will do. I hope this help! |
@ymlambert I took a quick look and looks like you are right. Are you going to submit a pull request? I'm' asking because I can do that. |
@grzesiek2010 Please go ahead! |
Thanks for following up @grzesiek2010 and for offering to submit a PR! @ymlambert, thanks for sharing your use case, that is very helpful to know. The update should be out in the Play Store in about a month.
Super! Je suis Québécoise. 😊
That would be fantastic. https://forum.opendatakit.org/c/showcase would be a great place for it. Looking forward to checking it out. |
Closes #1764
What has been done to verify that this works as intended?
I've tested three different forms attached below.
Why is this the best possible solution? Were any other approaches considered?
I've tested lots of different solutions and this is the easiest one since I don't need any additional library and I can use an original svg file.
Are there any risks to merging this code? If so, what are they?
I refactored a few classes to share the code so we need to test all select widgets to avoid regression.
Do we need any specific form for testing your changes? If so, please attach one.
I used three different forms for testing:
canada.zip
poland.zip
shapes.zip
There is one problem, the solution doesn't work on Android <5. The map is displayed but we can't click. I noticed the same issue using another solution http://dmitrybaranovskiy.github.io/raphael/ so there is something wrong with WebViews on Android 4 and I need to investigate it.
Everything looks ok on Android 5, 6, 8