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

File change detection rewrite #3048

Merged
merged 19 commits into from Jan 19, 2015

Conversation

Projects
None yet
2 participants
@pvrs12
Contributor

pvrs12 commented Jan 18, 2015

I rewrote the original file change detection code that I had written so that it no longer uses the Java 7 FileWatcher tools. This should fix many of the issues that the old system had.

The big changes between this and the previous version are

  • Change detection code has been moved from Editor.java to ChangeDetector.java
  • Detecting changes simply scans the directory for any files whose contents do not match the contents in memory or any files which have been created or deleted
  • The dialogs have been changed to be more clear, particularly emphasizing that reloading your sketch will clear any unsaved work
  • There is more support for choosing not to reload your files by marking the sketch files as modified so they will actually be written out when the sketch is saved

I have tested this on Archlinux, Windows 8, and OS X 10.10 (via a hackintosh VM) and I believe it should be working correctly now. Fixing #2852 and #2759

The commit log for this is a huge mess because I still don't fully get git. If anyone would be willing to help me clean it up I would be grateful.

pvrs12 added some commits Jan 18, 2015

Merge branch 'master' of github.com:pvrs12/processing
Conflicts:
	core/src/processing/core/PSurfaceAWT.java
Commented out old change detection from Editor.java and
EditorHeader.java
Added a new ChangeDetector class which does not rely on Java 7's
FileWatcher
Added error handling if Base.loadFile throws an IOException (disable
listener)
Fully removed the old file change detection code
Added support for choosing not to reload the sketch by setting the
changed files to modifed, so that when saved they will be updated on
disk
Made the reload confirmation message more clear in specifying that
unsaved changes will be destroyed
More support for attempting to reload after deleting the entire sketch
Changed ChangeDetector's constructor's signature to only require an
Editor
Reworking file detection code to be independent of Java's detection code
Added error handling if Base.loadFile throws an IOException (disable
listener)
Fully removed the old file change detection code

Added support for choosing not to reload the sketch by setting the
changed files to modifed, so that when saved they will be updated on
disk
Made the reload confirmation message more clear in specifying that
unsaved changes will be destroyed
Moved all file change detection code out to ChangeDetector.java
No longer relies on Java's FileWatcher
Dialogs are more clear in emphasizing that changes will be lost on reload
More support for choosing not to reload by setting the changed files to be modified
Moved all file change detection code out to ChangeDetector.java
No longer relies on Java's FileWatcher
Dialogs are more clear in emphasizing that changes will be lost on reload
More support for choosing not to reload by setting the changed files to be modified
@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 19, 2015

Member

Thanks very much for looking into it. This also needs to run on a thread (the file listing on window selection), otherwise anytime a user has an extremely full sketch folder (i.e. they've had saveFrame() running inside draw()), there will be significant lag/UI lockup when making the window active.

Member

benfry commented Jan 19, 2015

Thanks very much for looking into it. This also needs to run on a thread (the file listing on window selection), otherwise anytime a user has an extremely full sketch folder (i.e. they've had saveFrame() running inside draw()), there will be significant lag/UI lockup when making the window active.

@pvrs12

This comment has been minimized.

Show comment
Hide comment
@pvrs12

pvrs12 Jan 19, 2015

Contributor

Good point about threading, I'll take care of that. Is there a specific format that you like to use with processing for threaded tasks or just the standard java threading?

Contributor

pvrs12 commented Jan 19, 2015

Good point about threading, I'll take care of that. Is there a specific format that you like to use with processing for threaded tasks or just the standard java threading?

@pvrs12

This comment has been minimized.

Show comment
Hide comment
@pvrs12

pvrs12 Jan 19, 2015

Contributor

This commit should switch it to using Java Threads to run the scan for changes

Contributor

pvrs12 commented Jan 19, 2015

This commit should switch it to using Java Threads to run the scan for changes

benfry added a commit that referenced this pull request Jan 19, 2015

@benfry benfry merged commit c59d754 into processing:master Jan 19, 2015

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 19, 2015

Member

Cool, thanks. Merged for the next alpha and we can try to shake out any bugs there.

For the threading, just the regular Java threading is all we're looking for, though just mind any issues re: using the EDT. Update the GUI from the EDT using EventQueue.invokeLater() as necessary, since a new thread will be separate from the EDT.

Member

benfry commented Jan 19, 2015

Cool, thanks. Merged for the next alpha and we can try to shake out any bugs there.

For the threading, just the regular Java threading is all we're looking for, though just mind any issues re: using the EDT. Update the GUI from the EDT using EventQueue.invokeLater() as necessary, since a new thread will be separate from the EDT.

@pvrs12

This comment has been minimized.

Show comment
Hide comment
@pvrs12

pvrs12 Jan 19, 2015

Contributor

Do you want me to add the EventQueue.invokeLater() to the code? The only GUI elements to it are the dialogs, and some calls to header.rebuild()

Contributor

pvrs12 commented Jan 19, 2015

Do you want me to add the EventQueue.invokeLater() to the code? The only GUI elements to it are the dialogs, and some calls to header.rebuild()

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 19, 2015

Member

Yikes, this is totally broken on OS X. Even attempting a "save" brings up the "do you want to reload?" dialog before I even have a chance to save. Is this scanning for all files (not just .java and .pde or the valid extensions for the mode)? I wonder if OS X is writing/modifying the .DS_Store file.

Member

benfry commented Jan 19, 2015

Yikes, this is totally broken on OS X. Even attempting a "save" brings up the "do you want to reload?" dialog before I even have a chance to save. Is this scanning for all files (not just .java and .pde or the valid extensions for the mode)? I wonder if OS X is writing/modifying the .DS_Store file.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 19, 2015

Member

Yes, add invokeLater() as necessary. If you're not familiar with it, please read up on it first.

Member

benfry commented Jan 19, 2015

Yes, add invokeLater() as necessary. If you're not familiar with it, please read up on it first.

@pvrs12

This comment has been minimized.

Show comment
Hide comment
@pvrs12

pvrs12 Jan 19, 2015

Contributor

I'm surprised that's happening, I checked it on an OS X VM and everything seemed good.

It runs 2 scans, the first to see if there are any additions to the directory of files with the extensions specifed in the editor's current mode, and a second on the files and their contents that are actually in the sketch.

I'll run through it some more on OS X to see if I can replicate your error. OS X seems to be the bane of my programming

Contributor

pvrs12 commented Jan 19, 2015

I'm surprised that's happening, I checked it on an OS X VM and everything seemed good.

It runs 2 scans, the first to see if there are any additions to the directory of files with the extensions specifed in the editor's current mode, and a second on the files and their contents that are actually in the sketch.

I'll run through it some more on OS X to see if I can replicate your error. OS X seems to be the bane of my programming

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 22, 2015

Member

Ok, we may just have an issue with how the logic works on the change detection works in the current version. What should be happening is that the timestamp (File.lastModified()) of each file should be stored when it's loaded or saved in SketchCode. Then when the window gets focus, the current timestamp of the file should be compared to that. It's not perfect (if modified externally w/o activating/deactivating the frame, it won't pick it up), but it's far more reliable (and faster) than re-loading the contents of the file.

Member

benfry commented Jan 22, 2015

Ok, we may just have an issue with how the logic works on the change detection works in the current version. What should be happening is that the timestamp (File.lastModified()) of each file should be stored when it's loaded or saved in SketchCode. Then when the window gets focus, the current timestamp of the file should be compared to that. It's not perfect (if modified externally w/o activating/deactivating the frame, it won't pick it up), but it's far more reliable (and faster) than re-loading the contents of the file.

@pvrs12

This comment has been minimized.

Show comment
Hide comment
@pvrs12

pvrs12 Jan 22, 2015

Contributor

That's a good point and a much better implementation that reading the whole file. Would it make sense to just grab the File.lastModified() time when focus is lost and compare it to when it is gained again?

Additionally, the current version scans the directory and compares the number of source files with the number of files in the sketch. This will cause it to also refresh if a new .pde file is added, or one is deleted. Does it make sense to keep this too?

Contributor

pvrs12 commented Jan 22, 2015

That's a good point and a much better implementation that reading the whole file. Would it make sense to just grab the File.lastModified() time when focus is lost and compare it to when it is gained again?

Additionally, the current version scans the directory and compares the number of source files with the number of files in the sketch. This will cause it to also refresh if a new .pde file is added, or one is deleted. Does it make sense to keep this too?

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 22, 2015

Member

I think during load/save is best, because that's technically the contents of what the PDE knows to be the correct code.

The edge case is a sketch that's open and running from a network-accessible location (a shared folder, or Dropbox, for that matter), in which case the file could be modified without the user de-focusing and re-focusing the window. For that case, we should probably do a check when saving, and if the stamp doesn't match, notify the user and rename the modified file to .conflict or something like that.

And yes, we'll want to keep the file scan. Thanks for your efforts on this.

Member

benfry commented Jan 22, 2015

I think during load/save is best, because that's technically the contents of what the PDE knows to be the correct code.

The edge case is a sketch that's open and running from a network-accessible location (a shared folder, or Dropbox, for that matter), in which case the file could be modified without the user de-focusing and re-focusing the window. For that case, we should probably do a check when saving, and if the stamp doesn't match, notify the user and rename the modified file to .conflict or something like that.

And yes, we'll want to keep the file scan. Thanks for your efforts on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment