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

Fix auto upload #807

Merged
merged 19 commits into from
Apr 3, 2019
Merged

Fix auto upload #807

merged 19 commits into from
Apr 3, 2019

Conversation

auxves
Copy link
Contributor

@auxves auxves commented Mar 24, 2019

Short description of what this resolves:

This PR fixes auto-upload by using Chokidar and vscode.extensions.onDidChange instead of vscode.FileSystemWatcher, which only works on files in the current workspace.

Changes proposed in this pull request:

  • Use Chokidar instead of vscode.FileSystemWatcher to detect changes in user settings
  • Use VSC's onDidChange event to check for changes in extensions
  • Unlock lock file when returning from the StartWatch() function after locking to prevent it from becoming unusable

Fixes: 3

How Has This Been Tested?

I have tested it by installing/uninstalling extensions and modifying my settings. I can confirm that the changes don't affect any other areas in the code, and it uploads and downloads successfully.

Checklist:

  • I have read the contribution guidelines.
  • My change requires a change to the documentation and GitHub Wiki.
  • I have updated the documentation and Wiki accordingly.

@shanalikhan
Copy link
Owner

Thanks for the PR.
I will look deeply in few days and accept after testing.

But if you look into this link.
https://code.visualstudio.com/api/references/vscode-api#workspace.createFileSystemWatcher

It says

A glob pattern that is applied to the absolute paths of created, changed, and deleted files. Use a relative pattern to limit events to a certain workspace folder.

I think we still can use vscode watcher itself, if they provide watch on absolute paths. If yes, then we only need to fix the paths.

@auxves auxves closed this Mar 24, 2019
@auxves auxves reopened this Mar 24, 2019
@shanalikhan
Copy link
Owner

It was removed on 7716e71#diff-72050d84a8ab08bacc0bd4ac1c1bfb8f by @nekonenene

I think what it means by that is you can watch for a specific folder in your

In 'Workspaces' only or we can set by absolute paths ?

@auxves
Copy link
Contributor Author

auxves commented Mar 24, 2019

On that same page, it says this:

Note that only files within the current workspace folders can be watched.

Use a relative pattern to limit events to a certain workspace folder.

I'm guessing this means that you can use relative paths to watch a subfolder of a workspace folder, like src or src/utilities, but the rules still apply.

@shanalikhan
Copy link
Owner

alright, thanks
I will merge this into next version.

@auxves auxves mentioned this pull request Mar 31, 2019
13 tasks
@auxves
Copy link
Contributor Author

auxves commented Mar 31, 2019

This PR has a bug which has been fixed in #814. Please try that one as it not only fixes this bug, but it also adds some great new features.

@auxves auxves closed this Mar 31, 2019
@shanalikhan
Copy link
Owner

Can you re-open this and fix this independent so I can publish the new version containing this only this fix alongwith other PR ?

@auxves
Copy link
Contributor Author

auxves commented Mar 31, 2019

Sure. However, It might take a while since I am going to be away from home for a few days. I'll try to fix it as soon as possible.

@auxves auxves reopened this Mar 31, 2019
@auxves
Copy link
Contributor Author

auxves commented Mar 31, 2019

I should be able to do it today, but I'm not 100% sure.

@shanalikhan
Copy link
Owner

No problem, we can wait. Give me a signal to test and publish when you are ready.

@shanalikhan shanalikhan changed the base branch from master to v3.2.8 March 31, 2019 21:01
@shanalikhan shanalikhan added this to the v3.2.8 milestone Mar 31, 2019
@auxves
Copy link
Contributor Author

auxves commented Apr 1, 2019

These changes should fix it. Please look out for formatting issues in the code as I used my old laptop, and it seems like it messed up the formatting a bit. Other than that, it should work. If it doesn't, just let me know and I can try to fix it when I get back home.

Copy link
Owner

@shanalikhan shanalikhan left a comment

Choose a reason for hiding this comment

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

I have reviewed the code.
One thing is must that we need to use async/ await where possible.

src/service/fileService.ts Outdated Show resolved Hide resolved
src/service/fileService.ts Outdated Show resolved Hide resolved
src/setting.ts Outdated Show resolved Hide resolved
src/sync.ts Outdated Show resolved Hide resolved
src/sync.ts Outdated Show resolved Hide resolved
src/commons.ts Outdated Show resolved Hide resolved
auxves added 5 commits April 2, 2019 15:24
Since the autoUploadService is an instance in Commons, using a global
Commons instance will allow everything to use the same instance.
@auxves
Copy link
Contributor Author

auxves commented Apr 2, 2019

Ok, I have reverted to using asynchronous functions, and the code is ready to be further tested.

Copy link
Owner

@shanalikhan shanalikhan left a comment

Choose a reason for hiding this comment

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

Thanks. I have added more reviews.

One problem that users were having problem regarding the lock was not working as expected.

You can test the lock by running the following scenarios:

  1. Run auto upload, while its processing. Change one observed file and see, does the autoUpload runs again by destroying the old processing state or waits for it to be complete and ignore the again upload call.
  2. Open 2 code windows. edit any file and run autoupload from one code window by changing any observed file, while auto upload is running - Change the file once again from the second code window and observe - Acceptance will be it should ignore the auto upload process from the second code window and inform user that its already in process.

Can you please let me know after running the above two test cases.

Other than that, can we add some test cases for autoUpload as well ?

These are the final reviews, once done alongwith tests performed. It will be ready to merge and release with new version :)

src/sync.ts Show resolved Hide resolved
src/setting.ts Show resolved Hide resolved
src/commons.ts Show resolved Hide resolved
src/service/autoUploadService.ts Show resolved Hide resolved
@auxves
Copy link
Contributor Author

auxves commented Apr 3, 2019

Here are the results from my testing

  • Run auto upload, while it's processing. Change one observed file and see, does the autoUpload runs again by destroying the old processing state or waits for it to be complete and ignore the again upload call.

Open 2 code windows

I couldn't get two windows open with the development host, but the code still depends on the lock file, so it shouldn't matter how many windows there are open.

@shanalikhan
Copy link
Owner

I couldn't get two windows open with the development host, but the code still depends on the lock file, so it shouldn't matter how many windows there are open.

You can make package from vsce package of extension.
Install extension on your computer from compiled and run the code on both windows.

@auxves
Copy link
Contributor Author

auxves commented Apr 3, 2019

Ok, I'll try that as soon as I can and update the results when I'm done.

@auxves
Copy link
Contributor Author

auxves commented Apr 3, 2019

Ok, I have tested it and can confirm that the lock file works. After starting auto upload on one window, attempting to start it on another window stopped the operation because the lock file was locked.

@shanalikhan shanalikhan merged commit 560b376 into shanalikhan:v3.2.8 Apr 3, 2019
@shanalikhan
Copy link
Owner

Thank you for work.
Its merged and will be released in next version
One more question? Are you available for future bug fixes regarding auto uploads?
I can tag you when where there will be any problem

@auxves
Copy link
Contributor Author

auxves commented Apr 3, 2019

Thanks for merging! And yes, I will be available, just tag me and I'll get to work as soon as possible.

@shanalikhan
Copy link
Owner

New version is published. :)
Thanks for the PR

@shanalikhan shanalikhan mentioned this pull request Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants