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

Multi valued item location map #2088

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

patrickdalla
Copy link
Collaborator

Implements support for items with multi valued locations metadata.

Highlight/selection are done based on item, i. e., when one item is highlighted all correspondent marks on map are highlighted, and one mark on map is highlighted all other marks of the same item are highlighted.

patrickdalla and others added 5 commits May 20, 2024 11:33
selected item does not have its own JSON feature.
… a new selected item does not have its own JSON feature."

This reverts commit 34d169c.
selected item does not have its own JSON feature.

Merge branch 'MultiValuedItemLocMap' of
https://github.com/sepinf-inc/IPED.git into MultiValuedItemLocMap
@lfcnassif
Copy link
Member

lfcnassif commented Jun 3, 2024

Hi @patrickdalla, I'm starting to test this, thanks for this fix!

Highlight/selection are done based on item, i. e., when one item is highlighted all correspondent marks on map are highlighted, and one mark on map is highlighted all other marks of the same item are highlighted.

How is the expected behavior for checkboxes? When one item is checked on table, all locations should be checked on map? And if one or just a subset of the locations related to the same table item are checked on map, what should be the expected item state: checked or not?

@patrickdalla
Copy link
Collaborator Author

"How is the expected behavior for checkboxes? When one item is checked on table, all locations should be checked on map? "
Yes.

"And if one or just a subset of the locations related to the same table item are checked on map, what should be the expected item state: checked or not?"
Checked. All related subitems of the items that had at least one subitem as input of the check operation will be checked also.

As the check/uncheck is per item, all subitems on map will reflect the item state.

@lfcnassif
Copy link
Member

lfcnassif commented Jun 3, 2024

Testing with one WhatsApp database, expanding single messages, 119 locations were plotted on the map. When disabling single message expansion, just 115 locations were plotted. I think the number should be the same, right?

@lfcnassif
Copy link
Member

lfcnassif commented Jun 4, 2024

Testing with one WhatsApp database, expanding single messages, 119 locations were plotted on the map. When disabling single message expansion, just 115 locations were plotted. I think the number should be the same, right?

I didn't find the cause of this issue, but there is multithreaded code in GetResultsJSWorker class reading and writing to non thread safe variables, like itemsWithGPS and gpsItems, please take a look @patrickdalla. And maybe the Semaphore class usage might be avoided and replaced by a simpler synchronization approach.

@patrickdalla
Copy link
Collaborator Author

You are right! In fact, no real advantage is gained with this multi threading, they run almost in sequence. I made some structure to test multi threading, leaving the final parameters and code to run as if no multi thread was used, as I could not note any performance gain. But this code increases complexity, I will remove it and simplify.

really gain no performance, keeping code simple in single thread
implementation.
@patrickdalla
Copy link
Collaborator Author

patrickdalla commented Jun 4, 2024

Testing with one WhatsApp database, expanding single messages, 119 locations were plotted on the map. When disabling single message expansion, just 115 locations were plotted. I think the number should be the same, right?

I found similar case. The whatsapp chat parsers extracted only 19 geolocations in metadata when extractMessages is false for one of the chats. But when extractMessages is true, it extracts 21 distinct instant messages georeferenced items. For this chat I could note that there are 2 pairs of IMs with same georeferences. Maybe is the whatsapp chat parser removes duplicates.

Check if it is also the same issue for your case. In this case it would not be an error of this PR.

Maybe it would not even be an issue of whatsapp parser, but a normal result. Wouldn't be lucene that removes these redundancies on same metadata?

@wladimirleite
Copy link
Member

Maybe is the whatsapp chat parser removes duplicates.

As far as I know, the parser does not remove duplicated locations.

Maybe it would not even be an issue of whatsapp parser, but a normal result. Wouldn't be lucene that removes these redundancies on same metadata?

Duplicated values in the same item are explicitly removed by MetadataUtil.removeDuplicateValues(Metadata metadata).
If there are duplicated locations in the same chat, the number of locations is expected to be different, right?!
If extractMessages is enabled, locations are set in individual messages (in this case, duplicated locations will be assigned to different items).
And if extractMessages is disabled, all locations belonging to a chat will be assigned to a single item, and later the duplicated ones will be removed.

@lfcnassif
Copy link
Member

Duplicated values in the same item are explicitly removed by MetadataUtil.removeDuplicateValues(Metadata metadata).
If there are duplicated locations in the same chat, the number of locations is expected to be different, right?!
If extractMessages is enabled, locations are set in individual messages (in this case, duplicated locations will be assigned to different items).
And if extractMessages is disabled, all locations belonging to a chat will be assigned to a single item, and later the duplicated ones will be removed.

Thanks @patrickdalla and @wladimirleite, that is the exact cause! I forgot the MetadataUtil.removeDuplicateValues(Metadata metadata) method (implemented by me...), disabling it for testing, the same number of locations were extracted with extractMessages on or off.

@lfcnassif
Copy link
Member

Maybe is the whatsapp chat parser removes duplicates.

I checked this yesterday carefully and it doesn't happen, the cause is really what @wladimirleite pointed out. I think current behavior is fine. Sorry @patrickdalla for taking your time.

@lfcnassif
Copy link
Member

"How is the expected behavior for checkboxes? When one item is checked on table, all locations should be checked on map? " Yes.

"And if one or just a subset of the locations related to the same table item are checked on map, what should be the expected item state: checked or not?" Checked. All related subitems of the items that had at least one subitem as input of the check operation will be checked also.

As the check/uncheck is per item, all subitems on map will reflect the item state.
"How is the expected behavior for checkboxes? When one item is checked on table, all locations should be checked on map? " Yes.

"And if one or just a subset of the locations related to the same table item are checked on map, what should be the expected item state: checked or not?" Checked. All related subitems of the items that had at least one subitem as input of the check operation will be checked also.

As the check/uncheck is per item, all subitems on map will reflect the item state.

Unfortunately behavior described above is not working properly, see video below:

checkboxes.inconsistency.mp4

@lfcnassif
Copy link
Member

PS1: You can see the checked item on table but both locations unchecked on map at right side.
PS2: When checking one location on map, the other, at the same place and related to the same item, is not checked.
PS3: The balloon checkbox inconsistency also happens with expanded locations when extractMessages is true.

patrickdalla added a commit to thiagofuer/IPED that referenced this pull request Jun 6, 2024
…r to the

value in output text field (if one already exists).
patrickdalla added a commit to thiagofuer/IPED that referenced this pull request Jun 6, 2024
patrickdalla added a commit to thiagofuer/IPED that referenced this pull request Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants