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

[Feature] Transaction-based access to the GPS positions buffer #18

Closed
HarelM opened this issue Apr 24, 2021 · 10 comments · Fixed by #38
Closed

[Feature] Transaction-based access to the GPS positions buffer #18

HarelM opened this issue Apr 24, 2021 · 10 comments · Fixed by #38
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed
Milestone

Comments

@HarelM
Copy link
Collaborator

HarelM commented Apr 24, 2021

See here: IsraelHikingMap/Site#1427

When using the locations buffer to get the location from the plugin and after that deleting the location buffer it might get to a race condition where the delete happens after a location was added or that getting all location will happen before all locations were deleted.

Your Environment

  • Plugin version: 1.0
  • Platform: Android (observed there but might be on iOS too)
  • OS version: 9
  • Device manufacturer and model: An old tablet
  • Cordova version (cordova -v): 10.0
  • Cordova platform version (cordova platform ls): Android 9.1
  • Plugin configuration options: Not so relevant
  • Link to your project: See issue above for a link to the project

Context

Problem in getting GPS positions while the app is going back and forth from the background

Expected Behavior

Allow the app not to miss or duplicate positions

Actual Behavior

Positions are duplicated or missed

Possible Fix

Add an API method to do both get and delete

Steps to Reproduce

  1. Hard to say, but using the get locations and then right after the delete locations

Context

See screen shot in linked issue

Debug logs

Not available

@RaddishIoW RaddishIoW added bug Something isn't working help wanted Extra attention is needed enhancement New feature or request and removed bug Something isn't working labels Apr 24, 2021
@HarelM HarelM self-assigned this May 3, 2021
@HarelM
Copy link
Collaborator Author

HarelM commented May 10, 2021

I'm planning to try and resolve this soon.
Game plan:

  1. Branch out
  2. Add a new API method getValidLocationsAndDelete() to allow deleting right after fetching from database according to the IDs fetched.
  3. In Android I'm planning to change SQLiteLocationDAO.java using delete and get in the same method so that I delete according to indexes that I just fetched
  4. In iOS MAURSQLiteHelper.m using the same concept.
  5. Test on devices
  6. PR, merge, bump minor version (1.1 probably)

The query should be something like "update location-table set status deleted where id in (? ? ? ? ? ?) , .
See here:
https://stackoverflow.com/questions/42896599/update-multiple-rows-based-on-id-in-sqlite-on-android

This shouldn't take more than a few lines of code...

@HarelM HarelM added this to the 1.1 milestone May 10, 2021
@HarelM
Copy link
Collaborator Author

HarelM commented May 10, 2021

I'm disappointed from the code I see in MAURSQLiteLocationDAO.m.
There are three request to the database using different fields kinda copy-paste from one to the other, I also think there's also a bug there - trying to get a value of a column that wasn't fetched:

NSTimeInterval recordedAt = [rs longForColumnIndex:11];

I'm thinking I should refactor this to use the same columns and update the object using all the data and not miss one column or another, like it is in the android version...
@RaddishIoW let me know what you think...

@RaddishIoW
Copy link

Yep, all sounds good to me. :) Nice one.

HarelM added a commit that referenced this issue May 12, 2021
@HarelM
Copy link
Collaborator Author

HarelM commented May 12, 2021

OK, I committed the main changes to a branch without compiling or anything, which is a practice I'm not familiar with, but I'm not sure how to work in Cordova plugin environment yet...
I would like to see if I can use the plugin code straight from github to test it in my local environment, I'm not entirely sure how to do it yet, but it's an opportunity to learn :-)

@HarelM
Copy link
Collaborator Author

HarelM commented May 12, 2021

Note to myself: I'm not sure the class com.marianhello.bgloc.data.sqlite.SQLiteLocationDAO is used anywhere in the project... Might be an old class that's not in use anymore...?

HarelM added a commit that referenced this issue May 12, 2021
…fixes incorrect calls and missing facade and js code
HarelM added a commit that referenced this issue May 12, 2021
HarelM added a commit that referenced this issue May 12, 2021
HarelM added a commit that referenced this issue May 12, 2021
HarelM added a commit that referenced this issue May 12, 2021
HarelM added a commit that referenced this issue May 13, 2021
@RaddishIoW
Copy link

I would like to see if I can use the plugin code straight from github to test it in my local environment, I'm not entirely sure how to do it yet, but it's an opportunity to learn :-)

I believe you can reference the GitHub.com repo URL directly when installing:
cordova plugin add http://github.com/HaylLtd/cordova-background-geolocation-plugin.git
^ I haven't checked that command - so check it before you run it! :)

@HarelM
Copy link
Collaborator Author

HarelM commented May 13, 2021

Yes, I can use it this way... Took me a few tries until it compiled... I think it's working.
I'll send a pull request soon so you can review it.

@HarelM
Copy link
Collaborator Author

HarelM commented Jun 25, 2021

After merging the related PR I plan to release a version with both fixes, the documentation are getting updated as soon as I push to develop, so they will probably show before the version is released, but I'm fine with that.

@RaddishIoW
Copy link

After merging the related PR I plan to release a version with both fixes, the documentation are getting updated as soon as I push to develop, so they will probably show before the version is released, but I'm fine with that.

Yep, I'm good with releasing these changes as 1.1. I've created a discussion (#40) that could deal with the issue of the docs changing before peoples' eyes - I think it would make sense...

HarelM added a commit that referenced this issue Jun 26, 2021
* [Feature] Transaction-based access to the GPS positions buffer #18 - initial commit

* [Feature] Transaction-based access to the GPS positions buffer #18 - fixes incorrect calls and missing facade and js code

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix compilation

* [Feature] Transaction-based access to the GPS positions buffer #18 - Add typesript definitions

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix iOS compilation and typo

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix iOS compilation

* [Feature] Transaction-based access to the GPS positions buffer #18 - Add missing import

* Update documentation and allow using promise.
@HarelM
Copy link
Collaborator Author

HarelM commented Jun 26, 2021

Resolved by #38

@HarelM HarelM closed this as completed Jun 26, 2021
HarelM added a commit that referenced this issue Jun 27, 2021
* Set theme jekyll-theme-hacker

* Update Changelog

* docs: move to github pages (#35)

Fixes #32

* Update Changelog

* Adds promises and subscription (#36)

Resolves #16 

* Add promises and subscription to js interface #16 - initial commit

* #16 - fix subscriable interface

* bump version, fix docs

* fix incorrect link

* Update Changelog

* build: add workflow to automatically merge to stable on release (#42)

* Update Changelog

* GPS transactional location fetch (#38)

* [Feature] Transaction-based access to the GPS positions buffer #18 - initial commit

* [Feature] Transaction-based access to the GPS positions buffer #18 - fixes incorrect calls and missing facade and js code

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix compilation

* [Feature] Transaction-based access to the GPS positions buffer #18 - Add typesript definitions

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix iOS compilation and typo

* [Feature] Transaction-based access to the GPS positions buffer #18 - Fix iOS compilation

* [Feature] Transaction-based access to the GPS positions buffer #18 - Add missing import

* Update documentation and allow using promise.

* Update Changelog

Co-authored-by: Adam Radestock <[email protected]>
Co-authored-by: RaddishIoW <[email protected]>
Co-authored-by: HarelM <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants