Automatic detection of filesystem changes

[Edgiest05]

This feature is somewhat working (it magically sometimes does not crash), but I need a little time to nail down its implementation. I'll have to actually understand how to refresh the library and not just guessing what needs to be done and also decide which events should trigger an update. I'm open to any suggestions.

[Edgiest05]

Functionality successfully implemented and logic moved to proper place in his own method.
@Thesacraft if you have anything else to suggest I'm all for it.

[Thesacraft]

I think the class definition should completly be moved out of the QtDriver class and should instead take in a QtDriver to run the events needed. Or take in the function.

[Thesacraft]

Also i don't think calling the add_new_files_callback function should be used here without modification because when the library is big there is a loading bar window that pops up and tagstudio becomes unusable aslong as the refresh is happening so add_new_files_callback could be split up into one handling the actual refresh and one that provides a pop up window.

[Edgiest05]

Understood, I'll look into it

[Edgiest05]

@Thesacraft, is this closer to what you had in mind?

[Edgiest05]

I still have to manage some TODOs (crash if changes detected while loading), but functionality is there

[Thesacraft]

Why are you defining the IsDirModifiedHandler class inside of another class it could be a standalone class and i think it should not be inside of another class for readability

[Edgiest05]

Why are you defining the IsDirModifiedHandler class inside of another class it could be a standalone class and i think it should not be inside of another class for readability

Mainly because there is no further need of such class beside of that location. It is there to just implement the "on_any_event" method that is what requires the watchdog module with its Observer class.
From a readability standpoint, if the code for the cache invalidation is not small I will make the class a scope of his own without a doubt. Do you think I should do it nonetheless?

[Thesacraft]

I think it should be moved out of there. I don't think it makes a huge diffrence because it will be loaded either way and it is just a small class. Or what do you mean by the cache invalidation

[Edgiest05]

Ok, I'll move it outside.
By cache invalidation I mean that I need logic that clears the "file_cache" dict or else it will grow for as long as the program runs (for every different file it has detected as changes)

[Edgiest05]

@Thesacraft it should be ready now, what do you think?

[Edgiest05]

Both assertions were there just for debugging purposes, sorry. I'll just remove them

[Thesacraft]

Seems good

[Edgiest05]

I've updated to the latest commit and modified the code in order to take into account the possibility of manually opening libraries. If no further changes are needed I'll be waiting for merging

[Edgiest05]

@yedpodtrzitko, the code you mention ended up being in this PR because of running automatic formatters on my end. It is not actually part of my contribution. Nonetheless I can apply the changes if you'd like me to.
Instead, regarding "requirements.txt", the packages I've introduced are latest version: what proper upper bound should I use? The latest version itself?

[yedpodtrzitko]

I was assuming you were changing the code in some way, because it was already formatted so I wouldnt expect the formatter touching something what's formatted already 🤔

anyway I dont have strong opinion about it, I just usually try to narrow and simplify the code which is being touched.

[yedpodtrzitko]

regarding "requirements.txt", the packages I've introduced are latest version: what proper upper bound should I use? The latest version itself?

it's the latest version for now. But let's say watchdog will release version 5.0 in a few weeks with breaking changes affecting us (because why not - it's a major version bump). At which point the dependency would get installed here as well.

So I'd cap it below next major versions.

watchdog>=4.0.0,<5.0.0
cachetools>=5.3.3,<6.0.0

[Edgiest05]

I was assuming you were changing the code in some way, because it was already formatted so I wouldnt expect the formatter touching something what's formatted already 🤔

anyway I dont have strong opinion about it, I just usually try to narrow and simplify the code which is being touched.

Well, improvements surely aren't a bad thing, I'll refactor it.

it's the latest version for now. But let's say watchdog will release version 5.0 in a few weeks with breaking changes affecting us (because why not - it's a major version bump). At which point the dependency would get installed here as well.

So I'd cap it below next major versions.

watchdog>=4.0.0,<5.0.0
cachetools>=5.3.3,<6.0.0

Good call, thanks for your suggestion.

[Edgiest05]

All done, I kept the pointless logic because it is probably something that is still being worked on.