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

Dismiss difference fix #146

Merged
merged 4 commits into from
Apr 2, 2024
Merged

Conversation

sampellino
Copy link
Contributor

Fix for issue #145


Before:

before.mp4

After:

after.mp4

@moshfeu
Copy link
Owner

moshfeu commented Mar 24, 2024

Thanks for the quick fix! Please let me a few days to get into it. If you have any idea how to avoid specific platform handling, that would be great. Cross platform code is usually feels better than platform-specific

@sampellino
Copy link
Contributor Author

sampellino commented Mar 25, 2024

Thanks for the quick fix! Please let me a few days to get into it. If you have any idea how to avoid specific platform handling, that would be great. Cross platform code is usually feels better than platform-specific

I've never tried writing a VSCode extension, but I can take a crack at trying to make the code platform agnostic👌

@moshfeu
Copy link
Owner

moshfeu commented Mar 29, 2024

Hi @sampellino :)
The root cause is that the dir-compare package returns different paths in windows (with the scheme) and mac (without the scheme) and I guess that it's because of the different file system

What do you think about changing the filterIgnoredFromDiffs so it would compare only paths (force to remove the schema which is irrelevant here anyway)?

private filterIgnoredFromDiffs() {
this._diffs!.distinct = this._diffs!.distinct
.filter(diff => {
const { path: path1 } = Uri.parse(diff[0]);
const { path: path2 } = Uri.parse(diff[1]);
console.log(path1, path2);
return !this.ignoreDifferencesList.has(path1) &&
!this.ignoreDifferencesList.has(path2);
});
}

Here is in a commit

@moshfeu
Copy link
Owner

moshfeu commented Apr 2, 2024

I'll merge now and will pull my suggestion afterwards
🙏

@moshfeu moshfeu merged commit 8409922 into moshfeu:master Apr 2, 2024
4 checks passed
@sampellino
Copy link
Contributor Author

I'll merge now and will pull my suggestion afterwards 🙏

Hey, sorry I didn't get back to you earlier! I can still try to look into the platform-agnostic solution this week if I get a chance. My job has been taking a lot of my time these past few weeks.

Glad to see it went to prod! Let me know if you need help with any future issues, I'd be more than happy to contribute.

@moshfeu
Copy link
Owner

moshfeu commented Apr 2, 2024

No worries, unfortunately we need to work from time to time 🙃

I've pushed the agnostic platform solution, I'd be happy for you to review it

Thanks for willing to contribute, really appreciated!
Any idea for features or ux improvements would be great

@sampellino
Copy link
Contributor Author

Confirmed working for Win 11 ✅

Recording.2024-04-02.194840.mp4

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.

Clicking "Dismiss Difference" [X] button does nothing + spelling error in tooltip
2 participants