Photos from photo point and section does not appear at "Info topo" photo list.

rsevero commented 6 months ago

Describe the bug Photos from photo point and section does not appear at "Info topo" photo list.

To Reproduce Steps to reproduce the behavior:

Take a photo for a photo point or for a section
Go to "Info topo" screen
Click on "Photo list" button
See that the photo does not appear on the list

Expected behavior All photos related to the survey should appear at the photo list.

Device:

Samsung Galaxy 23FE
Android 14
TopoDroid 6.2.72

rsevero commented 6 months ago

Fixed with commit cc823ad51

Commentary: I believe photoID in 'photos' table should be unique on the whole table and not only among photos of the same survey. Things seams to get unnecessarily complicated as it is.

Is there an important advantage having them unique only among the photos of the same survey and not among all photos on the table?

marcocorvi commented 6 months ago

there is a problem. from the photo list dialog the user can open the photo dialog and delete the photo. for photos in sketches the related points has to be deleted, and this requires editing the tdr file.

rsevero commented 6 months ago

I see.

I am sure it is not desirable but how bad is it for a photo point to not have it's photo?

To solve this issue I see three paths:

to make the photo dialog not show the delete button for non shot photos when openned through the photo list;
have some cleanup process when openning the sketch to delete the photo point;
leave as it is, so eventually a photo point would not have it 's photo because the use deleted it on the photo dialog openned through the photo list.

Does any of this ideas make sense to you? Do you ahve any ohter idea?

It would be really hard for me to implement 2, I might be able to implement 1. And 3 :)

rsevero commented 6 months ago

Oh, and there is the 4th option:

To make the photo dialog delete the photo point in the sketch but that sounds really complicated.

rsevero commented 6 months ago

Apparently there are is code implementing option 1 but it is not working:

  if ( mAtShot ) {
      buttonDelete.setOnClickListener( this );
    } else {
      buttonDelete.setVisibility( View.GONE );
    }

That's in PhotoEditdialog > onCreate

marcocorvi commented 6 months ago

(3) is not acceptable: user will have a photo point without photo.

(2) can have problems, as well, because of the dangling photo point left around until the sketch is opened.

(1) would be a less than optimal temporary solution: as a user why should i be able to delete some photo and not others?

to start the list of photos should say whether a photo is attached to a shot or a sketch.

the delete button should be left in place. deleting a sketch photo must edit the tdr file and remove the photo/picture point.

in the meanwhile another problem arose: if a shot is moved to a different survey the photo should be moved as well. same for moving a portion of a sketch.

On Wed, Jun 5, 2024, 19:20 Rodrigo Severo @.***> wrote:

I see.

I am sure it is not desirable but how bad is it for a photo point to not have it's photo?

To solve this issue I see three paths:

to make the photo dialog not show the delete button for non shot photos when openned through the photo list;

have some cleanup process when openning the sketch to delete the photo point;

leave as it is, so eventually a photo point would not have it 's photo because the use deleted it on the photo dialog openned through the photo list.

Does any of this ideas make sense to you? Do you ahve any ohter idea?

It would be really hard for me to implement 2, I might be able to implement 1. And 3 :)

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2150575065, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC33B52JJ66NOS2XJP3ZF5CEXAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQGU3TKMBWGU . You are receiving this because you commented.Message ID: @.***>

rsevero commented 6 months ago

The feature that was already present to disable the "delete" button for non shot photos in PhotoEditDialog is working again with commit c6666d427

rsevero commented 6 months ago

(1) would be a less than optimal temporary solution: as a user why should i be able to delete some photo and not others?

For now I made this feature work again.

to start the list of photos should say whether a photo is attached to a shot or a sketch.

In reality the user can see that: if the photo has no station info and has "null-null", that a non shot photo.

the delete button should be left in place.

I can disable the fix I just commited. Please let me know what you prefer.

deleting a sketch photo must edit the tdr file and remove the photo/picture point.

I really don't know where to start implementing this.

rsevero commented 6 months ago

I can put something better than "null-null" in the non shot photos. Maybe "_-_".

And documenting it on the help page.

marcocorvi commented 6 months ago

On Wed, Jun 5, 2024, 19:48 Rodrigo Severo @.***> wrote:

(1) would be a less than optimal temporary solution: as a user why should i be able to delete some photo and not others?

For now I made this feature work again.

to start the list of photos should say whether a photo is attached to a shot or a sketch.

In reality the user can see that: if the photo has no station info and has "null-null", that a non shot photo.

the delete button should be left in place.

I can disable the fix I just commited. Please let me know what you prefer.

deleting a sketch photo must edit the tdr file and remove the photo/picture point.

I really don't know where to start implementing this.

the photos record could have a reference to the plot, to avoid reading all the tdr. then it's just reading the tdr and writing it to a temporary file, skipping the photo point (see drawingio). finally move the temporary onto the tdr.

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2150623029, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYCY7DL4ZMEMUJF3A7MLZF5FOLAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQGYZDGMBSHE . You are receiving this because you commented.Message ID: @.***>

marcocorvi commented 6 months ago

or you can put a more informative string, like the name of the sketch, maybe enclosed in brackets.

On Wed, Jun 5, 2024, 19:50 Rodrigo Severo @.***> wrote:

I can put something better than "null-null" in the non shot photos. Maybe " -".

And documentaing it on the help page.

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2150626315, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYCZGOVU7XJLO4F5M7M3ZF5FWLAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJQGYZDMMZRGU . You are receiving this because you commented.Message ID: @.***>

rsevero commented 6 months ago

or you can put a more informative string, like the name of the sketch, maybe enclosed in brackets.

To do that I will need to created a new column in the "photos" table first to hold this info, AFAICT.

I have to leave home now. I will take a look at it tomorow, ok?

rsevero commented 6 months ago

Fixed with d4019686e

marcocorvi commented 6 months ago

i think it is better to use the item ID (unfortunately named shotid) to refer to the parent of the photo, and add a flag field to distinguish the item type (ie, table). this leaves open the possibility to have sketch photos, in the future, if topodroid will ever get sketches.

the item ID should be used also for audio media.

On Fri, Jun 7, 2024 at 4:15 AM Rodrigo Severo @.***> wrote:

Fixed with d401968 https://github.com/marcocorvi/topodroid/commit/d4019686e3a4cfdbe60e0bfe6aaa9bb6ced11a60

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2153730555, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC73JVLM7QLVHCGHF63ZGEJVXAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJTG4ZTANJVGU . You are receiving this because you commented.Message ID: @.***>

rsevero commented 6 months ago

To implement this itemId/itemType structure queries like:

private static final String qjShotPhoto   =
    "select p.id, s.id, p.title, s.fStation, s.tStation, p.date, p.comment, p.camera, p.code from photos as p join shots as s on p.shotId=s.id where p.surveyId=? AND s.surveyId=? AND p.shotId=? ";

will have one left join per item type.

Are you ok with that?

rsevero commented 6 months ago

Right now TopoDroid has photo of the following types:

shot
point
section

Is that it? Is that all?

rsevero commented 6 months ago

will have one left join per item type.

Or maybe use UNIONs.

marcocorvi commented 6 months ago

i was looking at the issue. it is a lot more complicated than i thought.

it should concern all media, not only photos.

i had something that compiles, but the delete of the point from the sketch is a work in progress.

i dont remember how x-section photo are handled. x-sections can have both photo and sketch. therefore deleting a x-section photo should not delete the x-section.

On Fri, Jun 7, 2024, 13:44 Rodrigo Severo @.***> wrote:

Right now TopoDroid has photo of the following types:

shot

point

section

Is that it? Is that all?

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2154667682, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC6ZAFITYI2VYI4O6F3ZGGMJJAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJUGY3DONRYGI . You are receiving this because you commented.Message ID: @.***>

rsevero commented 6 months ago

i was looking at the issue. it is a lot more complicated than i thought. it should concern all media, not only photos.

I saw this too when I started renaming "shotId" to "itemId".

i had something that compiles, but the delete of the point from the sketch is a work in progress. i dont remember how x-section photo are handled. x-sections can have both photo and sketch. therefore deleting a x-section photo should not delete the x-section.

I will let it to you by now as 2 people messing on the same code at the same time is a great recipe for trouble.

Please let me know if I can help in any way.

marcocorvi commented 5 months ago

the work is more complex than i thought at first.

before fiddling with the code i would like to think about what to do, and how to do that. i would like to start discussing the issues. then i can code. i appreciate if you can revise what i do. it will take a few days. maybe a week or more.

my idea is:

to create a MediaInfo class parent of Audio, Photo, and Sensor
the media tables have a "shotid" field. I cannot rename it, but it should be considered as "reference item ID".
to extend media tables with a field for the reference item type
then there are plenty of changes to do in the sources,
in particular the audio has a negative ID when it is a plot audio and a positive ID when it is a shot audio. this was a bad choice (made to save one field). it is not really a problem because audio point is a tester feature, and it is in the extra symbols.
then we should add a method to the DrawingIO class that drop a media point from the tdr file. this should be executed in background, possibly with a protection to avoid opening the drawing while the file is edited (it could be also a global protection that prevent opening any drawing).
not necessary but related: the DrawingWindow dialog of shots chould have the photo and audio like the ShotWindow dialog

anything else ?

On Fri, Jun 7, 2024 at 3:51 PM Rodrigo Severo @.***> wrote:

i was looking at the issue. it is a lot more complicated than i thought. it should concern all media, not only photos.

I saw this too when I started renaming "shotId" to "itemId".

i had something that compiles, but the delete of the point from the sketch is a work in progress. i dont remember how x-section photo are handled. x-sections can have both photo and sketch. therefore deleting a x-section photo should not delete the x-section.

I will let it to you by now as 2 people messing on the same code at the same time is a great recipe for trouble.

Please let me know if I can help in any way.

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2154891084, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYCYX6RLIHBNHLYC46UTZGG3F7AVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJUHA4TCMBYGQ . You are receiving this because you commented.Message ID: @.***>

rsevero commented 5 months ago

the work is more complex than i thought at first. before fiddling with the code i would like to think about what to do, and how to do that. i would like to start discussing the issues. then i can code.

Better way, for sure.

my idea is:

to create a MediaInfo class parent of Audio, Photo, and Sensor

Yes.

the media tables have a "shotid" field. I cannot rename it, but it should be considered as "reference item ID".

Why not? On my second try coding a solution for this issue, I started renaming shotId to itemId. It didn't seem particulary hard to do. There were, AFAIC remember, less than 20 places to change.

I can do this part for you and commit it. I would change nothing else. May be refItemId?

to extend media tables with a field for the reference item type

then there are plenty of changes to do in the sources,

Sure.

in particular the audio has a negative ID when it is a plot audio and a positive ID when it is a shot audio. this was a bad choice (made to save one field). it is not really a problem because audio point is a tester feature, and it is in the extra symbols.

We can solve this issue later. When MediaInfo is working.

then we should add a method to the DrawingIO class that drop a media point from the tdr file. this should be executed in background, possibly with a protection to avoid opening the drawing while the file is edited (it could be also a global protection that prevent opening any drawing).

If possible, blocking only the referred file.

not necessary but related: the DrawingWindow dialog of shots chould have the photo and audio like the ShotWindow dialog

Nice touch.

anything else ?

How photo files are exported? Are them included in zip files? Any other export format has the photos? How to export only the photos?

marcocorvi commented 5 months ago

On Sat, Jun 8, 2024, 17:08 Rodrigo Severo @.***> wrote:

the work is more complex than i thought at first. before fiddling with the code i would like to think about what to do, and how to do that. i would like to start discussing the issues. then i can code.

Better way, for sure.

my idea is:

to create a MediaInfo class parent of Audio, Photo, and Sensor

Yes.

the media tables have a "shotid" field. I cannot rename it, but it should be considered as "reference item ID".

Why not? On my second try coding a solution for this issue, I started renaming shotId to itemId. It didn't seem particulary hard to do. There were, AFAIC remember, less than 20 places to change.

i meant the name of the column in the db table. sqlite rename column is from 3.25 that is android 30 and later. therefore, better not rename the column name.

I can do this part for you and commit it. May be refItemId?

to extend media tables with a field for the reference item type

then there are plenty of changes to do in the sources,

Sure.

in particular the audio has a negative ID when it is a plot audio and a positive ID when it is a shot audio. this was a bad choice (made to save one field). it is not really a problem because audio point is a tester feature, and it is in the extra symbols.

We can solve this issue later. When MediaInfo is working.

there is a menu to list audios. this should be handled like the photos listing, ie include shot audio and drawing audio.

then we should add a method to the DrawingIO class that drop a media point from the tdr file. this should be executed in background, possibly with a protection to avoid opening the drawing while the file is edited (it could be also a global protection that prevent opening any drawing).

If possible, blocking only the referred file.

i would start with a global block.

photo delete should not be a frequent action.

blocking a file is more complicated because each time the user opens a plot, two files are loaded, the plan and the profile. the opening of the plot should check that neither is blocked, before starting.

not necessary but related: the DrawingWindow dialog of shots chould have the photo and audio like the ShotWindow dialog

Nice touch.

this should be straightforward.

anything else ?

How photo files are exported? Are them included in zip files? Any other export format has the photos? How to export only the photos?

media are included in the zip archive because they are part of the survey work.

the photo files are in the photo folder. of course one need also the link between files and survey. so far only csurvey supports photos: they are embedded in the exported file.

everything that is done for photo/audio could be done also for sensors, conditioned by the relevant custom setting.

i would leave this for later. when photo/audio is ok, it will be easy to replicate for sensors. to start i would setup the sensorinfo and the database table, although the item type is always shot and the itemId is always the shot id.

—

Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2156070079, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC3RVUYD4PDHYHDOCN3ZGMM7NAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGA3TAMBXHE . You are receiving this because you commented.Message ID: @.***>

rsevero commented 5 months ago

i meant the name of the column in the db table. sqlite rename column is from 3.25 that is android 30 and later. therefore, better not rename the column name.

The old way to rename column

SQLite did not support the ALTER TABLE RENAME COLUMN syntax before version 3.25.0.

If you’re using the SQLite with the version lower than 3.25.0 and could not upgrade, then you should follow these steps to rename a column:

    First, start a [transaction](https://www.sqlitetutorial.net/sqlite-transaction/).
    Second, [create a new table](https://www.sqlitetutorial.net/sqlite-create-table/) whose structure is the same as the original one except for the column that you want to rename.
    Third, copy data from the original table to the new table.
    Fourth, [drop](https://www.sqlitetutorial.net/sqlite-drop-table/) the original table.
    Fifth, rename the new table to the original table.
    Finally, commit the transaction.

A few steps, but I don't see it being an issue.

there is a menu to list audios. this should be handled like the photos listing, ie include shot audio and drawing audio.

Sure.

marcocorvi commented 5 months ago

this is probably ok. however the name of the field is a minor nuisance, since it is private to one class (DataHelper). and the tables are probably mostly empty, or with a few records.

[1] an issue is that the current "shotId" of, say, photos in drawings is -1, while it should be set to the ID of the plot that contains the photo point.

a way to deal with this is to exclude from the listing the photos with negative "shotId". the list of photos will include only the new photos in drawings.

[2] furthemore x-section photos have currently no record in the database. they can be seen only from the section-line edit dialog the x-section photo can be re-taken , but not deleted.

to start, when an x-section photo is taken it should be added to the ph0tos table. but when it is retaken no new record should be added, but the date-time should be updated.

[3] i am considering only the photos, leaving audios and sensors for later, when the management of photos is worked out

On Sat, Jun 8, 2024 at 11:03 PM Rodrigo Severo @.***> wrote:

i meant the name of the column in the db table. sqlite rename column is from 3.25 that is android 30 and later. therefore, better not rename the column name.

The old way to rename column

SQLite did not support the ALTER TABLE RENAME COLUMN syntax before version 3.25.0.

If you’re using the SQLite with the version lower than 3.25.0 and could not upgrade, then you should follow these steps to rename a column:
First, start a [transaction](https://www.sqlitetutorial.net/sqlite-transaction/).
Second, [create a new table](https://www.sqlitetutorial.net/sqlite-create-table/) whose structure is the same as the original one except for the column that you want to rename.
Third, copy data from the original table to the new table.
Fourth, [drop](https://www.sqlitetutorial.net/sqlite-drop-table/) the original table.
Fifth, rename the new table to the original table.
Finally, commit the transaction.
A few steps, but I don't see it being an issue.

there is a menu to list audios. this should be handled like the photos listing, ie include shot audio and drawing audio.

Sure.

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2156182757, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC26IDSIRLLAUPPJLZLZGNWSBAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGE4DENZVG4 . You are receiving this because you commented.Message ID: @.***>

marcocorvi commented 5 months ago

i made the branch media_items. i started doing changes there.

please review the changes.

i would like to get photos done first.

old photo points (ref id = -1) and xsection photo will not be included in the photo list. this must be documented in the man pages.

these changes will bring version to 6.3

(1) create MediaInfo extended by photo/audio/sensor infos

(2) lot of cosmetics: new names for variables and parameters

(3) many comments with TODO

(4) DrawingSensorPath left for later, even if mentioned in some comments

(5) DrawingAudioPath incomplete - likely with errors.

(6) it may compile but there are runtime errors almost for sure. test it on a scrap folder.

(7) new db version, but left old "shotId" for now

On Sun, Jun 9, 2024, 20:55 Marco Corvi @.***> wrote:

this is probably ok. however the name of the field is a minor nuisance, since it is private to one class (DataHelper). and the tables are probably mostly empty, or with a few records.

[1] an issue is that the current "shotId" of, say, photos in drawings is -1, while it should be set to the ID of the plot that contains the photo point.

a way to deal with this is to exclude from the listing the photos with negative "shotId". the list of photos will include only the new photos in drawings.

[2] furthemore x-section photos have currently no record in the database. they can be seen only from the section-line edit dialog the x-section photo can be re-taken , but not deleted.

to start, when an x-section photo is taken it should be added to the ph0tos table. but when it is retaken no new record should be added, but the date-time should be updated.

[3] i am considering only the photos, leaving audios and sensors for later, when the management of photos is worked out

On Sat, Jun 8, 2024 at 11:03 PM Rodrigo Severo @.***> wrote:
i meant the name of the column in the db table. sqlite rename column is from 3.25 that is android 30 and later. therefore, better not rename the column name.

The old way to rename column

SQLite did not support the ALTER TABLE RENAME COLUMN syntax before version 3.25.0.

If you’re using the SQLite with the version lower than 3.25.0 and could not upgrade, then you should follow these steps to rename a column:
First, start a [transaction](https://www.sqlitetutorial.net/sqlite-transaction/).
Second, [create a new table](https://www.sqlitetutorial.net/sqlite-create-table/) whose structure is the same as the original one except for the column that you want to rename.
Third, copy data from the original table to the new table.
Fourth, [drop](https://www.sqlitetutorial.net/sqlite-drop-table/) the original table.
Fifth, rename the new table to the original table.
Finally, commit the transaction.
A few steps, but I don't see it being an issue.

there is a menu to list audios. this should be handled like the photos listing, ie include shot audio and drawing audio.

Sure.

— Reply to this email directly, view it on GitHub https://github.com/marcocorvi/topodroid/issues/118#issuecomment-2156182757, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACXMYC26IDSIRLLAUPPJLZLZGNWSBAVCNFSM6AAAAABI26JFQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWGE4DENZVG4 . You are receiving this because you commented.Message ID: @.***>

rsevero commented 5 months ago

I am in a cave trip in Peru until the 25th.

I won't be able to help until I am back home.

marcocorvi commented 5 months ago

waiting for testing

marcocorvi / topodroid

Photos from photo point and section does not appear at "Info topo" photo list. #118