Skip to content
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

Cannot add photo to note #3967

Closed
MikaelBe opened this issue Apr 16, 2022 · 29 comments
Closed

Cannot add photo to note #3967

MikaelBe opened this issue Apr 16, 2022 · 29 comments
Assignees
Labels

Comments

@MikaelBe
Copy link

MikaelBe commented Apr 16, 2022

How to Reproduce

Create new note at my present location.
Click "add photo to the note".
Take photo.
Accept the photo (check mark).
Click OK to save the note.
Result: note gets saved, but:

  • No photo is attached.
  • I cannot see the resulting note in my streetcomplete app.

To be clear:

  • I can see my notes in the iD editor - but without images.
  • I can see other people's notes - with links to their images - in the streetcomplete app.
  • Neither I nor my wife can see my (open) notes in the app.

Versions affected

Streetcomplete v. 42.0
Android v11, Moto One Hyper, model nr XT2027-1
Username: \Mike
And that's 5 characters, yes, there's a backslash in my username/display name. Could this mess something up? I tried to change my display name on openstreetmap.org to something without that special character, but can still not see my own notes. Haven't tried adding a photo since the display name change.

@MikaelBe MikaelBe added the bug label Apr 16, 2022
@MikaelBe
Copy link
Author

Typo.

@peternewman
Copy link
Collaborator

Well you've obviously gone:
https://www.openstreetmap.org/user/%5CMike/notes

Is this now you?
https://www.openstreetmap.org/user/Mike/notes

I'd start by trying to add a new note now you've changed your name?

When you say without images, do you mean they have the link but it points to a 404?

This sounds like potentially two bugs, firstly you can't see notes with your edge case username in SC (is this all, or image only ones)? Secondly your edge case username doesn't correctly attach images to notes.

Does this also happen if you comment on someone else's note with an image?

@matkoniecz
Copy link
Member

Can you link one of affected notes?

@matkoniecz matkoniecz added the feedback required more info is needed, issue will be likely closed if it is not provided label Apr 17, 2022
@MikaelBe
Copy link
Author

MikaelBe commented Apr 17, 2022

I changed my user name to Mike_Be.

One of the notes from yesterday that I apparently haven't closed yet is this: https://www.openstreetmap.org/note/3136708 (old user name; I tried to take a photo when I created this note)

No, there were no links whatsoever.

However: on today's walks, the photo in note did work: see
https://www.openstreetmap.org/note/3138263

If I make a note in SC it never shows up there, and if I comment on another user's note, it disappears from SC for me.
https://www.openstreetmap.org/note/3130098 (was visible in SC until I added the comment, now it's gone from SC for me).
Am I supposed to be able to do see my notes? When reading these discussions here today, I saw someone mentioning in passing something about "since I can't see my own notes in SC, I have to...". I'm not sure if I'll be able to find that comment again, but that made me think that this was a design decision?

So I guess at least one part of the bug could be closed as there's a workaround: don't use the \ character in the user name, at least not as the first character.
That fixed the photo-in-note issue, but there might still be a "can't see own notes" issue where I'm not sure about intended behavior.

@MikaelBe
Copy link
Author

MikaelBe commented Apr 17, 2022

I asked another user if they can see my unresolved notes in the SC app. They can not.

Neither could they see the note where I had commented on their existing note (3138263). Not clear if they would have been able to see their own notes before I commented, of if my comment was what hid it.

@peternewman
Copy link
Collaborator

See Settings>Show all notes, for some background see #2692 and #3002. Checking on OSM proper is a better test.

The photo stuff is probably still independent.

@matkoniecz
Copy link
Member

One of the notes from yesterday that I apparently haven't closed yet is this: https://www.openstreetmap.org/note/3136708 (old user name; I tried to take a photo when I created this note)

What you mean by "tried to take photo"? Have you managed to see photo attached in SC?

I suspect that things are going wromg with your photo app or with handover of photo to SC.

@MikaelBe
Copy link
Author

MikaelBe commented Apr 18, 2022

One of the notes from yesterday that I apparently haven't closed yet is this: https://www.openstreetmap.org/note/3136708 (old user name; I tried to take a photo when I created this note)

What you mean by "tried to take photo"? Have you managed to see photo attached in SC?

I suspect that things are going wromg with your photo app or with handover of photo to SC.

Let's see if I can describe this better:

Case 1:
Display name "\Mike" (no quotation marks)

  • Create new note
  • Enter relevant note text; press the camera button.
  • Take photo.
  • Accept the photo in the preview viewer (checkmark)
  • I'm back to the note interface with no trace of the photo that I attempted to take.
  • Save note
  • In SC, I cannot see the note (but that's apparently by design). In OpenStreetMap (iD editor), I can find the note, but there's no link to any photo.
  • Thus, I consider this case broken.

Case 2:
Display name "Mike_Be" (no quotation marks)

  • Create new note
  • Enter relevant note text; press the camera button.
  • Take photo.
  • Accept thee photo in the preview viewer (checkmark)
  • I'm back to the note interface, and now there's a preview of the photo I just took, just to the right of the camera button.
  • Save note
  • In SC, I cannot see the note (but that's apparently by design). In OpenStreetMap (iD editor), I can find the note, and I see a link to the photo.
  • Thus, I consider this case working.

Therefore, it appears to be the case that the initial \ character broke something in the handling of photos.
Personally, I'm okay with the workaround of using a display name that doesn't involve special characters.

Did I answer your question?

@westnordost
Copy link
Member

The OSM username is not used in any part of the taking-a-photo process. So when you rename yourself to \Mike, can you still reproduce this?

Generally, if there is an error while taking the photo (cannot write to disk etc.), there should be a toast that shows an error message. An error message is only not shown if either

  • the app does not have the camera permission and the user declined to grant it
  • the photo app reported back that no picture was taken

@MikaelBe
Copy link
Author

I can't comment on what the camera app does or does not do. All I can report is what effects I see, and that was that I tried perhaps a dozen times to add a photo to a note when I used the user name "\Mike" and zero of them got saved; then I changed user name and succeeded in perhaps 6 out of 6 attempts, so I did indeed draw a, perhaps premature, connection between that user name and the behavior of the app.

I have now changed my user name back, and have managed to get the app to attach photos to notes in 3 out of 3 attempts.
No, I don't know how to explain that.

Time to close the bug as unreproducible?

@westnordost
Copy link
Member

Yeah, I guess. My bet is on the camera app, but 🤷

@gojhub
Copy link

gojhub commented Apr 20, 2022

I have the same problem with v42.0 (f-droid) on a Sony Xperia XA2.

I recently noticed that attaching photos to notes doesn't work anymore, but the behavior is a little bit other than the on described by MikaelBe:

  1. I press the camera button in the note editor,
  2. the default photo app opens,
  3. I take the photo,
  4. the photo app closes, but no confirmation, no preview viewer
  5. and the photo is not attached.

Generally, if there is an error while taking the photo (cannot write to disk etc.), there should be a toast that shows an error message. An error message is only not shown if either

  • the app does not have the camera permission and the user declined to grant it
  • the photo app reported back that no picture was taken

I didn't get a error toast, and the app has the camera permission (checked it), so your explanation seems that the photo app reported that no photo was taken...

But it worked in the past and afaik I had not changed anything since then.

  • The last note with photo in my history was taken with StreetComplete v40.2.
  • The first note which should have a photo but it wasn't attached was taken with StreetComplete v41.2.

@Cj-Malone
Copy link
Contributor

Did anyones version of Android change?

Just checked on v42.0 from FDroid. Mi A1, Lineage OS 18.1/Android 11.

It worked like usual. Launched the normal camera activity, confirmed it and the thumbnail was at the bottom as expected.

@gojhub
Copy link

gojhub commented Apr 20, 2022

Not mine, it is still an Android 9...

@westnordost westnordost reopened this Apr 20, 2022
@Cj-Malone
Copy link
Contributor

But it worked in the past and afaik I had not changed anything since then.

* The last note with photo in my history was taken with StreetComplete v40.2.

* The first note which should have a photo but it wasn't attached was taken with StreetComplete v41.2.

103b662

@westnordost
Copy link
Member

@gojhub try downgrading to v40.2, can you attach a photo? Then upgrade to v41.2. Can you attach a photo?

@gojhub
Copy link

gojhub commented Apr 20, 2022

@westnordost I tried to downgrade, but the installer told me it can't do it for an unknown reason...

So I decided to do a reboot and before trying again to downgrade, I tested it again with v42...
I think you can guess it, now it works... The photo app closes and below the input box finally the icon with the photo preview appeared as before.

But: As my mobile nags me after 28 days that I really should do a reboot and I obey my mobile ;-), I had done a reboot not so long ago (maybe two weeks ago). Don't know why this reboot now solves the problem.

So, sorry for the noise, can't confirm @MikaelBe problem anymore.
@MikaelBe: Had you tried to reboot your device? Maybe it works for you too?

@westnordost
Copy link
Member

Hm. Another theory:

The app asks for the camera permission before taking a photo. If the user denies it, a rationale is displayed and after acknowledgement by the user, asks again. If the user then denies this again, the app will not ask for permission at all anymore.

(This is enforced by the Android system, to the app it just looks as if the user has denied the permission.)

Now, for v41, the camera permission was added. Maybe for some Android versions, an upgrade to v41 led to a state where the camera permission was not granted and it was set as "do not ask again".

@westnordost
Copy link
Member

Oh nevermind, a message is shown to the user in that case "no camera permission".

Actually, in case a photo is not attached, the only code path where no (error) message is displayed is when the photo app simply does not return a photo.

@gojhub
Copy link

gojhub commented Apr 23, 2022 via email

@westnordost
Copy link
Member

When it does not work - does it fix it if you remove StreetComplete from recents (i.e. cold re-start) the app? Or the other way round: If you remove the camera app from recents?

@gojhub
Copy link

gojhub commented Apr 24, 2022

I did some testing, for which I removed all apps from recents.
In this series I started StreetComplete, did a long click near my location for taking a note, clicked directly on the photo icon, made a photo and checked if it would be attached to the note. Neither if it does or not I canceled the note taking with the back button, hit the square for the recents, swiped StreetComplete away and started over.

In the afternoon I had a long series of around 25 continuous attempts for which no photo was attached.
A few moments ago I had a series of 8 continuous attempts for which a photo had been attached to the note editor, then the ninth try resulted in no photo...
Currently I see no reliable way to provoke either case.

In the meanwhile I got the notification about running out of space on my internal space, so I had a look at /storage/emulated/0/Android/data/de.westnordost.streetcomplete/files/Pictures and found all the pictures of my failed attempts. All with a filename like photo_<13digit code>.jpg, where the digit code seems to be the unix timestamp.
Could it be that the photo app saves the picture but tells StreetComplete that no photo was taken?

@westnordost
Copy link
Member

Interesting... does the photo file contain the photo at all, or is it empty?

@westnordost westnordost reopened this Apr 25, 2022
@peternewman
Copy link
Collaborator

In the meanwhile I got the notification about running out of space on my internal space, so I had a look at /storage/emulated/0/Android/data/de.westnordost.streetcomplete/files/Pictures and found all the pictures of my failed attempts. All with a filename like photo_<13digit code>.jpg, where the digit code seems to be the unix timestamp. Could it be that the photo app saves the picture but tells StreetComplete that no photo was taken?

FWIW I looked at the same place out of curiosity and I had three files in mine, two test ones showing bits of my sofa, and a postbox from in the real world.

Mine were all from a long time ago, but shouldn't these failed photos be cleaned up at some point at least?

@gojhub
Copy link

gojhub commented Apr 25, 2022 via email

@westnordost
Copy link
Member

Thank you for this clue. I think I now know what is happening:

The process of taking a photo is interrupted if the AttachPhotoFragment (the view that lets you add, delete, ... photos) is destroyed. Android destroys views at its own leasure (and recreates them when necessary), usually due to memory restrictions. This also explains why it is reproducible only sometimes.
In detail, the process of taking a photo is within a suspending coroutine which is being cancelled when the fragment's view is being destroyed.

Right now I can't think of a good solution for this, will think about this some more.

@westnordost westnordost removed the feedback required more info is needed, issue will be likely closed if it is not provided label Apr 26, 2022
@westnordost
Copy link
Member

This can be reproduced reliably when enabling "Don't keep activities" in the developer options.

The issue is that the asynchronous "start another activity and return its result" in StreetComplete has been (recently) refactored to work with coroutines so that it can be written like synchronous code. E.g. something like

if (!hasCameraPermission && !requestCameraPermission()) return
val photo = takePhoto()
resizePhoto(photo)
// etc...

...instead of the above code scattered all over the place in a state machine because activities are (potentially) volatile. It is too bad that I didn't think of this case before.

This method is not only done for taking a photo, but also for measuring a width/height, for requesting location to be enabled and for requesting camera permission (for AR). This refactor made the code and logic much easier to read and much shorter, so I am kind of reluctant to revert this (if I even cleanly put that refactor into one commit).

I am considering (finally) using ViewModels (and thus viewModelScope for coroutines) in StreetComplete to circumvent this, if only (at the start) for those places where other activities are started for a result. Using ViewModels would make sense anyway, but the app was created before that library existed and to really consistently use ViewModels everywhere is a huge refactoring undertaking.

So, I don't know, other ideas or volunteers who'd like to try out initial ViewModel support are welcome.

@westnordost westnordost added the help wanted help by contributors is appreciated; might be a good first contribution for first-timers label Apr 26, 2022
@westnordost
Copy link
Member

I noticed that the ViewModels wouldn't solve this at all, because ViewModels have of course no access to the view. But starting an activity for result is done from fragments / activities, which are part of the view.

@Helium314
Copy link
Collaborator

Since the photos exist, SC could just look whether there are some new ones.
Example how this could be done: Helium314@a9c1b31 (rough solution, working only for the first note).
Then the new photo(s) would be processed normally, i.e. resize and create the mini-preview.

@westnordost westnordost self-assigned this Jan 4, 2023
@westnordost westnordost removed the help wanted help by contributors is appreciated; might be a good first contribution for first-timers label Jan 4, 2023
westnordost added a commit that referenced this issue Jan 6, 2023
The reason why this does not (always) work is because coroutines may not survive the app being tabbed out and tabbed in again (e.g. going to location settings). So the flow cannot be handled by coroutines on the lifecycle scope of the activity/fragment (see #3967)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants