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

Can use incorrect space/tab setting when switching between split screen windows #6

Open
murrayju opened this issue Oct 28, 2014 · 19 comments

Comments

@murrayju
Copy link

I suspect that this is caused by an interaction between this extension, and two others that I am using:

  • Autosave on View Change
  • EditorConfig

The case is, I have two files open in split screen mode. One file is using spaces, the other is using tabs (this happens automatically based on my EditorConfig, one is HTML, the other is JS). If I make some edits in the first file, then switch over to the second file, the autosave plugin saves the first file, which in turn runs this wsSanitizer plugin. The problem is that it uses the spaces/tabs preference from the second file instead of the first.

@kidwm
Copy link

kidwm commented Nov 18, 2014

Hi, I'm the maintainer of brackets-editorconfig, I don't recommend you use wsSanitizer and EditorConfig together, since these two share a lot of same codes and do the similar job.

@MiguelCastillo
Copy link
Owner

@murrayju I am sorry I didn't notice your issues! I haven't used EditorConfig.

@kidwm Pity there was never a pull request... It confuses users when extensions overlap and conflict. Do you think you can productize reditorconfig? Maybe we can make a standalone module that other extensions can consume, including wsSanitizer.

@kidwm
Copy link

kidwm commented Nov 18, 2014

@MiguelCastillo That would be great, but I have no idea how to do that.
Maybe I should remove the same sanitize part with wsSanitizer in brackets-editorconfig to avoid the conflict and push user to install these two extensions both?

@MiguelCastillo
Copy link
Owner

@kidwm The idea is to make editorconfig a generic extension that can broadcast events about changes to the config file, where extensions can opt in to participate in getting config changes. It seems like you are already writing settings to PreferencesManager so extensions can just use that as the event bus. And you are also loading the config files based on the current document. What happens if the config file isn't found where the current document is?

Let me take a close look at what's happening in editorconfig so that we can brainstorm and make this happen. It seems like you just need to blinding write all settings found in the config file into the PreferencesManager. And all an extension would need to do is listen to PreferencesManager changes.

@MiguelCastillo
Copy link
Owner

@kidwm How feasible is it for you to write the entire config file you to the PreferencesManager? https://github.com/kidwm/brackets-editorconfig/blob/master/main.js#L152

Doing that will allow extensions to register for preference changes to this https://github.com/kidwm/brackets-editorconfig/blob/master/main.js#L23

@kidwm
Copy link

kidwm commented Nov 19, 2014

@MiguelCastillo

What happens if the config file isn't found where the current document is?

As you see, the EditorConfig JavaScript core will return an empty object and nothing happens
https://github.com/kidwm/brackets-editorconfig/blob/master/main.js#L152

BTW, I didn't make a pull request for EditorConfig that is just because of this EditorConfig JavaScript core library, I don't think it should be part of wsSanitizer.

@MiguelCastillo
Copy link
Owner

@kidwm Yeah I agree editorconfig does not belong in wsSanitizer and understand why it wasn't a PR. I think editorconfig can be a really great standalone module used by wsSanitizer, which would eliminate having two wsSanitizers. If we can make that work, than I will be integrating all my other extensions as well! :)

What are your thoughts on my comments about having editorconfig write everything it finds into PreferencesManager and letting extensions register for change events?

@kidwm
Copy link

kidwm commented Nov 20, 2014

@MiguelCastillo Actually, I forgot that I have commented out the sanitize part codes in bracket-editorconfig, so it has nothing to do with this issue.

And for you question, I've tried to solve this issue, but found there is a big problem that Brackets seems not offer a method to store such preferences for each file, so it is hard for wsSanitizer to get the right setting after view changed.

Maybe I should write write everything into view state of files?

@murrayju
Copy link
Author

Glad to see you two talking, it does seem like the two plugins could be merged.

As far as the bug reported in this issue goes, I think it has more to do with how wsSanitizer is grabbing the whitespace settings of the incorrect file (due to a race condition where the file is saved after switching to a different tab). I only mentioned EditorConfig as being involved because it is setting the whitespace settings for each file.

@MiguelCastillo
Copy link
Owner

@murrayju I think I see what's happening... This issue seems to be right here https://github.com/MiguelCastillo/Brackets-wsSanitizer/blob/master/main.js#L50

I am grabbing the setting from Editor, which perhaps is the new document... I need to debug this one a bit.

@kidwm
Copy link

kidwm commented Nov 20, 2014

@MiguelCastillo This my attempt to get the correct setting from PreferencesManager with filePath argument assigned instead of Editor.
kidwm/brackets-editorconfig@e97f4fe

But it was not working since the id of preference item is unique regardless of filePath as context.

@MiguelCastillo
Copy link
Owner

That looks really promising. Let me give it a quick test.

@MiguelCastillo
Copy link
Owner

@kidwm I came up with something a bit different. The issue to me is that we want to sanitize a document upon saving it, and the trick was to figure out how to get the correct settings. I am hoping this works :D My tests showed that it did.

@murrayju Do you think can try latest from master branch to see if this resolves the issue for you, please?

@kidwm
Copy link

kidwm commented Nov 20, 2014

@MiguelCastillo
It would not work, I've tried to set context in brackets-editorconfig to test this method but in vain.

I'll try to set PathLayer in Preference to see if it solve this issue.

@MiguelCastillo
Copy link
Owner

@kidwm What wouldn't work? Getting the setting with fullpath?

@kidwm
Copy link

kidwm commented Nov 20, 2014

@MiguelCastillo yes, but I didn't know if I did setting correctly.

@MiguelCastillo
Copy link
Owner

@kidwm Getting the settings with the full path for the document should work. That's how we do it in the editor/Editor.js. I would be very surprised if it didn't and would probably log a bug.

@kidwm
Copy link

kidwm commented Nov 21, 2014

@MiguelCastillo
This is my test case, did I do something wrong?

var MainViewManager    = brackets.getModule('view/MainViewManager');
var PreferencesManager = brackets.getModule('preferences/PreferencesManager');
var files = MainViewManager.getAllOpenFiles();
PreferencesManager.set("useTabChar", true, {context: files[0].fullPath});
PreferencesManager.get("useTabChar", files[0].fullPath);
PreferencesManager.set("useTabChar", false, {context: files[1].fullPath});
PreferencesManager.get("useTabChar", files[1].fullPath);
PreferencesManager.get("useTabChar", files[0].fullPath);

@kidwm
Copy link

kidwm commented Nov 21, 2014

@peterflynn Can verify this test case in my previous comment?

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

No branches or pull requests

3 participants