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

Add support for Grid 3 #3

Merged
merged 22 commits into from
May 17, 2021
Merged

Conversation

gavinhenderson
Copy link
Contributor

@gavinhenderson gavinhenderson commented May 13, 2021

Still under development, just wanted to open a PR so you could follow my work.

  • Add Grid3 Datasource
  • Pull data out of a Grid3 datasource
  • Takes in a Grid 3 root folder path
  • Find real paths to grid3 files
  • Figure out why not all data ends up in history file (single words)
  • Figure out if we publish test-data (we dont)
  • Test on remote desktop
  • Test on real device
  • Automated tests

Currently not everything is logged to the phrases file, single words spoken words are not logged by the looks of things. Despite this its still valuable to get all the spoken full phrases from Grid

@codetheweb codetheweb marked this pull request as draft May 13, 2021 23:12
@gavinhenderson
Copy link
Contributor Author

@codetheweb Your code is brilliant 👌 Made adding this source so much easier

@gavinhenderson gavinhenderson marked this pull request as ready for review May 14, 2021 13:26
@gavinhenderson
Copy link
Contributor Author

Hey @codetheweb If you have time can you give this PR a review?

I've added a new app to the appFactory. I called it Grid. I considered doing a more generic sqlite data source but to be honest there is a fair amount of Grid specific code and the sqlite stuff is probably better off just being duplicated if we end up with another sqlite data source.

I also added a warning to the data source screens to mention that this will pull every users history. It is unlikely that anyone will have more than one user that is active but thought we should still call it out.

Any questions or comments to hesitate to ask :)

@gavinhenderson gavinhenderson changed the title [WIP] Add support for Grid 3 Add support for Grid 3 May 14, 2021
Copy link
Member

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks.

Last thing: several relatively large .sqlite files have been added for testing (👍); would it be possible to re-export a smaller subset of data from those files?

main/server/apps/grid.ts Show resolved Hide resolved
package.json Show resolved Hide resolved
renderer/pages/setup/4.tsx Outdated Show resolved Hide resolved
@willwade
Copy link

On the sources list I wonder if it would be possible to put each grid user it finds as a separate source. Rather than currently it puts them all as one source. You’re going to have the odd situation where a device is shared - maybe just on loan - and you might want to remove all the users except one. If it’s a headache I wouldn’t worry. It’s going to be a rare situation.

@gavinhenderson
Copy link
Contributor Author

gavinhenderson commented May 17, 2021

@codetheweb and @willwade

Changes made:

  • Made the sqlite files smaller by removing data we dont need
  • Make the hash a standard length by hashing the hashes
  • Updated the text (upload -> add)

@willwade - My thinking is we wait and see how much of problem that is? If we are constantly rejecting people from the collection due to not being able to submit because they are on a shared device then we can look into it but I don't think we should pre-empt the problem. Unless you are certain it is a common case?

Copy link
Member

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

LGTM, just waiting on @willwade.

@willwade
Copy link

willwade commented May 17, 2021

Looks good to me!

To clarify - I think you are right. I don't think it should be a big problem in the real world. Services shouldn't really be giving users devices with other peoples data on there..

Go for it :)

@codetheweb codetheweb merged commit 5c0633e into Baton-donation:master May 17, 2021
@codetheweb
Copy link
Member

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