Skip to content

make livesync work with renamed files and folders #3042

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Aug 17, 2017
Merged

make livesync work with renamed files and folders #3042

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94eeebb
added special check for file changes on Mac
Plamen5kov Aug 9, 2017
d7f06d2
fix missing imports in tests
Plamen5kov Aug 9, 2017
3692d8b
add dir check, modified file check
Plamen5kov Aug 10, 2017
c861d06
check isDirModified before anything else to save time
Plamen5kov Aug 10, 2017
3b8cb02
updates after review
Plamen5kov Aug 11, 2017
3c2713c
bring back check for symlinks
Plamen5kov Aug 14, 2017
9020a86
add check for modification in bot files and folders
Plamen5kov Aug 14, 2017
f99697d
fixed lint
Plamen5kov Aug 14, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lib/services/project-changes-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ export class ProjectChangesService implements IProjectChangesService {
constructor(
private $platformsData: IPlatformsData,
private $devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
private $fs: IFileSystem) {
private $fs: IFileSystem,
private $hostInfo: IHostInfo) {
}

public get currentChanges(): IProjectChangesInfo {
Expand Down Expand Up @@ -227,10 +228,15 @@ export class ProjectChangesService implements IProjectChangesService {

let fileStats = this.$fs.getFsStats(filePath);

let changed = fileStats.mtime.getTime() >= this._outputProjectMtime || fileStats.ctime.getTime() >= this._outputProjectCTime;
if (!changed) {
let lFileStats = this.$fs.getLsStats(filePath);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this code deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's unnecessary to do this.$fs.getLsStats(filePath) twice and the logic is kept the same in the refactored version

Copy link
Contributor

Choose a reason for hiding this comment

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

getLstStats and getFsStats are different. This code is used when the file/dir is symlink - we need to check the symlink itself (for example if it is recreated) and this is done by getLsStats.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @Plamen5kov

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 will bring back the code, so we continue working with synlinked files.

changed = lFileStats.mtime.getTime() >= this._outputProjectMtime || lFileStats.ctime.getTime() >= this._outputProjectCTime;
let changed = fileStats.mtime.getTime() > this._outputProjectMtime ||
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this without = sign, but the next check has it? I believe you need the = in both cases, as the _outputProjectMtime is set after the prepare. In case it is equal with a file's mtime, this means the file has been modified when the prepare is finishing (exactly at the same time), so we need to prepare the project again.

fileStats.ctime.getTime() >= this._outputProjectCTime;

if (this.$hostInfo.isDarwin && !fileStats.isDirectory() && !changed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only for darwin? The case is for the File System itself, not the OS, so it may happen on different OSes.

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 agree with you, but I don't know a trivial way of finding out what operating system is the code running on. That's why I check for darwin, because that's where the problem presents itself.

const parentDirPath: string = path.dirname(filePath);
const parentDirStat = this.$fs.getFsStats(parentDirPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

In case we have 50 files in the current dir and nothing is changed (including the parent dir), we'll get the stats of the parent directory 50 times.
Also, consider the following structure:

-- app
   |
   -- components
       |
        -- file1
        -- file 2

We'll call containsNewerFiles for app dir. It will list the contents of the app dir and for each of them it will check the ctime and mtime, i.e. it will check the times of components directory.
In case there's no change, the method will recursively check the contents of the components directory. At this point we'll check file1 and file2 and for each of them we'll get the parent dir (components) and check its ctime and mtime. But we've already done this in the previous step, why should we do it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rosen-vladimirov I did this as a temporary solution because I needed to test the fix on Mac and I needed to pull the code from it. I'm sorry if I inconvenienced you. I'm currently working on an optimized, permanent solution and will ping you when ready.


changed = parentDirStat.mtime.getTime() > this._outputProjectMtime ||
parentDirStat.ctime.getTime() >= this._outputProjectCTime;
}

if (changed) {
Expand Down
5 changes: 4 additions & 1 deletion test/project-changes-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { PlatformsData } from "../lib/platforms-data";
import { ProjectChangesService } from "../lib/services/project-changes-service";
import * as Constants from "../lib/constants";
import { FileSystem } from "../lib/common/file-system";
import * as HostInfoLib from "../lib/common/host-info";
import * as ErrorsLib from "../lib/common/errors";

// start tracking temporary folders/files
temp.track();
Expand All @@ -26,7 +28,8 @@ class ProjectChangesServiceTest extends BaseServiceTest {
this.injector.register("platformsData", PlatformsData);
this.injector.register("androidProjectService", {});
this.injector.register("iOSProjectService", {});

this.injector.register("hostInfo", HostInfoLib.HostInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used and shouldn't be added

this.injector.register("errors", ErrorsLib.Errors);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not used and shouldn't be added

this.injector.register("fs", FileSystem);
this.injector.register("devicePlatformsConstants", {});
this.injector.register("devicePlatformsConstants", {});
Expand Down