Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Saving the tabs settings per file/project #3039

Closed
wants to merge 5 commits into from
Closed

Saving the tabs settings per file/project #3039

wants to merge 5 commits into from

Conversation

TomMalbran
Copy link
Contributor

With this pull request changing the tabs type and tabs size will only change it for the current file. It also adds new tab settings for the entire project available at the project settings dialog. This will set all 3 tab settings for every open instance within the project, but only the ones that the file doesn't have saved, and will save them in the persistent storage so that the every new file opened will default to the project settings.

@ghost ghost assigned peterflynn Mar 8, 2013
@peterflynn
Copy link
Member

@TomMalbran I haven't done an in-depth code review of this yet, but I have a couple high-level concerns first:

  • The way per-file preferences are being stored feels like a bit of a hack. There's no mechanism for cleaning up stale prefs for files that no longer exist (or will never be opened again), for example. The existing per-project prefs are a little shaky, and this pushes it further -- so maybe it's time to build some actual preferences infrastructure for this stuff. (Which would also allow us to make it more robust to file/folder renames, etc.)
  • We may want to put some more work into preferences dialog infrastructure too. For example, it doesn't seem scalable that all callers of showProjectPreferencesDialog() have to fetch all the right prefs & pass them in... But I think without too many other use cases for prefs yet it will be hard to get that right -- so in a way this is a little "ahead of its time."
  • It seems like the UI needs to change to communicate the global/per-file duality more clearly. As it stands, users may not understand that the statusbar settings only affect one file (not even one file extension generally) and may have no idea how to change the setting more globally (or may not even know that it's possible). Might be worth pinging @GarthDB or NJ for their thoughts, or having a newsgroup thread...
  • There's no way to revert an editor back to using the project default (seems like changing the settings to match the current defaults should do this).
  • ProjectManager seems like the wrong place to add code for every pref (I think the base URL set a bad precedent there). I'm not entirely sure what the right place is... Editor and EditorManager are already pretty crowded, so I guess I'd vote for EditorStatusBar for now? (Later, I could picture us having various feature-area preferences modules that manage both the settings APIs and their chunk of the prefs dialog UI).
  • The isWithinProject() filtering in Editor.setProjectTabs() seems like it could confuse people. It also seems incorrect, given that freshly opened editors will respect the global setting regardless of whether they're within the project tree or not.
  • Multiple editors can be open for the same file (e.g. considering inline editors), so there needs to be some cross-_instances syncing even when the setting is changed just for one file.

So, that's a whole bunch of stuff to tackle... and the infrastructure & UI level discussions could take some time. I wonder whether you could address many of the same pain points by starting with tab setting autodetection per file, without manual per-file overrides. That seems like an easier thing to land first since it doesn't raise as many "big" questions/changes. Thoughts?


// Error message
if (errorMessage) {
$dlg.find(".settings-list").append("<div class='alert-message' style='margin-bottom: 0'>" + errorMessage + "</div>");
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for cleaning this error message stuff up btw -- much nicer the way you've done it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I could commit this changes as a separate request if this one isn't merged.

@TomMalbran
Copy link
Contributor Author

@peterflynn Thanks for the review, this are some of my thoughts/ideas:

  1. I see the problem there, and it doesn't seem to be an easy fix for that yet. It would require more research and lots of code, that should be addressed in other request probably.
  2. An easy fix could be a new ProjectPreferences file that had the dialog and all the preferences for the project. Then showProjectPreferencesDialog could just grab all the preferences needed from the functions in this file. A better way would be to have the project settings for each file saved as a JSON object and use mustache to show the content of the file and then save it back getting the input values and the names as some sort of key.
  3. One idea I had but didn't implemented, was having a preferences to determine what the status bar preferences affected. So if it was in global, changing the tab preferences would change them for every file like now. And then it could have project and file to deal with the new 2 modes. But then you wouldn't be able change individual files inside the project, in the global or project mode, unless this was a per file preference too.
  4. I thought about doing that, but maybe you have a thirdparty file with its own preferences (it uses spaces, for example), and might not want to change them if you decide to start using tabs instead of spaces for the entire project. But maybe using the option in point 3 would solve this.
  5. EditorStatusBar sounds good or as mentioned in point 2 a new file.
  6. Actually freshly opened editors should use the file saved preferences or default to the project ones, since the preferences are obtained from Editor.prototype._getTabPreference. But I noticed that it is missing a isWithinProject filter to default to the global ones for none project files.
  7. Right, I haven't thought of that.

This seems all doable, except for point 1. I was doing this since I am working on another project besides Brackets that uses tabs instead of spaces, so I forget many times to change the settings and end up commiting with tabs instead of spaces. Both just having only project preferences and autodetecting per file could solve this. I added per file just to go with the Trello card and it should be useful too.

I didn't thought that auto-detection per file could solve my problem. So the idea would be a global preference that when enabled, it would check the tabs/spaces every time you switch editors and update the global preferences to what was found in the file?

Just having project preferences would be nice too, but I guess that would still have several of those open/big questions.

And, do you want me to address the merge issues anyway?

@peterflynn
Copy link
Member

Oh, one other issue I just realized: afaict with this change as-is, there's no longer any way to set a global (not per-proj, not per-file) indentation preference. So if someone really dislikes our defaults, they'll have to manually reset them for every new project opened -- probably not ideal.

@TomMalbran
Copy link
Contributor Author

Yes, I realized that, but I couldn't figure where to add this, since there isn't yet a preferences dialog for Brackets, and this aren't true/false preferences. My suggestion in point 3, could solve this, but without the dual mode.

So I am guessing that this pull is ahead of its time. Might be better to leave it until there is a better way to set the global, project and per file preferences.

@TomMalbran
Copy link
Contributor Author

@peterflynn This requests looks like a no gone at this point in Brackets, since point 1 isn't fixable right now, and other need better planning or research. I am going to just close this, but let me know if there is something i can do to bring it back.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants