Skip to content

preview, edit and delete waypoints (#67, #328) #509

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

Merged
merged 9 commits into from
Feb 18, 2025

Conversation

Kevin-Salazar-itcr
Copy link
Contributor

This PR is a continuation of PRs #39 and #171, which were incomplete.
The following features have been added for waypoint management:

@jamescr
Copy link
Member

jamescr commented Feb 13, 2025

This PR will be paused until #508 in merged.

@jamescr jamescr added this to the 2024.12 --> 2025.02 milestone Feb 13, 2025
@jamescr jamescr requested a review from FR3DD221 February 13, 2025 17:19
@Andyporras
Copy link
Member

Andyporras commented Feb 14, 2025

📢 Bug found on Android 15

@Kevin-Salazar-itcr Hi! I tested the functionality and found an issue on Android 15:

  • Editing and deleting photos only work for images taken with this version.
  • If the waypoint has images taken before installing this version, they cannot be modified or deleted.
  • Notes work fine.

It seems like there's a restriction when handling older images. Could you check this? Let me know if you need more details or testing.

@miltonials miltonials self-requested a review February 14, 2025 15:32
Copy link
Member

@miltonials miltonials left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Kevin-Salazar-itcr ! Thanks for the functionality. Definitively is a nice function to have. I found some errors:

  1. When the user clicks the "Text note" button and then cancels the note, the app fails and the text note is saved.
  2. If a photo in the waypoint list has a vertical orientation, part of its content isn't visible because it is displayed with a horizontal layout.
  3. OSMTracker fails when a photo waypoint is too large and the user tries to preview it.
  4. When the waypoint is a photo and the phone is in landscape orientation, the update, delete, and cancel buttons are not shown, and the text input is not displayed properly.

Cell phones already have an image viewer and an audio player. What do you think about delegating the "Play audio" and "View photo" actions to them? Graphically, it would be similar: when the user clicks "Play audio," the device's audio player opens; similarly, for photos, we could show a "View photo" button instead of displaying the photo directly. What do you think, @Kevin-Salazar-itcr ?

Benefits:

  • Lower resources consumption.
  • Photos with zoom capability.
  • Audio slider to move forward or backward in the voice note.
  • Address observations 2, 3 and 4

@Kevin-Salazar-itcr
Copy link
Contributor Author

  • If the waypoint has images taken before installing this version, they cannot be modified or deleted.

Hi @Andyporras, this new version uses an unique ID to handle how works these new functionalities, so, if you have old tracks with waypoints, these won't handle the update.

Example:
image
Waypoints which uses media files are named every time as "Photo" or "Voice recording", and using the file name to show there (and handle them as ID), could be an UX problem, because it's too long... that's why code in #171 uses an uuid to handle the waypoints (the newers).

@FR3DD221
Copy link
Contributor

Hi @Kevin-Salazar-itcr, another issue found: when the text is too large, the cancel button is displayed incorrectly.
This is how it looks (API 35):

image

(API 25):

image

> Usability performed for landscape mode and zoomed fonts
> Preview using external apps
> Fixed: canceling text note creation caused an app crash
@Andyporras
Copy link
Member

I just tested the latest changes. Everything works as expected in Android 15!

Copy link
Member

@miltonials miltonials left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Kevin-Salazar-itcr! I commented some improvements in the code. Aditionally can you please match the color of the modal with the window and move the update, delete and cancel bottons a little bit up. See the screenshoot:

image

@Kevin-Salazar-itcr
Copy link
Contributor Author

Hi @FR3DD221, @Andyporras, @miltonials thanks for your comments, during yesterday i've resolved these bugs. If you have any suggestion feel free to comment here!

Copy link
Member

@miltonials miltonials left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kevin-Salazar-itcr thanks for the changes! Is working well with API25. Just fix the typo. 😉

Copy link
Member

@Andyporras Andyporras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good job. Is working well with API35 ✅

Copy link
Contributor

@FR3DD221 FR3DD221 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I´m just test the last commit and didn´t see any issue.

@jamescr jamescr merged commit 56a982d into labexp:develop Feb 18, 2025
1 of 2 checks passed
@FR3DD221 FR3DD221 mentioned this pull request Feb 24, 2025
@miltonials miltonials mentioned this pull request Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants