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

Auto-reload #99

Closed
wants to merge 13 commits into from
Closed

Auto-reload #99

wants to merge 13 commits into from

Conversation

funkyfuture
Copy link
Contributor

This is an auto-reload feature. I used it successfully in conjunction with Simpletask Cloudless and BTsync.
But it may have quirks on race conditions.

could solve #92

@mNantern mNantern modified the milestone: 1.4 Aug 30, 2014
@mNantern
Copy link
Collaborator

Ok, I checkout your code. I will try that this week but at first sight it works well !

It remains only #59 before launching QTodoTxt v1.4 (or 2.0 ?) !

@funkyfuture
Copy link
Contributor Author

before releasing a next Major release, i would improve at least some of the following aspects:

  • a more intuitive backend-API, like proposed in implement classes for task components #94
  • more signaling between components
  • CSS-theming
  • usage of .ui-files
  • a comprehensive code-review with a ot lof in-source-documentation (atm one can still figure out how something is supposed to work, but it's not that intutive and there are inconsistencies)

mNantern added a commit that referenced this pull request Sep 6, 2014
try:
self.parent().openFileByName(self._file.filename)
except ErrorLoadingFile:
pass # let auto-reload errors pass silently
Copy link
Collaborator

Choose a reason for hiding this comment

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

@funkyfuture : I just try the auto-reload with QTodoTxt on MacOs and Windows and I always have an exception here on Windows:

[Errno 13] Permission denied: 'path to my Dropbox file'

Looks like we try to openFileByName before the file is completely written. It happens only when I close or delete a task on Mac. Creating or modifying a task is correctly replicated.
Adding a (ugly) sleep(1) before loading the file works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think i will add some debbugging-statements and maybe some tests into it within the next days to really understand what's going on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and by "closing" a task, you mean to mark it as completed? just not to misunderstand you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, It was when I tried to complete a task.

- some debug-statements to keep track of file handling
@funkyfuture
Copy link
Contributor Author

at which point did you put the sleep(1) exactly?

anyway, i think we can get a better insight now with debug-logging enabled.

@funkyfuture
Copy link
Contributor Author

i just noticed that the logging doesn't actually doesn't take into account what is set as cli-parameter. it will log anyway.
but i will go to bed now.

@mNantern
Copy link
Collaborator

mNantern commented Sep 9, 2014

I put the sleep just before self.parent().openFileByName(self._file.filename)

@mNantern
Copy link
Collaborator

Ok I made more test and I just find the sleep solution. Even a 10ms sleep is enough so I will let that until we find a better solution.

Version 1.4 is now feature complete, I will test it during 1 week and if everything is ok I will release it !

@mattiasdh
Copy link

Cool, looking forward to it! 👍

@funkyfuture
Copy link
Contributor Author

@mNantern could you please post the logs for the problematic situation with and without the 10msec-sleep?

@mNantern
Copy link
Collaborator

@funkyfuture :

With the sleep:

19:58:44.26 [qtodotxt.lib.file] DEBUG: Setting up FileObserver instance.
19:58:44.36 [qtodotxt.ui.controllers.main_controller] DEBUG: MainController.openFileByName called with filename="C:/Users/Matt/Dropbox/todotxt/todo.txt"
19:58:44.186 [qtodotxt.ui.controllers.main_controller] DEBUG: Adding C:/Users/Matt/Dropbox/todotxt/todo.txt to watchlist
19:59:11.546 [qtodotxt.lib.file] DEBUG: Detected change on C:/Users/Matt/Dropbox/todotxt/todo.txt
removing it from watchlist
19:59:11.556 [qtodotxt.ui.controllers.main_controller] DEBUG: MainController.openFileByName called with filename="C:/Users/Matt/Dropbox/todotxt/todo.txt"
19:59:11.566 [qtodotxt.ui.controllers.main_controller] DEBUG: Adding C:/Users/Matt/Dropbox/todotxt/todo.txt to watchlist
19:59:19.556 [qtodotxt.lib.file] DEBUG: Detected change on C:/Users/Matt/Dropbox/todotxt/todo.txt
removing it from watchlist
19:59:19.566 [qtodotxt.ui.controllers.main_controller] DEBUG: MainController.openFileByName called with filename="C:/Users/Matt/Dropbox/todotxt/todo.txt"
19:59:19.576 [qtodotxt.ui.controllers.main_controller] DEBUG: Adding C:/Users/Matt/Dropbox/todotxt/todo.txt to watchlist

Without I can't reproduce it. I hate this bug !

It happens with every other log level (maybe that's just because logging in DEBUG takes time and so the micro sleep is not needed anymore...):

QTodoTxt\bin [master +1 ~1 -1 !]> .\qtodotxt.py -l INFO
[Errno 13] Permission denied: 'C:/Users/Matt/Dropbox/todotxt/todo.txt'

@funkyfuture
Copy link
Contributor Author

hmpf, so i guess a
if os.name == 'nt: sleep(0.01)
would help as much as the debugging.

i'd actually prefer a more controlled approach than a fixed time since you can't know the race conditions of a system.
shall i try something that does condition checks?

@mNantern
Copy link
Collaborator

What do you mean by "condition checks" ?

Ok, I will add a check on the OS name but I think that this feature is good like that. We can release it and see what bug we get :)

@funkyfuture
Copy link
Contributor Author

no, i mean that an OS-check would have the same effect as the debug-logging. it's useless.

with conditioncheck i mean it should be tested if the file can be opened. if not, wait until it can be. because you can't know if 10msec are enough on every system.

@mNantern
Copy link
Collaborator

With that every system (Mac and Linux) will pay the additional call to open.

We can leave it like that and when catching an Exception wait a few millisecs (10 for example) and call again openFileByName. We should also limit the number of retry to 3.

Something like that:

    def retryOpen(self,filename,retryNumber):
     while retryNumber < 3:
           retryNumber += 1
           try:
               self.parent().openFileByName(self._file.filename)
           except ErrorLoadingFile:
              sleep(0.01)
              retryOpen(self._file.filename,retryNumber)

What do you think ?

@funkyfuture
Copy link
Contributor Author

i would avoid recursions as much as possible. beside that i think another method is too much for that issue.

i made a change that will try to read the file within a maximum of one second. i think that is still reasonable for a fluent user experience and should also work on slower machines.

here's the commit: 042f4b2

@mNantern
Copy link
Collaborator

I will test that today. Seems ok even if they are no more the beauty of recursion 😜

mNantern added a commit that referenced this pull request Sep 26, 2014
We try to open todo.txt file for 1 sec before giving up
@mNantern
Copy link
Collaborator

It was almost working, just missing a last "break" statement to not keep looping until 1 sec passed.

I think we are good for a release tomorrow.

Thanks a lot for your hard work !

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.

None yet

3 participants