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

SQLite DB with SQLAlchemy II #332

Merged
merged 17 commits into from
Sep 9, 2024
Merged

Conversation

yedpodtrzitko
Copy link
Collaborator

@yedpodtrzitko yedpodtrzitko commented Jul 25, 2024

This takes some parts which has been done in #190 and keep working on it further

The transfer from JSON backend to SQL backend isnt as simple as replacing json.load() with db.select() so there is a lot of more refactoring to do before this will be production ready. The current JSON implementation suffers with memleaks (see #208) and is slightly over-engineered at some places, so that's yet another reason for a refactoring.

The reason why even open the PR at this time is to keep track of what's done, and to avoid potential duplication of work in case someone else would feel like working on the DB implementation. In such case, feel free to open PR against this branch.

Unfortunately I dont keep updating this branch with anything what's landing in the main branch, so by that time this will be any close to being done, the main branch might divert very far.

Migration process

  • opening library
  • displaying items
  • pagination
  • removing entry fields
  • adding entry fields
  • creating tags
  • items selection
  • searching via tag name
  • searching with include/exclude list
  • reserve tag IDs under 1000 for internal use
  • bug: searching is now case-sensitive
  • save paths in POSIX format
  • bug: Archived/Favorite tags are misbehaving with multiselect
  • bug: title field in multiselect shows as "Mixed Data"
  • feat: Macros - Folders to Tags
  • feat: Macros - Sort Fields
  • feat: Macros - Autofill
  • feat: Unlinked Entries
  • feat: move fields into DB
  • Closing Library errors
  • bug: Closing library keeps pagination shown
  • feat: Save Library Backup
  • misc: Save Library (probably just remove, all operations go directly to DB)
  • feat: subtags/parent tags
  • feat: tag alias
  • feat: tag shorthand

@CyanVoxel CyanVoxel added Type: Refactor Code that needs to be restructured or cleaned up TagStudio: Library Relating to the TagStudio library system Priority: High An important issue requiring attention TagStudio: Tags Relating to the TagStudio tag system labels Jul 25, 2024
@CyanVoxel
Copy link
Member

This looks like a great barebones(ish) replacement to the JSON system so far, I think this is the best bet to get a SQL in the door without worrying about new schemas.

Taking a look at your comments, I'm keeping track of the features that still need to be implemented. So far I can tell there's at least:

  • Select All functionality
  • Various Macro functionality (was unofficial in the first place so not a priority)
  • Ability to purge items from the NavigationState/FilterState
  • Probably everything else that hasn't been touched?

From my own precursory testing, when opening an existing library with 200 items (191 visible with extension list filter), it comes back with supposedly 20 invisible results and tells me that most if not all the items are "not in library yet", such as with the following log snippet:

2024-07-24 23:49:35 [info     ] item not in library yet        path=WindowsPath('3D/d20 scene.mtl')
2024-07-24 23:49:35 [info     ] item not in library yet        path=WindowsPath('3D/d20 scene.obj')

Refreshing the library for new files also does not change anything here. Could be a case of the Windows paths acting funky somewhere, although that's just a surface-level guess.

I would also recommend basing this against the Alpha-v9.4 branch whenever it feels right, that way the changes there don't come as a sudden surprise. I don't think there's much going on there that will get in the way of this, but new changes are new changes nonetheless. I don't expect this to be ready by 9.4, but if it were then I would give this precedent over any new feature PRs. I feel that this would likely be included in 9.5 along with the new search syntax additions (now that will need some work resolving).

Please let me know what I can do to help speed this along or help smooth the transition in any way as well. Thank you so much for working on this!

@CyanVoxel
Copy link
Member

I'll check out these changes soon and get back to you 👍

In the meantime, I noticed that the apprun.yaml workflow was removed. Was that intentional?

@yedpodtrzitko
Copy link
Collaborator Author

In the meantime, I noticed that the apprun.yaml workflow was removed. Was that intentional?

Yes, that's on purpose. It happened that there was some error when running the app a few times, but apprun job was still green. I replaced it with in tests/qt. Each of these is testing a different component of the app, so the overall coverage is much better. It's also more granular so when a test for a specific component fails, you know where to start looking.

@CyanVoxel CyanVoxel added the Status: Mergeable The code is ready to be merged label Sep 9, 2024
@CyanVoxel CyanVoxel merged commit e5e7b8a into TagStudioDev:main Sep 9, 2024
5 checks passed
@CyanVoxel CyanVoxel removed the Status: Mergeable The code is ready to be merged label Sep 9, 2024
@CyanVoxel
Copy link
Member

Thank you again SO MUCH for all of your hard work on this!! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High An important issue requiring attention TagStudio: Library Relating to the TagStudio library system TagStudio: Tags Relating to the TagStudio tag system Type: Refactor Code that needs to be restructured or cleaned up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants